[PATCHv3 0/8] target: Save memory on unused se_dev_entrys and se_luns

[Andy Grover]

Hi nab, hch, and all,

This patchset reduces the amount of memory for se_dev_entry and se_lun
arrays by waiting to allocate array members until they are
created. This patch saves up to 261KB per TPG, and up to 65KB per
ACL. It also fixes a number of locking bugs around these data
structures.

Changes since v2, based on hch's review:
* Fix braces and add precondition checking in
core_enable_device_list_for_node() in patch 2
* Move busy-loop outside of spinlock in
core_disable_device_list_for_node() in patch 3
* Merge patches 5 and 6

Against target-pending/for-next (3.16-rc2).

Thanks -- Regards -- Andy

Andy Grover (8):
target: Add locking to some accesses to nacl.device_list
target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs
target: Allocate se_dev_entrys in device list only when used
target: core_tpg_post_dellun can return void
target: Change core_dev_del_lun to take a se_lun instead of
unpacked_lun
target: Allocate se_luns only when used
target: Remove core_tpg_release_virtual_lun0 function
target: Refactor core_enable_device_list_for_node

drivers/target/sbp/sbp_target.c | 6 +-
drivers/target/target_core_device.c | 313 +++++++++++++--------------
drivers/target/target_core_fabric_configfs.c | 35 +--
drivers/target/target_core_internal.h | 9 +-
drivers/target/target_core_pr.c | 40 +++-
drivers/target/target_core_spc.c | 2 +-
drivers/target/target_core_tpg.c | 189 +++-------------
drivers/target/target_core_ua.c | 3 +
include/target/target_core_base.h | 17 +-
9 files changed, 248 insertions(+), 366 deletions(-)

[Andy Grover]

Make sure all accesses of deve elements in device_list are protected. This
will become critically important once device_list entries are potentially
NULL.

