| Message ID | 1497649650-7605-4-git-send-email-mchristi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-06-16 at 16:47 -0500, Mike Christie wrote: > In the next patches we will add tcmu netlink support that allows > userspace to send commands to target_core_user. To execute operations > on a se_device/tcmu_dev we need to be able to look up a dev by any old > id. This patch replaces the se_device->dev_index with a idr created > id. > > The next patches will also remove the g_device_list and replace it with > the idr. Hello Mike, Although this patch looks fine to me I would appreciate it if all uses of scsi_get_new_index() would be converted into IDR. It doesn't make sense to me to use a single data structure (scsi_mib_index[]) to keep track of indexes of HBA instances, device instances and ACL instances. Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2017 01:13 PM, Bart Van Assche wrote: > On Fri, 2017-06-16 at 16:47 -0500, Mike Christie wrote: >> In the next patches we will add tcmu netlink support that allows >> userspace to send commands to target_core_user. To execute operations >> on a se_device/tcmu_dev we need to be able to look up a dev by any old >> id. This patch replaces the se_device->dev_index with a idr created >> id. >> >> The next patches will also remove the g_device_list and replace it with >> the idr. > > Hello Mike, > > Although this patch looks fine to me I would appreciate it if all uses of > scsi_get_new_index() would be converted into IDR. It doesn't make sense to > me to use a single data structure (scsi_mib_index[]) to keep track of > indexes of HBA instances, device instances and ACL instances. > Just to make sure I understood you, do you mean have one idr for hba, acl and dev index ids or one idr for acl, one for hba, etc? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-06-21 at 13:20 -0500, Mike Christie wrote: > On 06/21/2017 01:13 PM, Bart Van Assche wrote: > > On Fri, 2017-06-16 at 16:47 -0500, Mike Christie wrote: > > > In the next patches we will add tcmu netlink support that allows > > > userspace to send commands to target_core_user. To execute operations > > > on a se_device/tcmu_dev we need to be able to look up a dev by any old > > > id. This patch replaces the se_device->dev_index with a idr created > > > id. > > > > > > The next patches will also remove the g_device_list and replace it with > > > the idr. > > > > Hello Mike, > > > > Although this patch looks fine to me I would appreciate it if all uses of > > scsi_get_new_index() would be converted into IDR. It doesn't make sense to > > me to use a single data structure (scsi_mib_index[]) to keep track of > > indexes of HBA instances, device instances and ACL instances. > > Just to make sure I understood you, do you mean have one idr for hba, > acl and dev index ids or one idr for acl, one for hba, etc? Hello Mike, What I meant is one idr for ACL, one for HBA and one for devices. That should match the current behavior of scsi_get_new_index(). Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2017 01:13 PM, Bart Van Assche wrote: > On Fri, 2017-06-16 at 16:47 -0500, Mike Christie wrote: >> In the next patches we will add tcmu netlink support that allows >> userspace to send commands to target_core_user. To execute operations >> on a se_device/tcmu_dev we need to be able to look up a dev by any old >> id. This patch replaces the se_device->dev_index with a idr created >> id. >> >> The next patches will also remove the g_device_list and replace it with >> the idr. > > Hello Mike, > > Although this patch looks fine to me I would appreciate it if all uses of > scsi_get_new_index() would be converted into IDR. It doesn't make sense to > me to use a single data structure (scsi_mib_index[]) to keep track of > indexes of HBA instances, device instances and ACL instances. > Hey Bart, I will handle this comment in a separate patchset. I am almost done and will post tomorrow when it is done testing. IDA seems like a better fit for acl and hba indexes since we do not need the lookup and iter parts, and they do not have a cyclic allocator like idr does, so wanted to add a idr.c function for that. Also, hba_id and hba_index seem like duplicates, so there is extra cleanup. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c index 9b8abd5..f075860 100644 --- a/drivers/target/target_core_configfs.c +++ b/drivers/target/target_core_configfs.c @@ -3196,6 +3196,8 @@ static int __init target_core_init_configfs(void) config_group_init(&subsys->su_group); mutex_init(&subsys->su_mutex); + target_init_device_idr(); + ret = init_se_kmem_caches(); if (ret < 0) return ret; @@ -3300,6 +3302,7 @@ static void __exit target_core_exit_configfs(void) rd_module_exit(); target_xcopy_release_pt(); release_se_kmem_caches(); + target_destroy_device_idr(); } MODULE_DESCRIPTION("Target_Core_Mod/ConfigFS"); diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 16a701f..25fbc27 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -51,6 +51,7 @@ DEFINE_MUTEX(g_device_mutex); LIST_HEAD(g_device_list); +static struct idr devices_idr; static struct se_hba *lun0_hba; /* not static, needed by tpg.c */ @@ -879,10 +880,20 @@ sector_t target_to_linux_sector(struct se_device *dev, sector_t lb) } EXPORT_SYMBOL(target_to_linux_sector); +void target_init_device_idr(void) +{ + idr_init(&devices_idr); +} + +void target_destroy_device_idr(void) +{ + idr_destroy(&devices_idr); +} + int target_configure_device(struct se_device *dev) { struct se_hba *hba = dev->se_hba; - int ret; + int ret, id; if (dev->dev_flags & DF_CONFIGURED) { pr_err("se_dev->se_dev_ptr already set for storage" @@ -890,9 +901,26 @@ int target_configure_device(struct se_device *dev) return -EEXIST; } + /* + * Add early so modules like tcmu can use during its + * configuration. + */ + mutex_lock(&g_device_mutex); + /* + * Use cyclic to try and avoid collisions with devices + * that were recently removed. + */ + id = idr_alloc_cyclic(&devices_idr, dev, 0, INT_MAX, GFP_KERNEL); + mutex_unlock(&g_device_mutex); + if (id < 0) { + ret = -ENOMEM; + goto out; + } + dev->dev_index = id; + ret = dev->transport->configure_device(dev); if (ret) - goto out; + goto out_free_index; /* * XXX: there is not much point to have two different values here.. */ @@ -907,12 +935,11 @@ int target_configure_device(struct se_device *dev) dev->dev_attrib.hw_block_size); dev->dev_attrib.optimal_sectors = dev->dev_attrib.hw_max_sectors; - dev->dev_index = scsi_get_new_index(SCSI_DEVICE_INDEX); dev->creation_time = get_jiffies_64(); ret = core_setup_alua(dev); if (ret) - goto out; + goto out_free_index; /* * Startup the struct se_device processing thread @@ -960,6 +987,10 @@ int target_configure_device(struct se_device *dev) out_free_alua: core_alua_free_lu_gp_mem(dev); +out_free_index: + mutex_lock(&g_device_mutex); + idr_remove(&devices_idr, dev->dev_index); + mutex_unlock(&g_device_mutex); out: se_release_vpd_for_dev(dev); return ret; @@ -977,6 +1008,7 @@ void target_free_device(struct se_device *dev) dev->transport->destroy_device(dev); mutex_lock(&g_device_mutex); + idr_remove(&devices_idr, dev->dev_index); list_del(&dev->g_dev_node); mutex_unlock(&g_device_mutex); diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h index 0912de7..dfa923b 100644 --- a/drivers/target/target_core_internal.h +++ b/drivers/target/target_core_internal.h @@ -132,6 +132,8 @@ struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg, /* target_core_transport.c */ extern struct kmem_cache *se_tmr_req_cache; +void target_init_device_idr(void); +void target_destroy_device_idr(void); int init_se_kmem_caches(void); void release_se_kmem_caches(void); u32 scsi_get_new_index(scsi_index_t); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index a3af69f..51a92f1 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -219,7 +219,6 @@ enum tcm_tmrsp_table { */ typedef enum { SCSI_INST_INDEX, - SCSI_DEVICE_INDEX, SCSI_AUTH_INTR_INDEX, SCSI_INDEX_TYPE_MAX } scsi_index_t;
In the next patches we will add tcmu netlink support that allows userspace to send commands to target_core_user. To execute operations on a se_device/tcmu_dev we need to be able to look up a dev by any old id. This patch replaces the se_device->dev_index with a idr created id. The next patches will also remove the g_device_list and replace it with the idr. Signed-off-by: Mike Christie <mchristi@redhat.com> --- drivers/target/target_core_configfs.c | 3 +++ drivers/target/target_core_device.c | 40 +++++++++++++++++++++++++++++++---- drivers/target/target_core_internal.h | 2 ++ include/target/target_core_base.h | 1 - 4 files changed, 41 insertions(+), 5 deletions(-)