* [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns
@ 2014-03-06 22:15 Andy Grover
2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover
` (7 more replies)
0 siblings, 8 replies; 26+ messages in thread
From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi
Hi nab and everyone,
This patchset reduces the amount of memory for se_dev_entry and se_lun
arrays by waiting to allocate array members, and includes some related
simplification patches too. This is a rework of a few patches I submitted
in December in a larger series, but keeping it simple by sticking with
fixed-size arrays for device_list and tpg_lun_list for now.
Testing: created and removed luns and mapped luns, worked ok.
Regards -- Andy
Andy Grover (6):
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: Rename core_tpg_post_dellun to remove_lun
target: Allocate se_luns only when used
target: Remove core_tpg_release_virtual_lun0 function
drivers/target/sbp/sbp_target.c | 6 +-
drivers/target/target_core_device.c | 206 ++++++++++-----------------
drivers/target/target_core_fabric_configfs.c | 25 ++--
drivers/target/target_core_internal.h | 9 +-
drivers/target/target_core_spc.c | 2 +-
drivers/target/target_core_tpg.c | 154 +++++---------------
include/target/target_core_base.h | 13 +-
7 files changed, 136 insertions(+), 279 deletions(-)
--
1.8.5.3
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-07 10:33 ` Christoph Hellwig 2014-03-06 22:15 ` [PATCH 2/6] target: core_tpg_post_dellun can return void Andy Grover ` (6 subsequent siblings) 7 siblings, 1 reply; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 108 +++++++++++++++------------ drivers/target/target_core_fabric_configfs.c | 2 +- drivers/target/target_core_spc.c | 2 +- drivers/target/target_core_tpg.c | 18 +---- include/target/target_core_base.h | 5 +- 5 files changed, 69 insertions(+), 66 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 65001e1..234498b 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; @@ -250,7 +249,7 @@ int core_free_device_list_for_node( 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,7 +267,7 @@ int core_free_device_list_for_node( } spin_unlock_irq(&nacl->device_list_lock); - array_free(nacl->device_list, TRANSPORT_MAX_LUNS_PER_TPG); + kfree(nacl->device_list); nacl->device_list = NULL; return 0; @@ -283,12 +282,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); } @@ -306,7 +307,12 @@ int core_enable_device_list_for_node( struct se_portal_group *tpg) { struct se_port *port = lun->lun_sep; - struct se_dev_entry *deve; + struct se_dev_entry *deve, *new_deve; + + /* Likely allocing a new deve, so alloc first, outside lock */ + new_deve = kzalloc(sizeof(*new_deve), GFP_KERNEL); + if (!new_deve) + return -ENOMEM; spin_lock_irq(&nacl->device_list_lock); @@ -317,7 +323,8 @@ 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) { + kfree(new_deve); if (deve->se_lun_acl != NULL) { pr_err("struct se_dev_entry->se_lun_acl" " already set for demo mode -> explicit" @@ -346,25 +353,28 @@ int core_enable_device_list_for_node( return 0; } - deve->se_lun = lun; - deve->se_lun_acl = lun_acl; - deve->mapped_lun = mapped_lun; - deve->lun_flags |= TRANSPORT_LUNFLAGS_INITIATOR_ACCESS; + nacl->device_list[mapped_lun] = new_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; - } + atomic_set(&new_deve->ua_count, 0); + atomic_set(&new_deve->pr_ref_count, 0); + spin_lock_init(&new_deve->ua_lock); + INIT_LIST_HEAD(&new_deve->alua_port_list); + INIT_LIST_HEAD(&new_deve->ua_list); + new_deve->se_lun = lun; + new_deve->se_lun_acl = lun_acl; + new_deve->mapped_lun = mapped_lun; - deve->creation_time = get_jiffies_64(); - deve->attach_count++; + if (lun_access & TRANSPORT_LUNFLAGS_READ_WRITE) + new_deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_WRITE; + else + new_deve->lun_flags |= TRANSPORT_LUNFLAGS_READ_ONLY; + + new_deve->creation_time = get_jiffies_64(); + new_deve->attach_count++; spin_unlock_irq(&nacl->device_list_lock); spin_lock_bh(&port->sep_alua_lock); - list_add_tail(&deve->alua_port_list, &port->sep_alua_list); + list_add_tail(&new_deve->alua_port_list, &port->sep_alua_list); spin_unlock_bh(&port->sep_alua_lock); return 0; @@ -383,7 +393,23 @@ 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; + } + + /* + * Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE + * PR operation to complete. + */ + while (atomic_read(&deve->pr_ref_count) != 0) + cpu_relax(); /* * If the MappedLUN entry is being disabled, the entry in @@ -398,29 +424,19 @@ 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); - /* - * Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE - * PR operation to complete. - */ - while (atomic_read(&deve->pr_ref_count) != 0) - cpu_relax(); + spin_unlock(&port->sep_alua_lock); - 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--; + nacl->device_list[mapped_lun] = NULL; + kfree(deve); + spin_unlock_irq(&nacl->device_list_lock); - core_scsi3_free_pr_reg_from_nacl(lun->lun_se_dev, nacl); return 0; } @@ -441,7 +457,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 7de9f04..635c857 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 = diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c index 43c5ca98..d91cce7 100644 --- a/drivers/target/target_core_spc.c +++ b/drivers/target/target_core_spc.c @@ -1239,7 +1239,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 c036595..fbed75f 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) { @@ -223,25 +223,13 @@ static void *array_zalloc(int n, size_t size, gfp_t flags) */ 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); + nacl->device_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), + 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; } diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index c9c7912..0c10d75 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, }; /* -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used 2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover @ 2014-03-07 10:33 ` Christoph Hellwig 0 siblings, 0 replies; 26+ messages in thread From: Christoph Hellwig @ 2014-03-07 10:33 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, linux-scsi > 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); > + nacl->device_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), > + GFP_KERNEL); Seems like ->device_list should simply be an array instead of a pointer. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/6] target: core_tpg_post_dellun can return void 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-06 22:15 ` [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover ` (5 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Nothing in it can raise an error. Signed-off-by: Andy Grover <agrover@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 fbed75f..63033ae 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -855,18 +855,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; } -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover 2014-03-06 22:15 ` [PATCH 2/6] target: core_tpg_post_dellun can return void Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-07 10:35 ` Christoph Hellwig 2014-03-06 22:15 ` [PATCH 4/6] target: Rename core_tpg_post_dellun to remove_lun Andy Grover ` (4 subsequent siblings) 7 siblings, 1 reply; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 18 +++++----------- drivers/target/target_core_fabric_configfs.c | 2 +- drivers/target/target_core_internal.h | 3 +-- drivers/target/target_core_tpg.c | 32 +--------------------------- 4 files changed, 8 insertions(+), 47 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 234498b..c98a82a 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1261,24 +1261,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_post_dellun(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 635c857..35da34d 100644 --- a/drivers/target/target_core_fabric_configfs.c +++ b/drivers/target/target_core_fabric_configfs.c @@ -821,7 +821,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..22c4261 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,7 +82,6 @@ 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 *); /* target_core_transport.c */ diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index 63033ae..3b07e27 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -323,7 +323,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); @@ -825,36 +825,6 @@ 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( struct se_portal_group *tpg, struct se_lun *lun) -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun 2014-03-06 22:15 ` [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover @ 2014-03-07 10:35 ` Christoph Hellwig 2014-03-07 18:20 ` Andy Grover 0 siblings, 1 reply; 26+ messages in thread From: Christoph Hellwig @ 2014-03-07 10:35 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, linux-scsi On Thu, Mar 06, 2014 at 02:15:28PM -0800, Andy Grover wrote: > +void core_dev_del_lun( > struct se_portal_group *tpg, > + struct se_lun *lun) > { > + 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), lun->unpacked_lun, > tpg->se_tpg_tfo->get_fabric_name()); > > + core_tpg_post_dellun(tpg, lun); Seems like core_tpg_post_dellun should be folded into core_dev_del_lun? Also would be nice to get some sane names for all the functions in this area.. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun 2014-03-07 10:35 ` Christoph Hellwig @ 2014-03-07 18:20 ` Andy Grover 0 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-07 18:20 UTC (permalink / raw) To: Christoph Hellwig; +Cc: target-devel, linux-scsi On 03/07/2014 02:35 AM, Christoph Hellwig wrote: > On Thu, Mar 06, 2014 at 02:15:28PM -0800, Andy Grover wrote: >> +void core_dev_del_lun( >> struct se_portal_group *tpg, >> + struct se_lun *lun) >> { >> + 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), lun->unpacked_lun, >> tpg->se_tpg_tfo->get_fabric_name()); >> >> + core_tpg_post_dellun(tpg, lun); > > Seems like core_tpg_post_dellun should be folded into core_dev_del_lun? It gets more stuff added back to it in a later patch, and post_dellun gets renamed too. Regards -- Andy ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/6] target: Rename core_tpg_post_dellun to remove_lun 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (2 preceding siblings ...) 2014-03-06 22:15 ` [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-06 22:15 ` [PATCH 5/6] target: Allocate se_luns only when used Andy Grover ` (3 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi A clearer name, now that pre_dellun is gone. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 2 +- drivers/target/target_core_internal.h | 2 +- drivers/target/target_core_tpg.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index c98a82a..cb69413 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1270,7 +1270,7 @@ void core_dev_del_lun( tpg->se_tpg_tfo->tpg_get_tag(tpg), lun->unpacked_lun, tpg->se_tpg_tfo->get_fabric_name()); - core_tpg_post_dellun(tpg, lun); + 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_internal.h b/drivers/target/target_core_internal.h index 22c4261..42ef4ab 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -82,7 +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 *); -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 3b07e27..914a0a6 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -655,7 +655,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( @@ -825,7 +825,7 @@ int core_tpg_add_lun( return 0; } -void core_tpg_post_dellun( +void core_tpg_remove_lun( struct se_portal_group *tpg, struct se_lun *lun) { -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 5/6] target: Allocate se_luns only when used 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (3 preceding siblings ...) 2014-03-06 22:15 ` [PATCH 4/6] target: Rename core_tpg_post_dellun to remove_lun Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-07 10:41 ` Christoph Hellwig 2014-03-06 22:15 ` [PATCH 6/6] target: Remove core_tpg_release_virtual_lun0 function Andy Grover ` (2 subsequent siblings) 7 siblings, 1 reply; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Instead of allocating the tpg_lun_list and each member of the list when the tpg is created, just create the tpg_lun_list array, 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 and core_dev_get_lun. Change core_dev_add_lun to take a se_lun and return int Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/sbp/sbp_target.c | 6 +- drivers/target/target_core_device.c | 80 +++--------------------- drivers/target/target_core_fabric_configfs.c | 21 +++---- drivers/target/target_core_internal.h | 4 +- drivers/target/target_core_tpg.c | 92 ++++++++++------------------ include/target/target_core_base.h | 8 --- 6 files changed, 57 insertions(+), 154 deletions(-) diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c index 24884ca..bee56e5 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); @@ -1942,7 +1942,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++; @@ -2022,7 +2022,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 cb69413..afa7f6b 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1214,22 +1214,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(), @@ -1254,7 +1249,7 @@ struct se_lun *core_dev_add_lun( spin_unlock_irq(&tpg->acl_node_lock); } - return lun; + return rc; } /* core_dev_del_lun(): @@ -1271,68 +1266,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_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; + core_tpg_free_lun(tpg, lun); } struct se_lun_acl *core_dev_init_initiator_node_lun_acl( @@ -1374,7 +1308,9 @@ int core_dev_add_initiator_node_lun_acl( struct se_lun *lun; struct se_node_acl *nacl; - lun = core_dev_get_lun(tpg, unpacked_lun); + spin_lock(&tpg->tpg_lun_lock); + lun = tpg->tpg_lun_list[unpacked_lun]; + spin_unlock(&tpg->tpg_lun_lock); if (!lun) { pr_err("%s Logical Unit Number: %u is not active on" " Target Portal Group: %hu, ignoring request.\n", diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c index 35da34d..50a71bb 100644 --- a/drivers/target/target_core_fabric_configfs.c +++ b/drivers/target/target_core_fabric_configfs.c @@ -754,7 +754,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); @@ -782,11 +781,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) { @@ -799,8 +797,6 @@ static int target_fabric_port_link( } return 0; -out: - return ret; } static int target_fabric_port_unlink( @@ -886,16 +882,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, @@ -917,6 +914,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 914a0a6..25e3a8e 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; spin_unlock(&tpg->tpg_lun_lock); @@ -189,34 +189,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_create_device_list_for_node(): * * @@ -318,8 +290,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); @@ -636,7 +607,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); @@ -665,30 +635,14 @@ 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); + se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), + 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; @@ -703,8 +657,7 @@ int core_tpg_register( 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); + kfree(se_tpg->tpg_lun_list); return -ENOMEM; } } @@ -764,7 +717,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); + kfree(se_tpg->tpg_lun_list); return 0; } EXPORT_SYMBOL(core_tpg_deregister); @@ -773,7 +726,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" @@ -784,9 +737,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(), @@ -794,9 +752,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( @@ -819,7 +789,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; @@ -832,8 +801,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 0c10d75..6981642 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, @@ -702,8 +696,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; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] target: Allocate se_luns only when used 2014-03-06 22:15 ` [PATCH 5/6] target: Allocate se_luns only when used Andy Grover @ 2014-03-07 10:41 ` Christoph Hellwig 2014-03-07 18:12 ` Andy Grover 0 siblings, 1 reply; 26+ messages in thread From: Christoph Hellwig @ 2014-03-07 10:41 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, linux-scsi > - se_tpg->tpg_lun_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG, > - sizeof(struct se_lun), GFP_KERNEL); > + se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), > + GFP_KERNEL); Again seems like the pointer array should just be embedded instead of dynamically allocated. Also given that it's properly typdef the kcalloc should have used the type instead of void *. And it should have been kcalloc to start with.. > +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); I can't see how the synchronization can work without refcounting the lun structure. The lock just protectes the assignment, but you free it right after. What happens to how jsut dereferenced it under the lock but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] target: Allocate se_luns only when used 2014-03-07 10:41 ` Christoph Hellwig @ 2014-03-07 18:12 ` Andy Grover 2014-03-07 18:22 ` Christoph Hellwig 0 siblings, 1 reply; 26+ messages in thread From: Andy Grover @ 2014-03-07 18:12 UTC (permalink / raw) To: Christoph Hellwig; +Cc: target-devel, linux-scsi On 03/07/2014 02:41 AM, Christoph Hellwig wrote: >> - se_tpg->tpg_lun_list = array_zalloc(TRANSPORT_MAX_LUNS_PER_TPG, >> - sizeof(struct se_lun), GFP_KERNEL); >> + se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), >> + GFP_KERNEL); > > Again seems like the pointer array should just be embedded instead of > dynamically allocated. Also given that it's properly typdef the kcalloc > should have used the type instead of void *. And it should have been > kcalloc to start with.. Yup. I posted a "7 of 6" patch that folds the arrays into their parent structures. >> +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); > > I can't see how the synchronization can work without refcounting the lun > structure. The lock just protectes the assignment, but you free it > right after. What happens to how jsut dereferenced it under the lock > but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). Well you're right, but this is one instance of a larger lio locking/refcounting hairball. This will be addressed in a separate patch series. Regards -- Andy ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] target: Allocate se_luns only when used 2014-03-07 18:12 ` Andy Grover @ 2014-03-07 18:22 ` Christoph Hellwig 2014-03-07 18:31 ` Andy Grover 2014-03-12 19:04 ` Nicholas A. Bellinger 0 siblings, 2 replies; 26+ messages in thread From: Christoph Hellwig @ 2014-03-07 18:22 UTC (permalink / raw) To: Andy Grover; +Cc: Christoph Hellwig, target-devel, linux-scsi On Fri, Mar 07, 2014 at 10:12:09AM -0800, Andy Grover wrote: > >I can't see how the synchronization can work without refcounting the lun > >structure. The lock just protectes the assignment, but you free it > >right after. What happens to how jsut dereferenced it under the lock > >but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). > > Well you're right, but this is one instance of a larger lio > locking/refcounting hairball. This will be addressed in a separate > patch series. I don't think that's true. Before your series we might be accessing a lun structure that was marked as not active just before, but now the race becomes a genuine use after free. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] target: Allocate se_luns only when used 2014-03-07 18:22 ` Christoph Hellwig @ 2014-03-07 18:31 ` Andy Grover 2014-03-12 19:04 ` Nicholas A. Bellinger 1 sibling, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-07 18:31 UTC (permalink / raw) To: Christoph Hellwig; +Cc: target-devel, linux-scsi On 03/07/2014 10:22 AM, Christoph Hellwig wrote: > On Fri, Mar 07, 2014 at 10:12:09AM -0800, Andy Grover wrote: >>> I can't see how the synchronization can work without refcounting the lun >>> structure. The lock just protectes the assignment, but you free it >>> right after. What happens to how jsut dereferenced it under the lock >>> but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). >> >> Well you're right, but this is one instance of a larger lio >> locking/refcounting hairball. This will be addressed in a separate >> patch series. > > I don't think that's true. Before your series we might be accessing a > lun structure that was marked as not active just before, but now the > race becomes a genuine use after free. OK. I'll work on a follow-on patch that ensures the locks are held long enough. Thanks -- Andy ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/6] target: Allocate se_luns only when used 2014-03-07 18:22 ` Christoph Hellwig 2014-03-07 18:31 ` Andy Grover @ 2014-03-12 19:04 ` Nicholas A. Bellinger 1 sibling, 0 replies; 26+ messages in thread From: Nicholas A. Bellinger @ 2014-03-12 19:04 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Andy Grover, target-devel, linux-scsi On Fri, 2014-03-07 at 10:22 -0800, Christoph Hellwig wrote: > On Fri, Mar 07, 2014 at 10:12:09AM -0800, Andy Grover wrote: > > >I can't see how the synchronization can work without refcounting the lun > > >structure. The lock just protectes the assignment, but you free it > > >right after. What happens to how jsut dereferenced it under the lock > > >but then uses it outside (e.g. core_dev_add_initiator_node_lun_acl). > > > > Well you're right, but this is one instance of a larger lio > > locking/refcounting hairball. This will be addressed in a separate > > patch series. > > I don't think that's true. Before your series we might be accessing a > lun structure that was marked as not active just before, but now the > race becomes a genuine use after free. > FYI, since v3.13 code se_lun is using percpu refcounting with commit 5277797d.. --nab ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 6/6] target: Remove core_tpg_release_virtual_lun0 function 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (4 preceding siblings ...) 2014-03-06 22:15 ` [PATCH 5/6] target: Allocate se_luns only when used Andy Grover @ 2014-03-06 22:15 ` Andy Grover 2014-03-07 17:50 ` [PATCH 7/6] target: Do not allocate device_list and tpg_lun_list arrays separately Andy Grover 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 7 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-06 22:15 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Simple and just called from one place. Signed-off-by: Andy Grover <agrover@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 25e3a8e..522b325 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -621,13 +621,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, @@ -714,7 +707,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; kfree(se_tpg->tpg_lun_list); -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 7/6] target: Do not allocate device_list and tpg_lun_list arrays separately 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (5 preceding siblings ...) 2014-03-06 22:15 ` [PATCH 6/6] target: Remove core_tpg_release_virtual_lun0 function Andy Grover @ 2014-03-07 17:50 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 7 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-07 17:50 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Instead of allocing these dynamically, include them in their parent structures. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 6 ----- drivers/target/target_core_tpg.c | 44 +++---------------------------------- include/target/target_core_base.h | 4 ++-- 3 files changed, 5 insertions(+), 49 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index afa7f6b..173c4f0 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -242,9 +242,6 @@ 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]; @@ -267,9 +264,6 @@ int core_free_device_list_for_node( } spin_unlock_irq(&nacl->device_list_lock); - kfree(nacl->device_list); - nacl->device_list = NULL; - return 0; } diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index 522b325..38a6d1b 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -189,23 +189,6 @@ static int core_set_queue_depth_for_node( return 0; } -/* core_create_device_list_for_node(): - * - * - */ -static int core_create_device_list_for_node(struct se_node_acl *nacl) -{ - nacl->device_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), - GFP_KERNEL); - if (!nacl->device_list) { - pr_err("Unable to allocate memory for" - " struct se_node_acl->device_list\n"); - return -ENOMEM; - } - - return 0; -} - /* core_tpg_check_initiator_node_acl() * * @@ -242,11 +225,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); @@ -367,11 +345,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); @@ -628,14 +601,6 @@ int core_tpg_register( void *tpg_fabric_ptr, int se_tpg_type) { - se_tpg->tpg_lun_list = kzalloc(TRANSPORT_MAX_LUNS_PER_TPG * sizeof(void*), - GFP_KERNEL); - if (!se_tpg->tpg_lun_list) { - pr_err("Unable to allocate struct se_portal_group->" - "tpg_lun_list\n"); - return -ENOMEM; - } - se_tpg->se_tpg_type = se_tpg_type; se_tpg->se_tpg_fabric_ptr = tpg_fabric_ptr; se_tpg->se_tpg_tfo = tfo; @@ -648,12 +613,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) { - kfree(se_tpg->tpg_lun_list); + 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); @@ -710,7 +672,7 @@ int core_tpg_deregister(struct se_portal_group *se_tpg) core_tpg_remove_lun(se_tpg, &se_tpg->tpg_virt_lun0); se_tpg->se_tpg_fabric_ptr = NULL; - kfree(se_tpg->tpg_lun_list); + return 0; } EXPORT_SYMBOL(core_tpg_deregister); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index 6981642..c0d93c5 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -577,7 +577,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; @@ -864,7 +864,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; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (6 preceding siblings ...) 2014-03-07 17:50 ` [PATCH 7/6] target: Do not allocate device_list and tpg_lun_list arrays separately Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list Andy Grover ` (8 more replies) 7 siblings, 9 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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, and includes some related simplification patches too. This is a rework of a few patches I submitted in December in a larger series, but keeping it simple by sticking with fixed-size arrays for device_list and tpg_lun_list for now. Testing: created and removed luns and mapped luns, worked ok. Changes in v2: Added patches 1-2, which ensure locking around the arrays Added patch 9, refactors enable_device_list_for_node Patch 7/6 folded into series Regards -- Andy Andy Grover (9): 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: Rename core_tpg_post_dellun to remove_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 | 311 +++++++++++++-------------- 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, 246 insertions(+), 366 deletions(-) -- 1.8.5.3 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 2/9] target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs Andy Grover ` (7 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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 <hch@lst.de> Signed-off-by: Andy Grover <agrover@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 65001e1..54f5b14 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 2f5d779..1068b4f 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 505519b..c63abad 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: * -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 2/9] target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-10 22:45 ` [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 3/9] target: Allocate se_dev_entrys in device list only when used Andy Grover ` (6 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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 <agrover@redhat.com> --- drivers/target/target_core_device.c | 78 ++++++++++++++----------------------- drivers/target/target_core_tpg.c | 3 -- 2 files changed, 29 insertions(+), 52 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 54f5b14..6d91e4d 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -295,7 +295,7 @@ void core_update_device_list_access( /* core_enable_device_list_for_node(): * - * + * Called with tpg_lun_lock held & irqs off */ int core_enable_device_list_for_node( struct se_lun *lun, @@ -307,8 +307,9 @@ 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); + spin_lock(&nacl->device_list_lock); deve = nacl->device_list[mapped_lun]; @@ -322,15 +323,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 +343,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 +366,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(): @@ -1295,39 +1296,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, @@ -1366,19 +1334,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(&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)) @@ -1388,7 +1361,10 @@ int core_dev_add_initiator_node_lun_acl( if (core_enable_device_list_for_node(lun, lacl, lacl->mapped_lun, lun_access, nacl, tpg) < 0) - return -EINVAL; + { + ret = -EINVAL; + goto out; + } spin_lock(&lun->lun_acl_lock); list_add_tail(&lacl->lacl_list, &lun->lun_acl_list); @@ -1406,7 +1382,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(&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); } -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 3/9] target: Allocate se_dev_entrys in device list only when used 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-10 22:45 ` [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list Andy Grover 2014-03-10 22:45 ` [PATCHv2 2/9] target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 4/9] target: core_tpg_post_dellun can return void Andy Grover ` (5 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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 <agrover@redhat.com> --- drivers/target/target_core_device.c | 94 +++++++++++++++------------- 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(+), 89 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 6d91e4d..8a37471 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); } @@ -318,7 +313,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" @@ -346,18 +341,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++; @@ -385,7 +389,23 @@ 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; + } + + /* + * Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE + * PR operation to complete. + */ + while (atomic_read(&deve->pr_ref_count) != 0) + cpu_relax(); /* * If the MappedLUN entry is being disabled, the entry in @@ -400,29 +420,19 @@ 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); - /* - * Wait for any in process SPEC_I_PT=1 or REGISTER_AND_MOVE - * PR operation to complete. - */ - while (atomic_read(&deve->pr_ref_count) != 0) - cpu_relax(); + spin_unlock(&port->sep_alua_lock); - 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--; + nacl->device_list[mapped_lun] = NULL; + kfree(deve); + spin_unlock_irq(&nacl->device_list_lock); - core_scsi3_free_pr_reg_from_nacl(lun->lun_se_dev, nacl); return 0; } @@ -443,7 +453,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 43c5ca98..d91cce7 100644 --- a/drivers/target/target_core_spc.c +++ b/drivers/target/target_core_spc.c @@ -1239,7 +1239,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 c9c7912..34e1ae4 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, }; /* @@ -584,7 +583,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; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 4/9] target: core_tpg_post_dellun can return void 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (2 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 3/9] target: Allocate se_dev_entrys in device list only when used Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 5/9] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover ` (4 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Nothing in it can raise an error. Signed-off-by: Andy Grover <agrover@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; } -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 5/9] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (3 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 4/9] target: core_tpg_post_dellun can return void Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 6/9] target: Rename core_tpg_post_dellun to remove_lun Andy Grover ` (3 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 18 +++++----------- drivers/target/target_core_fabric_configfs.c | 2 +- drivers/target/target_core_internal.h | 3 +-- drivers/target/target_core_tpg.c | 32 +--------------------------- 4 files changed, 8 insertions(+), 47 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 8a37471..4e3de34 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1257,24 +1257,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_post_dellun(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..22c4261 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,7 +82,6 @@ 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 *); /* target_core_transport.c */ diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c index 321268d..b50d667 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); @@ -795,36 +795,6 @@ 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( struct se_portal_group *tpg, struct se_lun *lun) -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 6/9] target: Rename core_tpg_post_dellun to remove_lun 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (4 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 5/9] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 7/9] target: Allocate se_luns only when used Andy Grover ` (2 subsequent siblings) 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi A clearer name, now that pre_dellun is gone. Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 2 +- drivers/target/target_core_internal.h | 2 +- drivers/target/target_core_tpg.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 4e3de34..7224689 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1266,7 +1266,7 @@ void core_dev_del_lun( tpg->se_tpg_tfo->tpg_get_tag(tpg), lun->unpacked_lun, tpg->se_tpg_tfo->get_fabric_name()); - core_tpg_post_dellun(tpg, lun); + 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_internal.h b/drivers/target/target_core_internal.h index 22c4261..42ef4ab 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -82,7 +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 *); -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 b50d667..5d2c774 100644 --- a/drivers/target/target_core_tpg.c +++ b/drivers/target/target_core_tpg.c @@ -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,7 +795,7 @@ int core_tpg_add_lun( return 0; } -void core_tpg_post_dellun( +void core_tpg_remove_lun( struct se_portal_group *tpg, struct se_lun *lun) { -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 7/9] target: Allocate se_luns only when used 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (5 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 6/9] target: Rename core_tpg_post_dellun to remove_lun Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 8/9] target: Remove core_tpg_release_virtual_lun0 function Andy Grover 2014-03-10 22:45 ` [PATCHv2 9/9] target: Refactor core_enable_device_list_for_node Andy Grover 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi 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. Signed-off-by: Andy Grover <agrover@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 24884ca..bee56e5 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); @@ -1942,7 +1942,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++; @@ -2022,7 +2022,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 7224689..0bd09c8 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -1210,22 +1210,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(), @@ -1250,7 +1245,7 @@ struct se_lun *core_dev_add_lun( spin_unlock_irq(&tpg->acl_node_lock); } - return lun; + return rc; } /* core_dev_del_lun(): @@ -1267,35 +1262,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 34e1ae4..c0d93c5 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, @@ -702,8 +696,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; @@ -872,7 +864,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; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 8/9] target: Remove core_tpg_release_virtual_lun0 function 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (6 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 7/9] target: Allocate se_luns only when used Andy Grover @ 2014-03-10 22:45 ` Andy Grover 2014-03-10 22:45 ` [PATCHv2 9/9] target: Refactor core_enable_device_list_for_node Andy Grover 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Simple and just called from one place. Signed-off-by: Andy Grover <agrover@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; -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCHv2 9/9] target: Refactor core_enable_device_list_for_node 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover ` (7 preceding siblings ...) 2014-03-10 22:45 ` [PATCHv2 8/9] target: Remove core_tpg_release_virtual_lun0 function Andy Grover @ 2014-03-10 22:45 ` Andy Grover 8 siblings, 0 replies; 26+ messages in thread From: Andy Grover @ 2014-03-10 22:45 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Create helper functions to alloc a deve and to transition it from demo mode to explicit. Signed-off-by: Andy Grover <agrover@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 0bd09c8..c15b7aa 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(): * * Called with tpg_lun_lock held & irqs off @@ -314,62 +381,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); -- 1.8.5.3 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2014-03-12 19:04 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-06 22:15 [PATCH 0/6] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-06 22:15 ` [PATCH 1/6] target: Allocate se_dev_entrys in device list only when used Andy Grover 2014-03-07 10:33 ` Christoph Hellwig 2014-03-06 22:15 ` [PATCH 2/6] target: core_tpg_post_dellun can return void Andy Grover 2014-03-06 22:15 ` [PATCH 3/6] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover 2014-03-07 10:35 ` Christoph Hellwig 2014-03-07 18:20 ` Andy Grover 2014-03-06 22:15 ` [PATCH 4/6] target: Rename core_tpg_post_dellun to remove_lun Andy Grover 2014-03-06 22:15 ` [PATCH 5/6] target: Allocate se_luns only when used Andy Grover 2014-03-07 10:41 ` Christoph Hellwig 2014-03-07 18:12 ` Andy Grover 2014-03-07 18:22 ` Christoph Hellwig 2014-03-07 18:31 ` Andy Grover 2014-03-12 19:04 ` Nicholas A. Bellinger 2014-03-06 22:15 ` [PATCH 6/6] target: Remove core_tpg_release_virtual_lun0 function Andy Grover 2014-03-07 17:50 ` [PATCH 7/6] target: Do not allocate device_list and tpg_lun_list arrays separately Andy Grover 2014-03-10 22:45 ` [PATCHv2 0/9] target: Save memory on unused se_dev_entrys and se_luns Andy Grover 2014-03-10 22:45 ` [PATCHv2 1/9] target: Add locking to some accesses to nacl.device_list Andy Grover 2014-03-10 22:45 ` [PATCHv2 2/9] target: Don't unlock/relock tpg_lun_lock in loop in add_node_to_devs Andy Grover 2014-03-10 22:45 ` [PATCHv2 3/9] target: Allocate se_dev_entrys in device list only when used Andy Grover 2014-03-10 22:45 ` [PATCHv2 4/9] target: core_tpg_post_dellun can return void Andy Grover 2014-03-10 22:45 ` [PATCHv2 5/9] target: Change core_dev_del_lun to take a se_lun instead of unpacked_lun Andy Grover 2014-03-10 22:45 ` [PATCHv2 6/9] target: Rename core_tpg_post_dellun to remove_lun Andy Grover 2014-03-10 22:45 ` [PATCHv2 7/9] target: Allocate se_luns only when used Andy Grover 2014-03-10 22:45 ` [PATCHv2 8/9] target: Remove core_tpg_release_virtual_lun0 function Andy Grover 2014-03-10 22:45 ` [PATCHv2 9/9] target: Refactor core_enable_device_list_for_node Andy Grover
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).