[PATCH 04/15] target/iscsi: move session_index to common se_session
The next patches will make session$SID dir for each session so move the iscsi session session_index to the se_session. This differs from the previous code in that it now uses idr_alloc_cyclic to help prevent apps from confusing a reused sid with a previous session. Signed-off-by: Mike Christie --- drivers/target/iscsi/iscsi_target.c | 8 drivers/target/iscsi/iscsi_target_configfs.c | 4 +--- drivers/target/iscsi/iscsi_target_login.c| 21 - drivers/target/iscsi/iscsi_target_stat.c | 3 +-- drivers/target/target_core_transport.c | 25 - include/target/iscsi/iscsi_target_core.h | 2 -- include/target/target_core_base.h| 1 + 7 files changed, 27 insertions(+), 37 deletions(-) diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c index d547dcd..6d81a59 100644 --- a/drivers/target/iscsi/iscsi_target.c +++ b/drivers/target/iscsi/iscsi_target.c @@ -57,9 +57,7 @@ static DEFINE_SPINLOCK(tiqn_lock); static DEFINE_MUTEX(np_lock); static struct idr tiqn_idr; -struct idr sess_idr; struct mutex auth_id_lock; -spinlock_t sess_idr_lock; struct iscsit_global *iscsit_global; @@ -698,9 +696,7 @@ static int __init iscsi_target_init_module(void) spin_lock_init(&iscsit_global->ts_bitmap_lock); mutex_init(&auth_id_lock); - spin_lock_init(&sess_idr_lock); idr_init(&tiqn_idr); - idr_init(&sess_idr); ret = target_register_template(&iscsi_ops); if (ret) @@ -4373,10 +4369,6 @@ int iscsit_close_session(struct iscsi_session *sess) pr_debug("Decremented number of active iSCSI Sessions on" " iSCSI TPG: %hu to %u\n", tpg->tpgt, tpg->nsessions); - spin_lock(&sess_idr_lock); - idr_remove(&sess_idr, sess->session_index); - spin_unlock(&sess_idr_lock); - kfree(sess->sess_ops); sess->sess_ops = NULL; spin_unlock_bh(&se_tpg->session_lock); diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c index 1fcd9bc..39af194 100644 --- a/drivers/target/iscsi/iscsi_target_configfs.c +++ b/drivers/target/iscsi/iscsi_target_configfs.c @@ -1357,9 +1357,7 @@ static int iscsi_get_cmd_state(struct se_cmd *se_cmd) static u32 lio_sess_get_index(struct se_session *se_sess) { - struct iscsi_session *sess = se_sess->fabric_sess_ptr; - - return sess->session_index; + return se_sess->sid; } static u32 lio_sess_get_initiator_sid( diff --git a/drivers/target/iscsi/iscsi_target_login.c b/drivers/target/iscsi/iscsi_target_login.c index 923b1a9..d6a4b93 100644 --- a/drivers/target/iscsi/iscsi_target_login.c +++ b/drivers/target/iscsi/iscsi_target_login.c @@ -336,22 +336,6 @@ static int iscsi_login_zero_tsih_s1( timer_setup(&sess->time2retain_timer, iscsit_handle_time2retain_timeout, 0); - idr_preload(GFP_KERNEL); - spin_lock_bh(&sess_idr_lock); - ret = idr_alloc(&sess_idr, NULL, 0, 0, GFP_NOWAIT); - if (ret >= 0) - sess->session_index = ret; - spin_unlock_bh(&sess_idr_lock); - idr_preload_end(); - - if (ret < 0) { - pr_err("idr_alloc() for sess_idr failed\n"); - iscsit_tx_login_rsp(conn, ISCSI_STATUS_CLS_TARGET_ERR, - ISCSI_LOGIN_STATUS_NO_RESOURCES); - kfree(sess); - return -ENOMEM; - } - sess->creation_time = get_jiffies_64(); /* * The FFP CmdSN window values will be allocated from the TPG's @@ -1163,11 +1147,6 @@ void iscsi_target_login_sess_out(struct iscsi_conn *conn, goto old_sess_out; if (conn->sess->se_sess) transport_free_session(conn->sess->se_sess); - if (conn->sess->session_index != 0) { - spin_lock_bh(&sess_idr_lock); - idr_remove(&sess_idr, conn->sess->session_index); - spin_unlock_bh(&sess_idr_lock); - } kfree(conn->sess->sess_ops); kfree(conn->sess); conn->sess = NULL; diff --git a/drivers/target/iscsi/iscsi_target_stat.c b/drivers/target/iscsi/iscsi_target_stat.c index df0a398..b1a41bf5 100644 --- a/drivers/target/iscsi/iscsi_target_stat.c +++ b/drivers/target/iscsi/iscsi_target_stat.c @@ -638,8 +638,7 @@ static ssize_t iscsi_stat_sess_indx_show(struct config_item *item, char *page) if (se_sess) { sess = se_sess->fabric_sess_ptr; if (sess) - ret = snprintf(page, PAGE_SIZE, "%u\n", - sess->session_index); + ret = snprintf(page, PAGE_SIZE, "%u\n", se_sess->sid); } spin_unlock_bh(&se_nacl->nacl_sess_lock); diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 75ddbbb..97a1ee5 100644 --- a/drivers/target/target_core_tra
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c > index 75ddbbb..97a1ee5 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -55,6 +55,8 @@ > > static struct workqueue_struct *target_completion_wq; > static struct kmem_cache *se_sess_cache; > +static DEFINE_SPINLOCK(se_sess_idr_lock); > +static DEFINE_IDR(se_sess_idr); Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these two data structures be members of the data structure associated with /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)? Thanks, Bart.
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On 07/18/2018 05:19 PM, Bart Van Assche wrote: > On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: >> diff --git a/drivers/target/target_core_transport.c >> b/drivers/target/target_core_transport.c >> index 75ddbbb..97a1ee5 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -55,6 +55,8 @@ >> >> static struct workqueue_struct *target_completion_wq; >> static struct kmem_cache *se_sess_cache; >> +static DEFINE_SPINLOCK(se_sess_idr_lock); >> +static DEFINE_IDR(se_sess_idr); > > Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these > two data structures be members of the data structure associated with > /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)? For tcmu we have a problem where we pass the scsi commands to userspace but then we need to know what I_T nexus it was sent through or what target port it was received on. I thought I could reuse the sid for tcmu commands where I could embed the sid in the tcmu_cmd and then userspace can look up the sid and know what session the command came in on. If the device is exported through 2 tpgs then we need the sid target wide in case sessions on different tpgs have the same sid. And, then I thought if you exported the device through 2 fabrics then I thought you need it set globally. I am still working on that part with Bodo, so I can make it per tpg when I resend then do another path to change it later.
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On 07/18/2018 07:15 PM, Mike Christie wrote: > On 07/18/2018 05:19 PM, Bart Van Assche wrote: >> On Sun, 2018-07-15 at 18:16 -0500, Mike Christie wrote: >>> diff --git a/drivers/target/target_core_transport.c >>> b/drivers/target/target_core_transport.c >>> index 75ddbbb..97a1ee5 100644 >>> --- a/drivers/target/target_core_transport.c >>> +++ b/drivers/target/target_core_transport.c >>> @@ -55,6 +55,8 @@ >>> >>> static struct workqueue_struct *target_completion_wq; >>> static struct kmem_cache *se_sess_cache; >>> +static DEFINE_SPINLOCK(se_sess_idr_lock); >>> +static DEFINE_IDR(se_sess_idr); >> >> Is it necessary that se_sess_idr_lock and se_sess_idr are global? Could these >> two data structures be members of the data structure associated with >> /sys/kernel/config/target/iscsi/$port/$tpg (struct se_portal_group?)? > > For tcmu we have a problem where we pass the scsi commands to userspace > but then we need to know what I_T nexus it was sent through or what > target port it was received on. > > I thought I could reuse the sid for tcmu commands where I could embed > the sid in the tcmu_cmd and then userspace can look up the sid and know > what session the command came in on. If the device is exported through 2 > tpgs then we need the sid target wide in case sessions on different tpgs > have the same sid. And, then I thought if you exported the device > through 2 fabrics then I thought you need it set globally. > > I am still working on that part with Bodo, so I can make it per tpg when > I resend then do another path to change it later. Hey Bart, I looked into this some more and this value is also being used as the scsiAttIntrPortIndex. For that use, does it need to be unique across a target when the target has multiple ports? So I think it needs to be on the se_wwn right?
Re: [PATCH 04/15] target/iscsi: move session_index to common se_session
On Wed, 2018-07-18 at 22:47 -0500, Mike Christie wrote: > Hey Bart, I looked into this some more and this value is also being used > as the scsiAttIntrPortIndex. For that use, does it need to be unique > across a target when the target has multiple ports? > > So I think it needs to be on the se_wwn right? Hello Mike, I think the best solution would be to decouple the session index that is used to export session information through configfs from the session index used by the SCSI-MIB (scsiAttIntrPortIndex). That approach would allow to make both indexes consecutive integers in all cases for both interfaces. However, neither configfs nor the SCSI-MIB requires that session indexes are consecutive integers. So I think moving se_sess_idr_lock and se_sess_idr into struct se_wwn is fine. Bart.