Reported-by: Christoph Hellwig <***@lst.de>
Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_device.c | 7 ++---
drivers/target/target_core_fabric_configfs.c | 10 +++++--
drivers/target/target_core_pr.c | 40 +++++++++++++++++++++-------
drivers/target/target_core_ua.c | 3 +++
4 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 11d26fe..10130ea 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -361,11 +361,12 @@ int core_enable_device_list_for_node(

deve->creation_time = get_jiffies_64();
deve->attach_count++;
- spin_unlock_irq(&nacl->device_list_lock);

- spin_lock_bh(&port->sep_alua_lock);
+ spin_lock(&port->sep_alua_lock);
list_add_tail(&deve->alua_port_list, &port->sep_alua_list);
- spin_unlock_bh(&port->sep_alua_lock);
+ spin_unlock(&port->sep_alua_lock);
+
+ spin_unlock_irq(&nacl->device_list_lock);

return 0;
}
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 7de9f04..804e7f0 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -141,13 +141,19 @@ static int target_fabric_mappedlun_unlink(
struct se_lun_acl *lacl = container_of(to_config_group(lun_acl_ci),
struct se_lun_acl, se_lun_group);
struct se_node_acl *nacl = lacl->se_lun_nacl;
- struct se_dev_entry *deve = nacl->device_list[lacl->mapped_lun];
+ struct se_dev_entry *deve;
struct se_portal_group *se_tpg;
+
+ spin_lock_irq(&nacl->device_list_lock);
+ deve = nacl->device_list[lacl->mapped_lun];
/*
* Determine if the underlying MappedLUN has already been released..
*/
- if (!deve->se_lun)
+ if (!deve->se_lun) {
+ spin_unlock_irq(&nacl->device_list_lock);
return 0;
+ }
+ spin_unlock_irq(&nacl->device_list_lock);

lun = container_of(to_config_group(lun_ci), struct se_lun, lun_group);
se_tpg = lun->lun_sep->sep_tpg;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index df35786..c647ca9 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -311,8 +311,13 @@ static int core_scsi3_pr_seq_non_holder(
int we = 0; /* Write Exclusive */
int legacy = 0; /* Act like a legacy device and return
* RESERVATION CONFLICT on some CDBs */
+ int def_pr_registered;

+ spin_lock_irq(&se_sess->se_node_acl->device_list_lock);
se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];
+ def_pr_registered = se_deve->def_pr_registered;
+ spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
+
/*
* Determine if the registration should be ignored due to
* non-matching ISIDs in target_scsi3_pr_reservation_check().
@@ -329,7 +334,7 @@ static int core_scsi3_pr_seq_non_holder(
* Some commands are only allowed for the persistent reservation
* holder.
*/
- if ((se_deve->def_pr_registered) && !(ignore_reg))
+ if ((def_pr_registered) && !(ignore_reg))
registered_nexus = 1;
break;
case PR_TYPE_WRITE_EXCLUSIVE_REGONLY:
@@ -339,7 +344,7 @@ static int core_scsi3_pr_seq_non_holder(
* Some commands are only allowed for registered I_T Nexuses.
*/
reg_only = 1;
- if ((se_deve->def_pr_registered) && !(ignore_reg))
+ if ((def_pr_registered) && !(ignore_reg))
registered_nexus = 1;
break;
case PR_TYPE_WRITE_EXCLUSIVE_ALLREG:
@@ -349,7 +354,7 @@ static int core_scsi3_pr_seq_non_holder(
* Each registered I_T Nexus is a reservation holder.
*/
all_reg = 1;
- if ((se_deve->def_pr_registered) && !(ignore_reg))
+ if ((def_pr_registered) && !(ignore_reg))
registered_nexus = 1;
break;
default:
@@ -947,13 +952,21 @@ int core_scsi3_check_aptpl_registration(
struct se_lun_acl *lun_acl)
{
struct se_node_acl *nacl = lun_acl->se_lun_nacl;
- struct se_dev_entry *deve = nacl->device_list[lun_acl->mapped_lun];
+ struct se_dev_entry *deve;
+ int ret;

if (dev->dev_reservation_flags & DRF_SPC2_RESERVATIONS)
return 0;

- return __core_scsi3_check_aptpl_registration(dev, tpg, lun,
- lun->unpacked_lun, nacl, deve);
+ spin_lock_irq(&nacl->device_list_lock);
+ deve = nacl->device_list[lun_acl->mapped_lun];
+
+ ret = __core_scsi3_check_aptpl_registration(dev, tpg, lun,
+ lun->unpacked_lun, nacl, deve);
+
+ spin_unlock_irq(&nacl->device_list_lock);
+
+ return ret;
}

static void __core_scsi3_dump_registration(
@@ -1451,7 +1464,6 @@ core_scsi3_decode_spec_i_port(

memset(dest_iport, 0, 64);

- local_se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];
/*
* Allocate a struct pr_transport_id_holder and setup the
* local_node_acl and local_se_deve pointers and add to
@@ -1466,11 +1478,18 @@ core_scsi3_decode_spec_i_port(
INIT_LIST_HEAD(&tidh_new->dest_list);
tidh_new->dest_tpg = tpg;
tidh_new->dest_node_acl = se_sess->se_node_acl;
+
+ spin_lock_irq(&se_sess->se_node_acl->device_list_lock);
+ local_se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];
+
tidh_new->dest_se_deve = local_se_deve;

local_pr_reg = __core_scsi3_alloc_registration(cmd->se_dev,
se_sess->se_node_acl, local_se_deve, l_isid,
sa_res_key, all_tg_pt, aptpl);
+
+ spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
+
if (!local_pr_reg) {
kfree(tidh_new);
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
@@ -2002,7 +2021,6 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
{
struct se_session *se_sess = cmd->se_sess;
struct se_device *dev = cmd->se_dev;
- struct se_dev_entry *se_deve;
struct se_lun *se_lun = cmd->se_lun;
struct se_portal_group *se_tpg;
struct t10_pr_registration *pr_reg, *pr_reg_p, *pr_reg_tmp;
@@ -2016,7 +2034,6 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
}
se_tpg = se_sess->se_tpg;
- se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];

if (se_tpg->se_tpg_tfo->sess_get_initiator_sid) {
memset(&isid_buf[0], 0, PR_REG_ISID_LEN);
@@ -2041,19 +2058,24 @@ core_scsi3_emulate_pro_register(struct se_cmd *cmd, u64 res_key, u64 sa_res_key,
return 0;

if (!spec_i_pt) {
+ struct se_dev_entry *se_deve;
/*
* Perform the Service Action REGISTER on the Initiator
* Port Endpoint that the PRO was received from on the
* Logical Unit of the SCSI device server.
*/
+ spin_lock_irq(&se_sess->se_node_acl->device_list_lock);
+ se_deve = se_sess->se_node_acl->device_list[cmd->orig_fe_lun];
if (core_scsi3_alloc_registration(cmd->se_dev,
se_sess->se_node_acl, se_deve, isid_ptr,
sa_res_key, all_tg_pt, aptpl,
register_type, 0)) {
pr_err("Unable to allocate"
" struct t10_pr_registration\n");
+ spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
return TCM_INVALID_PARAMETER_LIST;
}
+ spin_unlock_irq(&se_sess->se_node_acl->device_list_lock);
} else {
/*
* Register both the Initiator port that received
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index 101858e..5c9d980 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -51,9 +51,12 @@ target_scsi3_ua_check(struct se_cmd *cmd)
if (!nacl)
return 0;

+ spin_lock_irq(&nacl->device_list_lock);
deve = nacl->device_list[cmd->orig_fe_lun];
if (!atomic_read(&deve->ua_count))
return 0;
+ spin_unlock_irq(&nacl->device_list_lock);
+
/*
* From sam4r14, section 5.14 Unit attention condition:
*

[Maurizio Lombardi]

Hi Andy,

Post by Andy Grover
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index 101858e..5c9d980 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -51,9 +51,12 @@ target_scsi3_ua_check(struct se_cmd *cmd)
if (!nacl)
return 0;
+ spin_lock_irq(&nacl->device_list_lock);
deve = nacl->device_list[cmd->orig_fe_lun];
if (!atomic_read(&deve->ua_count))
return 0;
+ spin_unlock_irq(&nacl->device_list_lock);
+

Shouldn't the spinlock unlocked before the "return 0;" ?

Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Andy Grover]

Reported-by: Maurizio Lombardi <***@redhat.com>
Signed-off-by: Andy Grover <***@redhat.com>
---

Hi Maurizio, yup, thanks!

Here's a patch that fixes the issue - it can be applied as 9/8 or
alternatively squashed into 1/8.

Thanks again -- Regards -- Andy


drivers/target/target_core_ua.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index 5c9d980..0eac3a9 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -53,8 +53,10 @@ target_scsi3_ua_check(struct se_cmd *cmd)

spin_lock_irq(&nacl->device_list_lock);
deve = nacl->device_list[cmd->orig_fe_lun];
- if (!atomic_read(&deve->ua_count))
+ if (!atomic_read(&deve->ua_count)) {
+ spin_unlock_irq(&nacl->device_list_lock);
return 0;
+ }
spin_unlock_irq(&nacl->device_list_lock);

/*

[Andy Grover]

Simple and just called from one place.

Reviewed-by: Christoph Hellwig <***@lst.de>
Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_tpg.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index a63a3d7..bc0299a 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -591,13 +591,6 @@ static int core_tpg_setup_virtual_lun0(struct se_portal_group *se_tpg)
return 0;
}

-static void core_tpg_release_virtual_lun0(struct se_portal_group *se_tpg)
-{
- struct se_lun *lun = &se_tpg->tpg_virt_lun0;
-
- core_tpg_remove_lun(se_tpg, lun);
-}
-
int core_tpg_register(
struct target_core_fabric_ops *tfo,
struct se_wwn *se_wwn,
@@ -673,7 +666,7 @@ int core_tpg_deregister(struct se_portal_group *se_tpg)
spin_unlock_irq(&se_tpg->acl_node_lock);

if (se_tpg->se_tpg_type == TRANSPORT_TPG_TYPE_NORMAL)
- core_tpg_release_virtual_lun0(se_tpg);
+ core_tpg_remove_lun(se_tpg, &se_tpg->tpg_virt_lun0);

se_tpg->se_tpg_fabric_ptr = NULL;

[Andy Grover]

Instead of allocating the tpg_lun_list and each member of the list when
the tpg is created, make the list part of the parent struct, and use an
element being non-NULL to indicate an active LUN. This will save memory
if less than all the se_luns are actually configured.

Now that things are actually getting freed, split out core_tpg_free_lun
from core_tpg_remove_lun, because we don't want to free() the statically-
allocated virtual_lun0.

Remove array_free and array_zalloc.

Remove core_get_lun_from_tpg.

Change core_dev_add_lun to take a se_lun and return int

Do not allocate tpg_lun_list, put it directly in se_portal_group.

Reviewed-by: Christoph Hellwig <***@lst.de>
Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/sbp/sbp_target.c | 6 +-
drivers/target/target_core_device.c | 43 ++----------
drivers/target/target_core_fabric_configfs.c | 21 +++---
drivers/target/target_core_internal.h | 4 +-
drivers/target/target_core_tpg.c | 101 +++++++++------------------
include/target/target_core_base.h | 10 +--
6 files changed, 54 insertions(+), 131 deletions(-)

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index e7e9372..62b9d47 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -187,7 +187,7 @@ static struct se_lun *sbp_get_lun_from_tpg(struct sbp_tpg *tpg, int lun)
spin_lock(&se_tpg->tpg_lun_lock);
se_lun = se_tpg->tpg_lun_list[lun];

- if (se_lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE)
+ if (!se_lun)
se_lun = ERR_PTR(-ENODEV);

spin_unlock(&se_tpg->tpg_lun_lock);
@@ -1947,7 +1947,7 @@ static int sbp_count_se_tpg_luns(struct se_portal_group *tpg)
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
struct se_lun *se_lun = tpg->tpg_lun_list[i];

- if (se_lun->lun_status == TRANSPORT_LUN_STATUS_FREE)
+ if (!se_lun)
continue;

count++;
@@ -2027,7 +2027,7 @@ static int sbp_update_unit_directory(struct sbp_tport *tport)
struct se_device *dev;
int type;

- if (se_lun->lun_status == TRANSPORT_LUN_STATUS_FREE)
+ if (!se_lun)
continue;

spin_unlock(&tport->tpg->se_tpg.tpg_lun_lock);
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index bd467a4..4b8149b 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -1217,22 +1217,17 @@ int se_dev_set_block_size(struct se_device *dev, u32 block_size)
return 0;
}

-struct se_lun *core_dev_add_lun(
+int core_dev_add_lun(
struct se_portal_group *tpg,
struct se_device *dev,
- u32 unpacked_lun)
+ struct se_lun *lun)
{
- struct se_lun *lun;
int rc;

- lun = core_tpg_alloc_lun(tpg, unpacked_lun);
- if (IS_ERR(lun))
- return lun;
-
rc = core_tpg_add_lun(tpg, lun,
TRANSPORT_LUNFLAGS_READ_WRITE, dev);
if (rc < 0)
- return ERR_PTR(rc);
+ return rc;

pr_debug("%s_TPG[%u]_LUN[%u] - Activated %s Logical Unit from"
" CORE HBA: %u\n", tpg->se_tpg_tfo->get_fabric_name(),
@@ -1257,7 +1252,7 @@ struct se_lun *core_dev_add_lun(
spin_unlock_irq(&tpg->acl_node_lock);
}

- return lun;
+ return rc;
}

/* core_dev_del_lun():
@@ -1274,35 +1269,7 @@ void core_dev_del_lun(
tpg->se_tpg_tfo->get_fabric_name());

core_tpg_remove_lun(tpg, lun);
-}
-
-struct se_lun *core_get_lun_from_tpg(struct se_portal_group *tpg, u32 unpacked_lun)
-{
- struct se_lun *lun;
-
- spin_lock(&tpg->tpg_lun_lock);
- if (unpacked_lun > (TRANSPORT_MAX_LUNS_PER_TPG-1)) {
- pr_err("%s LUN: %u exceeds TRANSPORT_MAX_LUNS"
- "_PER_TPG-1: %u for Target Portal Group: %hu\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- TRANSPORT_MAX_LUNS_PER_TPG-1,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- spin_unlock(&tpg->tpg_lun_lock);
- return NULL;
- }
- lun = tpg->tpg_lun_list[unpacked_lun];
-
- if (lun->lun_status != TRANSPORT_LUN_STATUS_FREE) {
- pr_err("%s Logical Unit Number: %u is not free on"
- " Target Portal Group: %hu, ignoring request.\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- spin_unlock(&tpg->tpg_lun_lock);
- return NULL;
- }
- spin_unlock(&tpg->tpg_lun_lock);
-
- return lun;
+ core_tpg_free_lun(tpg, lun);
}

struct se_lun_acl *core_dev_init_initiator_node_lun_acl(
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 811b2f4..95c1cd5 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -760,7 +760,6 @@ static int target_fabric_port_link(
struct config_item *tpg_ci;
struct se_lun *lun = container_of(to_config_group(lun_ci),
struct se_lun, lun_group);
- struct se_lun *lun_p;
struct se_portal_group *se_tpg;
struct se_device *dev =
container_of(to_config_group(se_dev_ci), struct se_device, dev_group);
@@ -788,11 +787,10 @@ static int target_fabric_port_link(
return -EEXIST;
}

- lun_p = core_dev_add_lun(se_tpg, dev, lun->unpacked_lun);
- if (IS_ERR(lun_p)) {
+ ret = core_dev_add_lun(se_tpg, dev, lun);
+ if (ret < 0) {
pr_err("core_dev_add_lun() failed\n");
- ret = PTR_ERR(lun_p);
- goto out;
+ return ret;
}

if (tf->tf_ops.fabric_post_link) {
@@ -805,8 +803,6 @@ static int target_fabric_port_link(
}

return 0;
-out:
- return ret;
}

static int target_fabric_port_unlink(
@@ -892,16 +888,17 @@ static struct config_group *target_fabric_make_lun(
if (unpacked_lun > UINT_MAX)
return ERR_PTR(-EINVAL);

- lun = core_get_lun_from_tpg(se_tpg, unpacked_lun);
- if (!lun)
- return ERR_PTR(-EINVAL);
+ lun = core_tpg_alloc_lun(se_tpg, unpacked_lun);
+ if (IS_ERR(lun))
+ return ERR_CAST(lun);

lun_cg = &lun->lun_group;
lun_cg->default_groups = kmalloc(sizeof(struct config_group *) * 2,
GFP_KERNEL);
if (!lun_cg->default_groups) {
pr_err("Unable to allocate lun_cg->default_groups\n");
- return ERR_PTR(-ENOMEM);
+ errno = -ENOMEM;
+ goto out;
}

config_group_init_type_name(&lun->lun_group, name,
@@ -923,6 +920,8 @@ static struct config_group *target_fabric_make_lun(

return &lun->lun_group;
out:
+ if (lun)
+ core_tpg_free_lun(se_tpg, lun);
if (lun_cg)
kfree(lun_cg->default_groups);
return ERR_PTR(errno);
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 42ef4ab..4f3e6d5 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -45,9 +45,8 @@ int se_dev_set_max_sectors(struct se_device *, u32);
int se_dev_set_fabric_max_sectors(struct se_device *, u32);
int se_dev_set_optimal_sectors(struct se_device *, u32);
int se_dev_set_block_size(struct se_device *, u32);
-struct se_lun *core_dev_add_lun(struct se_portal_group *, struct se_device *, u32);
+int core_dev_add_lun(struct se_portal_group *, struct se_device *, struct se_lun *);
void core_dev_del_lun(struct se_portal_group *, struct se_lun *);
-struct se_lun *core_get_lun_from_tpg(struct se_portal_group *, u32);
struct se_lun_acl *core_dev_init_initiator_node_lun_acl(struct se_portal_group *,
struct se_node_acl *, u32, int *);
int core_dev_add_initiator_node_lun_acl(struct se_portal_group *,
@@ -83,6 +82,7 @@ struct se_lun *core_tpg_alloc_lun(struct se_portal_group *, u32);
int core_tpg_add_lun(struct se_portal_group *, struct se_lun *,
u32, struct se_device *);
void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);
+void core_tpg_free_lun(struct se_portal_group *, struct se_lun *);

/* target_core_transport.c */
extern struct kmem_cache *se_tmr_req_cache;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 5d2c774..a63a3d7 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -134,7 +134,7 @@ void core_tpg_add_node_to_devs(
spin_lock(&tpg->tpg_lun_lock);
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
lun = tpg->tpg_lun_list[i];
- if (lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE)
+ if (!lun)
continue;

dev = lun->lun_se_dev;
@@ -186,34 +186,6 @@ static int core_set_queue_depth_for_node(
return 0;
}

-void array_free(void *array, int n)
-{
- void **a = array;
- int i;
-
- for (i = 0; i < n; i++)
- kfree(a[i]);
- kfree(a);
-}
-
-static void *array_zalloc(int n, size_t size, gfp_t flags)
-{
- void **a;
- int i;
-
- a = kzalloc(n * sizeof(void*), flags);
- if (!a)
- return NULL;
- for (i = 0; i < n; i++) {
- a[i] = kzalloc(size, flags);
- if (!a[i]) {
- array_free(a, n);
- return NULL;
- }
- }
- return a;
-}
-
/* core_tpg_check_initiator_node_acl()
*
*
@@ -293,8 +265,7 @@ void core_tpg_clear_object_luns(struct se_portal_group *tpg)
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
lun = tpg->tpg_lun_list[i];

- if ((lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE) ||
- (lun->lun_se_dev == NULL))
+ if ((!lun) || (lun->lun_se_dev == NULL))
continue;

spin_unlock(&tpg->tpg_lun_lock);
@@ -606,7 +577,6 @@ static int core_tpg_setup_virtual_lun0(struct se_portal_group *se_tpg)
int ret;

lun->unpacked_lun = 0;
- lun->lun_status = TRANSPORT_LUN_STATUS_FREE;
atomic_set(&lun->lun_acl_count, 0);
init_completion(&lun->lun_shutdown_comp);
INIT_LIST_HEAD(&lun->lun_acl_list);
@@ -635,30 +605,6 @@ int core_tpg_register(
void *tpg_fabric_ptr,
int se_tpg_type)
{
- struct se_lun *lun;
- u32 i;
-
- se_tpg->tpg_lun_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG,
- sizeof(struct se_lun), GFP_KERNEL);
- if (!se_tpg->tpg_lun_list) {
- pr_err("Unable to allocate struct se_portal_group->"
- "tpg_lun_list\n");
- return -ENOMEM;
- }
-
- for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
- lun = se_tpg->tpg_lun_list[i];
- lun->unpacked_lun = i;
- lun->lun_link_magic = SE_LUN_LINK_MAGIC;
- lun->lun_status = TRANSPORT_LUN_STATUS_FREE;
- atomic_set(&lun->lun_acl_count, 0);
- init_completion(&lun->lun_shutdown_comp);
- INIT_LIST_HEAD(&lun->lun_acl_list);
- spin_lock_init(&lun->lun_acl_lock);
- spin_lock_init(&lun->lun_sep_lock);
- init_completion(&lun->lun_ref_comp);
- }
-
se_tpg->se_tpg_type = se_tpg_type;
se_tpg->se_tpg_fabric_ptr = tpg_fabric_ptr;
se_tpg->se_tpg_tfo = tfo;
@@ -671,13 +617,9 @@ int core_tpg_register(
spin_lock_init(&se_tpg->session_lock);
spin_lock_init(&se_tpg->tpg_lun_lock);

- if (se_tpg->se_tpg_type == TRANSPORT_TPG_TYPE_NORMAL) {
- if (core_tpg_setup_virtual_lun0(se_tpg) < 0) {
- array_free(se_tpg->tpg_lun_list,
- TRANSPORT_MAX_LUNS_PER_TPG);
+ if (se_tpg->se_tpg_type == TRANSPORT_TPG_TYPE_NORMAL)
+ if (core_tpg_setup_virtual_lun0(se_tpg) < 0)
return -ENOMEM;
- }
- }

spin_lock_bh(&tpg_lock);
list_add_tail(&se_tpg->se_tpg_node, &tpg_list);
@@ -734,7 +676,7 @@ int core_tpg_deregister(struct se_portal_group *se_tpg)
core_tpg_release_virtual_lun0(se_tpg);

se_tpg->se_tpg_fabric_ptr = NULL;
- array_free(se_tpg->tpg_lun_list, TRANSPORT_MAX_LUNS_PER_TPG);
+
return 0;
}
EXPORT_SYMBOL(core_tpg_deregister);
@@ -743,7 +685,7 @@ struct se_lun *core_tpg_alloc_lun(
struct se_portal_group *tpg,
u32 unpacked_lun)
{
- struct se_lun *lun;
+ struct se_lun *lun, *new_lun;

if (unpacked_lun > (TRANSPORT_MAX_LUNS_PER_TPG-1)) {
pr_err("%s LUN: %u exceeds TRANSPORT_MAX_LUNS_PER_TPG"
@@ -754,9 +696,14 @@ struct se_lun *core_tpg_alloc_lun(
return ERR_PTR(-EOVERFLOW);
}

+ new_lun = kzalloc(sizeof(*lun), GFP_KERNEL);
+ if (!new_lun)
+ return ERR_PTR(-ENOMEM);
+
spin_lock(&tpg->tpg_lun_lock);
lun = tpg->tpg_lun_list[unpacked_lun];
- if (lun->lun_status == TRANSPORT_LUN_STATUS_ACTIVE) {
+ if (lun) {
+ kfree(new_lun);
pr_err("TPG Logical Unit Number: %u is already active"
" on %s Target Portal Group: %u, ignoring request.\n",
unpacked_lun, tpg->se_tpg_tfo->get_fabric_name(),
@@ -764,9 +711,21 @@ struct se_lun *core_tpg_alloc_lun(
spin_unlock(&tpg->tpg_lun_lock);
return ERR_PTR(-EINVAL);
}
+
+ new_lun->unpacked_lun = unpacked_lun;
+ new_lun->lun_link_magic = SE_LUN_LINK_MAGIC;
+ atomic_set(&new_lun->lun_acl_count, 0);
+ init_completion(&new_lun->lun_shutdown_comp);
+ INIT_LIST_HEAD(&new_lun->lun_acl_list);
+ spin_lock_init(&new_lun->lun_acl_lock);
+ spin_lock_init(&new_lun->lun_sep_lock);
+ init_completion(&new_lun->lun_ref_comp);
+
+ tpg->tpg_lun_list[unpacked_lun] = new_lun;
+
spin_unlock(&tpg->tpg_lun_lock);

- return lun;
+ return new_lun;
}

int core_tpg_add_lun(
@@ -789,7 +748,6 @@ int core_tpg_add_lun(

spin_lock(&tpg->tpg_lun_lock);
lun->lun_access = lun_access;
- lun->lun_status = TRANSPORT_LUN_STATUS_ACTIVE;
spin_unlock(&tpg->tpg_lun_lock);

return 0;
@@ -802,8 +760,15 @@ void core_tpg_remove_lun(
core_clear_lun_from_tpg(lun, tpg);
transport_clear_lun_ref(lun);
core_dev_unexport(lun->lun_se_dev, tpg, lun);
+}

+void core_tpg_free_lun(
+ struct se_portal_group *tpg,
+ struct se_lun *lun)
+{
spin_lock(&tpg->tpg_lun_lock);
- lun->lun_status = TRANSPORT_LUN_STATUS_FREE;
+ tpg->tpg_lun_list[lun->unpacked_lun] = NULL;
spin_unlock(&tpg->tpg_lun_lock);
+
+ kfree(lun);
}
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index f3761da..e7b3d57 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -125,12 +125,6 @@ enum hba_flags_table {
HBA_FLAGS_PSCSI_MODE = 0x02,
};

-/* struct se_lun->lun_status */
-enum transport_lun_status_table {
- TRANSPORT_LUN_STATUS_FREE = 0,
- TRANSPORT_LUN_STATUS_ACTIVE = 1,
-};
-
/* struct se_portal_group->se_tpg_type */
enum transport_tpg_type_table {
TRANSPORT_TPG_TYPE_NORMAL = 0,
@@ -708,8 +702,6 @@ struct se_port_stat_grps {
struct se_lun {
#define SE_LUN_LINK_MAGIC 0xffff7771
u32 lun_link_magic;
- /* See transport_lun_status_table */
- enum transport_lun_status_table lun_status;
u32 lun_access;
u32 lun_flags;
u32 unpacked_lun;
@@ -878,7 +870,7 @@ struct se_portal_group {
struct list_head se_tpg_node;
/* linked list for initiator ACL list */
struct list_head acl_node_list;
- struct se_lun **tpg_lun_list;
+ struct se_lun *tpg_lun_list[TRANSPORT_MAX_LUNS_PER_TPG];
struct se_lun tpg_virt_lun0;
/* List of TCM sessions associated wth this TPG */
struct list_head tpg_sess_list;

[Andy Grover]

Create helper functions to alloc a deve and to transition it from
demo mode to explicit.

Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_device.c | 121 ++++++++++++++++++++++--------------
1 file changed, 74 insertions(+), 47 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 4b8149b..2c589a5 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -288,6 +288,73 @@ void core_update_device_list_access(
spin_unlock_irq(&nacl->device_list_lock);
}

+static struct se_dev_entry *core_alloc_se_dev_entry(
+ struct se_lun *lun,
+ struct se_lun_acl *lun_acl,
+ u32 mapped_lun,
+ u32 lun_access,
+ struct se_node_acl *nacl)
+{
+ struct se_dev_entry *deve;
+
+ /* Holding locks, can't sleep */
+ deve = kzalloc(sizeof(*deve), GFP_ATOMIC);
+ if (!deve)
+ return ERR_PTR(-ENOMEM);
+
+ atomic_set(&deve->ua_count, 0);
+ atomic_set(&deve->pr_ref_count, 0);
+ spin_lock_init(&deve->ua_lock);
+ INIT_LIST_HEAD(&deve->alua_port_list);
+ INIT_LIST_HEAD(&deve->ua_list);
+ deve->se_lun = lun;
+ deve->se_lun_acl = lun_acl;
+ deve->mapped_lun = mapped_lun;
+
+ if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE)
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
+ else
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
+
+ deve->creation_time = get_jiffies_64();
+
+ nacl->device_list[mapped_lun] = deve;
+
+ return deve;
+}
+
+static int core_transition_deve_to_explicit(
+ struct se_lun *lun,
+ struct se_lun_acl *lun_acl,
+ struct se_dev_entry *deve,
+ u32 lun_access)
+{
+ if (deve->se_lun_acl != NULL) {
+ pr_err("struct se_dev_entry->se_lun_acl"
+ " already set for demo mode -> explicit"
+ " LUN ACL transition\n");
+ return -EINVAL;
+ }
+ if (deve->se_lun != lun) {
+ pr_err("struct se_dev_entry->se_lun does"
+ " match passed struct se_lun for demo mode"
+ " -> explicit LUN ACL transition\n");
+ return -EINVAL;
+ }
+
+ deve->se_lun_acl = lun_acl;
+
+ if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
+ deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
+ } else {
+ deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
+ }
+
+ return 0;
+}
+
/* core_enable_device_list_for_node():
*
*/
@@ -316,62 +383,22 @@ int core_enable_device_list_for_node(
* + mapped_lun that was setup in demo mode..
*/
if (deve) {
- if (deve->se_lun_acl != NULL) {
- pr_err("struct se_dev_entry->se_lun_acl"
- " already set for demo mode -> explicit"
- " LUN ACL transition\n");
- ret = -EINVAL;
- goto out;
- }
- if (deve->se_lun != lun) {
- pr_err("struct se_dev_entry->se_lun does"
- " match passed struct se_lun for demo mode"
- " -> explicit LUN ACL transition\n");
- ret = -EINVAL;
- goto out;
- }
- deve->se_lun_acl = lun_acl;
-
- if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
- } else {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
- }
-
+ ret = core_transition_deve_to_explicit(lun, lun_acl, deve,
+ lun_access);
goto out;
}

- deve = kzalloc(sizeof(*deve), GFP_ATOMIC);
- if (!deve) {
- spin_unlock_irq(&nacl->device_list_lock);
- return -ENOMEM;
+ deve = core_alloc_se_dev_entry(lun, lun_acl, mapped_lun, lun_access, nacl);
+ if (IS_ERR(deve)) {
+ ret = PTR_ERR(deve);
+ goto out;
}

- nacl->device_list[mapped_lun] = deve;
-
- atomic_set(&deve->ua_count, 0);
- atomic_set(&deve->pr_ref_count, 0);
- spin_lock_init(&deve->ua_lock);
- INIT_LIST_HEAD(&deve->alua_port_list);
- INIT_LIST_HEAD(&deve->ua_list);
- deve->se_lun = lun;
- deve->se_lun_acl = lun_acl;
- deve->mapped_lun = mapped_lun;
-
- if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE)
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
- else
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
-
- deve->creation_time = get_jiffies_64();
- deve->attach_count++;
-
spin_lock(&port->sep_alua_lock);
list_add_tail(&deve->alua_port_list, &port->sep_alua_list);
spin_unlock(&port->sep_alua_lock);

+ deve->attach_count++;
out:
spin_unlock(&nacl->device_list_lock);

[Andy Grover]

Remove core_tpg_pre_dellun entirely, since we don't need to get/check
a pointer we already have.

Nothing else can return an error, so core_dev_del_lun can return void.

Rename core_tpg_post_dellun to remove_lun - a clearer name, now that
pre_dellun is gone.

Reviewed-by: Christoph Hellwig <***@lst.de>
Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_device.c | 18 ++++----------
drivers/target/target_core_fabric_configfs.c | 2 +-
drivers/target/target_core_internal.h | 5 ++--
drivers/target/target_core_tpg.c | 36 +++-------------------------
4 files changed, 11 insertions(+), 50 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 0bac890..bd467a4 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -1264,24 +1264,16 @@ struct se_lun *core_dev_add_lun(
*
*
*/
-int core_dev_del_lun(
+void core_dev_del_lun(
struct se_portal_group *tpg,
- u32 unpacked_lun)
+ struct se_lun *lun)
{
- struct se_lun *lun;
-
- lun = core_tpg_pre_dellun(tpg, unpacked_lun);
- if (IS_ERR(lun))
- return PTR_ERR(lun);
-
- core_tpg_post_dellun(tpg, lun);
-
- pr_debug("%s_TPG[%u]_LUN[%u] - Deactivated %s Logical Unit from"
+ pr_debug("%s_TPG[%u]_LUN[%u] - Deactivating %s Logical Unit from"
" device object\n", tpg->se_tpg_tfo->get_fabric_name(),
- tpg->se_tpg_tfo->tpg_get_tag(tpg), unpacked_lun,
+ tpg->se_tpg_tfo->tpg_get_tag(tpg), lun->unpacked_lun,
tpg->se_tpg_tfo->get_fabric_name());

- return 0;
+ core_tpg_remove_lun(tpg, lun);
}

struct se_lun *core_get_lun_from_tpg(struct se_portal_group *tpg, u32 unpacked_lun)
diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 0955c948..811b2f4 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -827,7 +827,7 @@ static int target_fabric_port_unlink(
tf->tf_ops.fabric_pre_unlink(se_tpg, lun);
}

- core_dev_del_lun(se_tpg, lun->unpacked_lun);
+ core_dev_del_lun(se_tpg, lun);
return 0;
}

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 463fddc..42ef4ab 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -46,7 +46,7 @@ int se_dev_set_fabric_max_sectors(struct se_device *, u32);
int se_dev_set_optimal_sectors(struct se_device *, u32);
int se_dev_set_block_size(struct se_device *, u32);
struct se_lun *core_dev_add_lun(struct se_portal_group *, struct se_device *, u32);
-int core_dev_del_lun(struct se_portal_group *, u32);
+void core_dev_del_lun(struct se_portal_group *, struct se_lun *);
struct se_lun *core_get_lun_from_tpg(struct se_portal_group *, u32);
struct se_lun_acl *core_dev_init_initiator_node_lun_acl(struct se_portal_group *,
struct se_node_acl *, u32, int *);
@@ -82,8 +82,7 @@ void core_tpg_wait_for_nacl_pr_ref(struct se_node_acl *);
struct se_lun *core_tpg_alloc_lun(struct se_portal_group *, u32);
int core_tpg_add_lun(struct se_portal_group *, struct se_lun *,
u32, struct se_device *);
-struct se_lun *core_tpg_pre_dellun(struct se_portal_group *, u32 unpacked_lun);
-void core_tpg_post_dellun(struct se_portal_group *, struct se_lun *);
+void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);

/* target_core_transport.c */
extern struct kmem_cache *se_tmr_req_cache;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 321268d..5d2c774 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -298,7 +298,7 @@ void core_tpg_clear_object_luns(struct se_portal_group *tpg)
continue;

spin_unlock(&tpg->tpg_lun_lock);
- core_dev_del_lun(tpg, lun->unpacked_lun);
+ core_dev_del_lun(tpg, lun);
spin_lock(&tpg->tpg_lun_lock);
}
spin_unlock(&tpg->tpg_lun_lock);
@@ -625,7 +625,7 @@ static void core_tpg_release_virtual_lun0(struct se_portal_group *se_tpg)
{
struct se_lun *lun = &se_tpg->tpg_virt_lun0;

- core_tpg_post_dellun(se_tpg, lun);
+ core_tpg_remove_lun(se_tpg, lun);
}

int core_tpg_register(
@@ -795,37 +795,7 @@ int core_tpg_add_lun(
return 0;
}

-struct se_lun *core_tpg_pre_dellun(
- struct se_portal_group *tpg,
- u32 unpacked_lun)
-{
- struct se_lun *lun;
-
- if (unpacked_lun > (TRANSPORT_MAX_LUNS_PER_TPG-1)) {
- pr_err("%s LUN: %u exceeds TRANSPORT_MAX_LUNS_PER_TPG"
- "-1: %u for Target Portal Group: %u\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- TRANSPORT_MAX_LUNS_PER_TPG-1,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- return ERR_PTR(-EOVERFLOW);
- }
-
- spin_lock(&tpg->tpg_lun_lock);
- lun = tpg->tpg_lun_list[unpacked_lun];
- if (lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE) {
- pr_err("%s Logical Unit Number: %u is not active on"
- " Target Portal Group: %u, ignoring request.\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- spin_unlock(&tpg->tpg_lun_lock);
- return ERR_PTR(-ENODEV);
- }
- spin_unlock(&tpg->tpg_lun_lock);
-
- return lun;
-}
-
-void core_tpg_post_dellun(
+void core_tpg_remove_lun(
struct se_portal_group *tpg,
struct se_lun *lun)
{

[Andy Grover]

Remove TRANSPORT_LUNFLAGS_INITIATOR_ACCESS and just look at if a
non-NULL value is in the node_acl->device_list for the given lun. Since
usually nowhere near all se_dev_entrys are used, this nets a sizable
reduction in memory use.

In core_disable_device_list_for_node, move all calls to functions
referencing deve inside the spinlock, since it's not safe to access deve
outside it. Skip reinitializing, since the deve is actually being freed.

Do not allocate device_list array separately, but include it in
se_node_acl.

Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_device.c | 93 ++++++++++++++++------------
drivers/target/target_core_fabric_configfs.c | 4 +-
drivers/target/target_core_spc.c | 2 +-
drivers/target/target_core_tpg.c | 41 +-----------
include/target/target_core_base.h | 7 +--
5 files changed, 59 insertions(+), 88 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 5d4a8b3..0bac890 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -67,7 +67,7 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)

spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags);
se_cmd->se_deve = se_sess->se_node_acl->device_list[unpacked_lun];
- if (se_cmd->se_deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
+ if (se_cmd->se_deve) {
struct se_dev_entry *deve = se_cmd->se_deve;

deve->total_cmds++;
@@ -143,7 +143,6 @@ EXPORT_SYMBOL(transport_lookup_cmd_lun);

int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
{
- struct se_dev_entry *deve;
struct se_lun *se_lun = NULL;
struct se_session *se_sess = se_cmd->se_sess;
struct se_tmr_req *se_tmr = se_cmd->se_tmr_req;
@@ -154,9 +153,9 @@ int transport_lookup_tmr_lun(struct se_cmd *se_cmd, u32 unpacked_lun)

spin_lock_irqsave(&se_sess->se_node_acl->device_list_lock, flags);
se_cmd->se_deve = se_sess->se_node_acl->device_list[unpacked_lun];
- deve = se_cmd->se_deve;
+ if (se_cmd->se_deve) {
+ struct se_dev_entry *deve = se_cmd->se_deve;

- if (deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
se_tmr->tmr_lun = deve->se_lun;
se_cmd->se_lun = deve->se_lun;
se_lun = deve->se_lun;
@@ -204,7 +203,7 @@ struct se_dev_entry *core_get_se_deve_from_rtpi(
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
deve = nacl->device_list[i];

- if (!(deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS))
+ if (!deve)
continue;

lun = deve->se_lun;
@@ -243,14 +242,11 @@ int core_free_device_list_for_node(
struct se_lun *lun;
u32 i;

- if (!nacl->device_list)
- return 0;
-
spin_lock_irq(&nacl->device_list_lock);
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
deve = nacl->device_list[i];

- if (!(deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS))
+ if (!deve)
continue;

if (!deve->se_lun) {
@@ -268,9 +264,6 @@ int core_free_device_list_for_node(
}
spin_unlock_irq(&nacl->device_list_lock);

- array_free(nacl->device_list, TRANSPORT_MAX_LUNS_PER_TPG);
- nacl->device_list = NULL;
-
return 0;
}

@@ -283,12 +276,14 @@ void core_update_device_list_access(

spin_lock_irq(&nacl->device_list_lock);
deve = nacl->device_list[mapped_lun];
- if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
- } else {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
- deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
+ if (deve) {
+ if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
+ deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
+ } else {
+ deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
+ deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
+ }
}
spin_unlock_irq(&nacl->device_list_lock);
}
@@ -320,7 +315,7 @@ int core_enable_device_list_for_node(
* transition. This transition must be for the same struct se_lun
* + mapped_lun that was setup in demo mode..
*/
- if (deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS) {
+ if (deve) {
if (deve->se_lun_acl != NULL) {
pr_err("struct se_dev_entry->se_lun_acl"
" already set for demo mode -> explicit"
@@ -348,18 +343,27 @@ int core_enable_device_list_for_node(
goto out;
}

+ deve = kzalloc(sizeof(*deve), GFP_ATOMIC);
+ if (!deve) {
+ spin_unlock_irq(&nacl->device_list_lock);
+ return -ENOMEM;
+ }
+
+ nacl->device_list[mapped_lun] = deve;
+
+ atomic_set(&deve->ua_count, 0);
+ atomic_set(&deve->pr_ref_count, 0);
+ spin_lock_init(&deve->ua_lock);
+ INIT_LIST_HEAD(&deve->alua_port_list);
+ INIT_LIST_HEAD(&deve->ua_list);
deve->se_lun = lun;
deve->se_lun_acl = lun_acl;
deve->mapped_lun = mapped_lun;
- deve->lun_flags |= TRANSPORT_LUNFLAGS_INITIATOR_ACCESS;

- if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_ONLY;
+ if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE)
deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE;
- } else {
- deve->lun_flags &= ~TRANSPORT_LUNFLAGS_READ_WRITE;
+ else
deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
- }

deve->creation_time = get_jiffies_64();
deve->attach_count++;
@@ -387,7 +391,16 @@ int core_disable_device_list_for_node(
struct se_portal_group *tpg)
{
struct se_port *port = lun->lun_sep;
- struct se_dev_entry *deve = nacl->device_list[mapped_lun];
+ struct se_dev_entry *deve;
+
+ core_scsi3_free_pr_reg_from_nacl(lun->lun_se_dev, nacl);
+
+ spin_lock_irq(&nacl->device_list_lock);
+ deve = nacl->device_list[mapped_lun];
+ if (!deve) {
+ spin_unlock_irq(&nacl->device_list_lock);
+ return 0;
+ }

/*
* If the MappedLUN entry is being disabled, the entry in
@@ -402,9 +415,18 @@ int core_disable_device_list_for_node(
* NodeACL context specific PR metadata for demo-mode
* MappedLUN *deve will be released below..
*/
- spin_lock_bh(&port->sep_alua_lock);
+ spin_lock(&port->sep_alua_lock);
list_del(&deve->alua_port_list);
- spin_unlock_bh(&port->sep_alua_lock);
+ spin_unlock(&port->sep_alua_lock);
+
+ /*
+ * Disable struct se_dev_entry LUN ACL mapping
+ */
+ core_scsi3_ua_release_all(deve);
+ nacl->device_list[mapped_lun] = NULL;
+
+ spin_unlock_irq(&nacl->device_list_lock);
+
/*
* Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE
* PR operation to complete.
@@ -412,19 +434,8 @@ int core_disable_device_list_for_node(
while (atomic_read(&deve->pr_ref_count) != 0)
cpu_relax();

- spin_lock_irq(&nacl->device_list_lock);
- /*
- * Disable struct se_dev_entry LUN ACL mapping
- */
- core_scsi3_ua_release_all(deve);
- deve->se_lun = NULL;
- deve->se_lun_acl = NULL;
- deve->lun_flags = 0;
- deve->creation_time = 0;
- deve->attach_count--;
- spin_unlock_irq(&nacl->device_list_lock);
+ kfree(deve);

- core_scsi3_free_pr_reg_from_nacl(lun->lun_se_dev, nacl);
return 0;
}

@@ -445,7 +456,7 @@ void core_clear_lun_from_tpg(struct se_lun *lun, struct se_portal_group *tpg)
spin_lock_irq(&nacl->device_list_lock);
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
deve = nacl->device_list[i];
- if (lun != deve->se_lun)
+ if (!deve || lun != deve->se_lun)
continue;
spin_unlock_irq(&nacl->device_list_lock);

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index 804e7f0..0955c948 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -113,7 +113,7 @@ static int target_fabric_mappedlun_link(
*/
spin_lock_irq(&lacl->se_lun_nacl->device_list_lock);
deve = lacl->se_lun_nacl->device_list[lacl->mapped_lun];
- if (deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS)
+ if (deve)
lun_access = deve->lun_flags;
else
lun_access =
@@ -149,7 +149,7 @@ static int target_fabric_mappedlun_unlink(
/*
* Determine if the underlying MappedLUN has already been released..
*/
- if (!deve->se_lun) {
+ if (!deve || !deve->se_lun) {
spin_unlock_irq(&nacl->device_list_lock);
return 0;
}
diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index 6cd7222..cb47ffe 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1250,7 +1250,7 @@ sense_reason_t spc_emulate_report_luns(struct se_cmd *cmd)
spin_lock_irq(&sess->se_node_acl->device_list_lock);
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
deve = sess->se_node_acl->device_list[i];
- if (!(deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS))
+ if (!deve)
continue;
/*
* We determine the correct LUN LIST LENGTH even once we
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 88fddcf..82a25e1 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -62,7 +62,7 @@ static void core_clear_initiator_node_from_tpg(
for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
deve = nacl->device_list[i];

- if (!(deve->lun_flags & TRANSPORT_LUNFLAGS_INITIATOR_ACCESS))
+ if (!deve)
continue;

if (!deve->se_lun) {
@@ -214,35 +214,6 @@ static void *array_zalloc(int n, size_t size, gfp_t flags)
return a;
}

-/* core_create_device_list_for_node():
- *
- *
- */
-static int core_create_device_list_for_node(struct se_node_acl *nacl)
-{
- struct se_dev_entry *deve;
- int i;
-
- nacl->device_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG,
- sizeof(struct se_dev_entry), GFP_KERNEL);
- if (!nacl->device_list) {
- pr_err("Unable to allocate memory for"
- " struct se_node_acl->device_list\n");
- return -ENOMEM;
- }
- for (i = 0; i < TRANSPORT_MAX_LUNS_PER_TPG; i++) {
- deve = nacl->device_list[i];
-
- atomic_set(&deve->ua_count, 0);
- atomic_set(&deve->pr_ref_count, 0);
- spin_lock_init(&deve->ua_lock);
- INIT_LIST_HEAD(&deve->alua_port_list);
- INIT_LIST_HEAD(&deve->ua_list);
- }
-
- return 0;
-}
-
/* core_tpg_check_initiator_node_acl()
*
*
@@ -279,11 +250,6 @@ struct se_node_acl *core_tpg_check_initiator_node_acl(

tpg->se_tpg_tfo->set_default_node_attributes(acl);

- if (core_create_device_list_for_node(acl) < 0) {
- tpg->se_tpg_tfo->tpg_release_fabric_acl(tpg, acl);
- return NULL;
- }
-
if (core_set_queue_depth_for_node(tpg, acl) < 0) {
core_free_device_list_for_node(acl, tpg);
tpg->se_tpg_tfo->tpg_release_fabric_acl(tpg, acl);
@@ -405,11 +371,6 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(

tpg->se_tpg_tfo->set_default_node_attributes(acl);

- if (core_create_device_list_for_node(acl) < 0) {
- tpg->se_tpg_tfo->tpg_release_fabric_acl(tpg, acl);
- return ERR_PTR(-ENOMEM);
- }
-
if (core_set_queue_depth_for_node(tpg, acl) < 0) {
core_free_device_list_for_node(acl, tpg);
tpg->se_tpg_tfo->tpg_release_fabric_acl(tpg, acl);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 9ec9864..f3761da 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -174,9 +174,8 @@ enum se_cmd_flags_table {
/* struct se_dev_entry->lun_flags and struct se_lun->lun_access */
enum transport_lunflags_table {
TRANSPORT_LUNFLAGS_NO_ACCESS = 0x00,
- TRANSPORT_LUNFLAGS_INITIATOR_ACCESS = 0x01,
- TRANSPORT_LUNFLAGS_READ_ONLY = 0x02,
- TRANSPORT_LUNFLAGS_READ_WRITE = 0x04,
+ TRANSPORT_LUNFLAGS_READ_ONLY = 0x01,
+ TRANSPORT_LUNFLAGS_READ_WRITE = 0x02,
};

/*
@@ -589,7 +588,7 @@ struct se_node_acl {
char acl_tag[MAX_ACL_TAG_SIZE];
/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
atomic_t acl_pr_ref_count;
- struct se_dev_entry **device_list;
+ struct se_dev_entry *device_list[TRANSPORT_MAX_LUNS_PER_TPG];
struct se_session *nacl_sess;
struct se_portal_group *se_tpg;
spinlock_t device_list_lock;

[Andy Grover]

Nothing in it can raise an error.

Reviewed-by: Christoph Hellwig <***@lst.de>
Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_internal.h | 2 +-
drivers/target/target_core_tpg.c | 5 +----
2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index de9cab7..463fddc 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -83,7 +83,7 @@ struct se_lun *core_tpg_alloc_lun(struct se_portal_group *, u32);
int core_tpg_add_lun(struct se_portal_group *, struct se_lun *,
u32, struct se_device *);
struct se_lun *core_tpg_pre_dellun(struct se_portal_group *, u32 unpacked_lun);
-int core_tpg_post_dellun(struct se_portal_group *, struct se_lun *);
+void core_tpg_post_dellun(struct se_portal_group *, struct se_lun *);

/* target_core_transport.c */
extern struct kmem_cache *se_tmr_req_cache;
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index 82a25e1..321268d 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -825,18 +825,15 @@ struct se_lun *core_tpg_pre_dellun(
return lun;
}

-int core_tpg_post_dellun(
+void core_tpg_post_dellun(
struct se_portal_group *tpg,
struct se_lun *lun)
{
core_clear_lun_from_tpg(lun, tpg);
transport_clear_lun_ref(lun);
-
core_dev_unexport(lun->lun_se_dev, tpg, lun);

spin_lock(&tpg->tpg_lun_lock);
lun->lun_status = TRANSPORT_LUN_STATUS_FREE;
spin_unlock(&tpg->tpg_lun_lock);
-
- return 0;
}

[Andy Grover]

It's not safe to unlock/relock in core_tpg_add_node_to_devs. Remove this.

As a consequence, core_enable_device_list_for_node needs to be called with
a lock held & irqs off. Add a comment mentioning this. Change
spin_lock_irqs to spin_locks. Change error handling to goto-style.

Also change the other place enable_device_list_for_node is called,
add_initiator_node_lun_acl. Hold tpg_lun_lock across all uses of lun.
Change error handling to release lock from error paths. Remove
core_dev_get_lun.

Signed-off-by: Andy Grover <***@redhat.com>
---
drivers/target/target_core_device.c | 81 ++++++++++++++-----------------------
drivers/target/target_core_tpg.c | 3 --
2 files changed, 31 insertions(+), 53 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 10130ea..5d4a8b3 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -295,7 +295,6 @@ void core_update_device_list_access(

/* core_enable_device_list_for_node():
*
- *
*/
int core_enable_device_list_for_node(
struct se_lun *lun,
@@ -307,8 +306,12 @@ int core_enable_device_list_for_node(
{
struct se_port *port = lun->lun_sep;
struct se_dev_entry *deve;
+ int ret = 0;

- spin_lock_irq(&nacl->device_list_lock);
+ assert_spin_locked(&tpg->tpg_lun_lock);
+ WARN_ON_ONCE(!irqs_disabled());
+
+ spin_lock(&nacl->device_list_lock);

deve = nacl->device_list[mapped_lun];

@@ -322,15 +325,15 @@ int core_enable_device_list_for_node(
pr_err("struct se_dev_entry->se_lun_acl"
" already set for demo mode -> explicit"
" LUN ACL transition\n");
- spin_unlock_irq(&nacl->device_list_lock);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
if (deve->se_lun != lun) {
pr_err("struct se_dev_entry->se_lun does"
" match passed struct se_lun for demo mode"
" -> explicit LUN ACL transition\n");
- spin_unlock_irq(&nacl->device_list_lock);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
deve->se_lun_acl = lun_acl;

@@ -342,8 +345,7 @@ int core_enable_device_list_for_node(
deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY;
}

- spin_unlock_irq(&nacl->device_list_lock);
- return 0;
+ goto out;
}

deve->se_lun = lun;
@@ -366,9 +368,10 @@ int core_enable_device_list_for_node(
list_add_tail(&deve->alua_port_list, &port->sep_alua_list);
spin_unlock(&port->sep_alua_lock);

- spin_unlock_irq(&nacl->device_list_lock);
+out:
+ spin_unlock(&nacl->device_list_lock);

- return 0;
+ return ret;
}

/* core_disable_device_list_for_node():
@@ -1299,39 +1302,6 @@ struct se_lun *core_get_lun_from_tpg(struct se_portal_group *tpg, u32 unpacked_l
return lun;
}

-/* core_dev_get_lun():
- *
- *
- */
-static struct se_lun *core_dev_get_lun(struct se_portal_group *tpg, u32 unpacked_lun)
-{
- struct se_lun *lun;
-
- spin_lock(&tpg->tpg_lun_lock);
- if (unpacked_lun > (TRANSPORT_MAX_LUNS_PER_TPG-1)) {
- pr_err("%s LUN: %u exceeds TRANSPORT_MAX_LUNS_PER"
- "_TPG-1: %u for Target Portal Group: %hu\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- TRANSPORT_MAX_LUNS_PER_TPG-1,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- spin_unlock(&tpg->tpg_lun_lock);
- return NULL;
- }
- lun = tpg->tpg_lun_list[unpacked_lun];
-
- if (lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE) {
- pr_err("%s Logical Unit Number: %u is not active on"
- " Target Portal Group: %hu, ignoring request.\n",
- tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
- tpg->se_tpg_tfo->tpg_get_tag(tpg));
- spin_unlock(&tpg->tpg_lun_lock);
- return NULL;
- }
- spin_unlock(&tpg->tpg_lun_lock);
-
- return lun;
-}
-
struct se_lun_acl *core_dev_init_initiator_node_lun_acl(
struct se_portal_group *tpg,
struct se_node_acl *nacl,
@@ -1370,19 +1340,24 @@ int core_dev_add_initiator_node_lun_acl(
{
struct se_lun *lun;
struct se_node_acl *nacl;
+ int ret = 0;

- lun = core_dev_get_lun(tpg, unpacked_lun);
+ spin_lock_irq(&tpg->tpg_lun_lock);
+ lun = tpg->tpg_lun_list[unpacked_lun];
if (!lun) {
pr_err("%s Logical Unit Number: %u is not active on"
" Target Portal Group: %hu, ignoring request.\n",
tpg->se_tpg_tfo->get_fabric_name(), unpacked_lun,
tpg->se_tpg_tfo->tpg_get_tag(tpg));
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}

nacl = lacl->se_lun_nacl;
- if (!nacl)
- return -EINVAL;
+ if (!nacl) {
+ ret = -EINVAL;
+ goto out;
+ }

if ((lun->lun_access & TRANSPORT_LUNFLAGS_READ_ONLY) &&
(lun_access & TRANSPORT_LUNFLAGS_READ_WRITE))
@@ -1391,8 +1366,10 @@ int core_dev_add_initiator_node_lun_acl(
lacl->se_lun = lun;

if (core_enable_device_list_for_node(lun, lacl, lacl->mapped_lun,
- lun_access, nacl, tpg) < 0)
- return -EINVAL;
+ lun_access, nacl, tpg) < 0) {
+ ret = -EINVAL;
+ goto out;
+ }

spin_lock(&lun->lun_acl_lock);
list_add_tail(&lacl->lacl_list, &lun->lun_acl_list);
@@ -1410,7 +1387,11 @@ int core_dev_add_initiator_node_lun_acl(
* pre-registrations that need to be enabled for this LUN ACL..
*/
core_scsi3_check_aptpl_registration(lun->lun_se_dev, tpg, lun, lacl);
- return 0;
+
+out:
+ spin_unlock_irq(&tpg->tpg_lun_lock);
+
+ return ret;
}

/* core_dev_del_initiator_node_lun_acl():
diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index c036595..88fddcf 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -137,8 +137,6 @@ void core_tpg_add_node_to_devs(
if (lun->lun_status != TRANSPORT_LUN_STATUS_ACTIVE)
continue;

- spin_unlock(&tpg->tpg_lun_lock);
-
dev = lun->lun_se_dev;
/*
* By default in LIO-Target $FABRIC_MOD,
@@ -166,7 +164,6 @@ void core_tpg_add_node_to_devs(

core_enable_device_list_for_node(lun, NULL, lun->unpacked_lun,
lun_access, acl, tpg);
- spin_lock(&tpg->tpg_lun_lock);
}
spin_unlock(&tpg->tpg_lun_lock);
}

[Christoph Hellwig]

Nic,

any progress on looking over these? Seems like there's actually
nothing at all queued up for 3.17 in the target tree, or am =D0=86 miss=
ing
something?

[Andy Grover]

Post by Christoph Hellwig
Nic,
any progress on looking over these? Seems like there's actually
nothing at all queued up for 3.17 in the target tree, or am =D0=86 mi=

ssing

Post by Christoph Hellwig
something?

ping?

[Christoph Hellwig]

Post by Christoph Hellwig
Nic,
=20
any progress on looking over these? Seems like there's actually
nothing at all queued up for 3.17 in the target tree, or am =D0=86 mi=

ssing

Post by Christoph Hellwig
something?

ping again. We're getting closer to the end of the 3.18 merge window
and there still hasn't been a response. Should Andy just send the patc=
hes
directly to Linus once 3.18 opens given that they have been out on the =
list
since Jun 23? (with a positive review from me and no negative one)

[Nicholas A. Bellinger]

Post by Christoph Hellwig

Post by Christoph Hellwig
Nic,
=20
any progress on looking over these? Seems like there's actually
nothing at all queued up for 3.17 in the target tree, or am =D0=86 =

missing

Post by Christoph Hellwig

Post by Christoph Hellwig
something?

=20
ping again. We're getting closer to the end of the 3.18 merge window
and there still hasn't been a response. Should Andy just send the pa=

tches

Post by Christoph Hellwig
directly to Linus once 3.18 opens given that they have been out on th=

e list

Post by Christoph Hellwig
since Jun 23? (with a positive review from me and no negative one)
=20

Removing unused per WWPN endpoint LUN + per NodeACL MappedLUN memory is
a nice optimization to have, but I'm not yet convinced that extending
existing control path spinlocks to support an array of pointers is
ultimately worth the complexity it adds here.

Another concern is how these changes effect active session + device I/O
shutdown, which is an area of regressions I'd rather avoid if the
primary benefit of this series is only reducing memory footprint for
unused LUNs + MappedLUNs. Lowering the TRANSPORT_MAX_LUNS_PER_TPG valu=
e
at compile time today is the simple way for reducing overall memory
footprint for folks who need to scale up the number of targets using
smaller individual LUN mappings.

As for something smarter, given the mostly read-only nature of LUN +
MappedLUN pointers to individual TPGT + NodeACLs context, I'd rather se=
e
something based on RCU arrays + percpu_ref counting to avoid this type
of complexity to existing code, and move in the direction of dropping
fast-path I_T ->device_list_lock access all together.

Beyond these objections, there are some useful fixes + cleanups from
this series that I'm OK with merging soon..

--nab

[Andy Grover]

Post by Nicholas A. Bellinger

Post by Christoph Hellwig
ping again. We're getting closer to the end of the 3.18 merge window
and there still hasn't been a response. Should Andy just send the patches
directly to Linus once 3.18 opens given that they have been out on the list
since Jun 23? (with a positive review from me and no negative one)

Removing unused per WWPN endpoint LUN + per NodeACL MappedLUN memory is
a nice optimization to have, but I'm not yet convinced that extending
existing control path spinlocks to support an array of pointers is
ultimately worth the complexity it adds here.

9 files changed, 250 insertions(+), 367 deletions(-).

This patchset removes 100+ lines of code. Furthermore, I wouldn't
characterize it as extending locks, so much as putting locks where they
should've always been. The fact that device_list[foo] is never null
means we've avoided crashes but not potentially incorrect accesses.

Post by Nicholas A. Bellinger
Another concern is how these changes effect active session + device I/O
shutdown, which is an area of regressions I'd rather avoid

I assume this set would spend time in your tree, followed by Linus' tree
before making it into a release. Also, any logic errors are likely to
result in a fault, so they should not remain hidden for long.

Post by Nicholas A. Bellinger
if the
primary benefit of this series is only reducing memory footprint for
unused LUNs + MappedLUNs.

Yes it does reduce wasted memory, that should be reason enough I'd say.
But this patchset is also a building block for further improvements that
are more significant. This set transitions all lun and mappedlun checks
from checking a flag to checking for NULL. This is necessary before we
can improve from a fixed-size array to more size-scalable data
structures like a radix tree, or lockless, with RCU.

Post by Nicholas A. Bellinger
Lowering the TRANSPORT_MAX_LUNS_PER_TPG value
at compile time today is the simple way for reducing overall memory
footprint for folks who need to scale up the number of targets using
smaller individual LUN mappings.

This is only an option for embedded. We should scale the amount of
memory we use with the number of allocated LUNs and mapped LUNs.

Post by Nicholas A. Bellinger
As for something smarter, given the mostly read-only nature of LUN +
MappedLUN pointers to individual TPGT + NodeACLs context, I'd rather see
something based on RCU arrays + percpu_ref counting to avoid this type
of complexity to existing code, and move in the direction of dropping
fast-path I_T ->device_list_lock access all together.

See above about pointers vs flags, this is a first step toward more
performant *and* space-efficient data structures.

Post by Nicholas A. Bellinger
Beyond these objections, there are some useful fixes + cleanups from
this series that I'm OK with merging soon..

I've pushed this patchset to

git://git.kernel.org/pub/scm/linux/kernel/git/grover/linux.git

on two branches against your and Linus' repos:
against-linus
against-target-pending-for-next

(looked-over and compile-tested)

For your convenience.

Regards -- Andy

[Nicholas A. Bellinger]

Post by Andy Grover

Post by Nicholas A. Bellinger

Post by Christoph Hellwig
ping again. We're getting closer to the end of the 3.18 merge window
and there still hasn't been a response. Should Andy just send the patches
directly to Linus once 3.18 opens given that they have been out on the list
since Jun 23? (with a positive review from me and no negative one)

Removing unused per WWPN endpoint LUN + per NodeACL MappedLUN memory is
a nice optimization to have, but I'm not yet convinced that extending
existing control path spinlocks to support an array of pointers is
ultimately worth the complexity it adds here.

9 files changed, 250 insertions(+), 367 deletions(-).
This patchset removes 100+ lines of code. Furthermore, I wouldn't
characterize it as extending locks, so much as putting locks where they
should've always been. The fact that device_list[foo] is never null
means we've avoided crashes but not potentially incorrect accesses.

It's the extending of locks over other locks that make aspects of this
series problematic to existing code. Please run them with lock
debugging on and perform a few active I/O shutdowns and you'll see what
I mean.

Not to mention the patches with the locking changes mix cleanups with
bug-fixes, which I really dislike when reviewing patches of this nature.

Post by Andy Grover

Post by Nicholas A. Bellinger
Another concern is how these changes effect active session + device I/O
shutdown, which is an area of regressions I'd rather avoid

I assume this set would spend time in your tree, followed by Linus' tree
before making it into a release. Also, any logic errors are likely to
result in a fault, so they should not remain hidden for long.

Post by Nicholas A. Bellinger
if the
primary benefit of this series is only reducing memory footprint for
unused LUNs + MappedLUNs.

Yes it does reduce wasted memory, that should be reason enough I'd say.
But this patchset is also a building block for further improvements that
are more significant. This set transitions all lun and mappedlun checks
from checking a flag to checking for NULL. This is necessary before we
can improve from a fixed-size array to more size-scalable data
structures like a radix tree, or lockless, with RCU.

Like I said, I don't agree this is the best way to get to something
smarter.

Post by Andy Grover

Post by Nicholas A. Bellinger
Lowering the TRANSPORT_MAX_LUNS_PER_TPG value
at compile time today is the simple way for reducing overall memory
footprint for folks who need to scale up the number of targets using
smaller individual LUN mappings.

This is only an option for embedded. We should scale the amount of
memory we use with the number of allocated LUNs and mapped LUNs.

Post by Nicholas A. Bellinger
As for something smarter, given the mostly read-only nature of LUN +
MappedLUN pointers to individual TPGT + NodeACLs context, I'd rather see
something based on RCU arrays + percpu_ref counting to avoid this type
of complexity to existing code, and move in the direction of dropping
fast-path I_T ->device_list_lock access all together.

See above about pointers vs flags, this is a first step toward more
performant *and* space-efficient data structures.

Post by Nicholas A. Bellinger
Beyond these objections, there are some useful fixes + cleanups from
this series that I'm OK with merging soon..

I've pushed this patchset to
git://git.kernel.org/pub/scm/linux/kernel/git/grover/linux.git
against-linus
against-target-pending-for-next
(looked-over and compile-tested)

I'm currently reviewing #2, #4, #5 and #7 and will consider merging
these. These cleanups account for most of the LOC reduction, and avoid
most of the larger concerns.

Also for patches like this, they really need testing on your end before
I'll consider them merging. Compile testing alone for these types of
locking changes is not enough, given the history of regressions with
these types of cleanups.

Thanks,

--nab

[Andy Grover]

Post by Nicholas A. Bellinger
I'm currently reviewing #2, #4, #5 and #7 and will consider merging
these. These cleanups account for most of the LOC reduction, and avoid
most of the larger concerns.
Also for patches like this, they really need testing on your end before
I'll consider them merging. Compile testing alone for these types of
locking changes is not enough, given the history of regressions with
these types of cleanups.

OK. I'll look for those to show up in your tree, then? And work on
better dividing up of the remaining changes, and also minimize their
risk of causing regressions.

Regards -- Andy