[PATCH] Use ida_simple for SCSI iSCSI transport session id

[PATCH] Use ida_simple for SCSI iSCSI transport session id

[Lee Duncan]

The scsi_transport_iscsi module already uses the ida_simple
routines for managing the target ID, if requested to do
so. This change replaces an ever-increasing atomic integer
that tracks the session ID itself with the ida_simple
family of routines. This means that the session ID
will be reclaimed and can be reused when the session
is freed.

Note that no maximum is placed on this value, though
user-space currently only seems to use the lower 24-bits.
It seems better to handle this in user space, though,
than to limit the value range for the session ID here.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/scsi_transport_iscsi.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 441481623fb9..50a10bf214a8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -79,7 +79,8 @@ struct iscsi_internal {
 	struct transport_container session_cont;
 };
 
-static atomic_t iscsi_session_nr; /* sysfs session id for next new session */
+static DEFINE_IDA(iscsi_session_id_ida);
+
 static struct workqueue_struct *iscsi_eh_timer_workq;
 
 static DEFINE_IDA(iscsi_sess_ida);
@@ -2074,7 +2075,12 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 	int err;
 
 	ihost = shost->shost_data;
-	session->sid = atomic_add_return(1, &iscsi_session_nr);
+	session->sid = ida_simple_get(&iscsi_session_id_ida, 0, 0, GFP_KERNEL);
+	if (session->sid < 0) {
+		iscsi_cls_session_printk(KERN_ERR, session,
+					 "Failure in Session ID Allocation\n");
+		return session->sid;
+	}
 
 	if (target_id == ISCSI_MAX_TARGET) {
 		id = ida_simple_get(&iscsi_sess_ida, 0, 0, GFP_KERNEL);
@@ -2082,7 +2088,8 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 		if (id < 0) {
 			iscsi_cls_session_printk(KERN_ERR, session,
 					"Failure in Target ID Allocation\n");
-			return id;
+			err = id;
+			goto release_session_id_ida;
 		}
 		session->target_id = (unsigned int)id;
 		session->ida_used = true;
@@ -2109,6 +2116,8 @@ int iscsi_add_session(struct iscsi_cls_session *session, unsigned int target_id)
 release_ida:
 	if (session->ida_used)
 		ida_simple_remove(&iscsi_sess_ida, session->target_id);
+release_session_id_ida:
+	ida_simple_remove(&iscsi_session_id_ida, session->target_id);
 
 	return err;
 }
@@ -2214,6 +2223,7 @@ void iscsi_free_session(struct iscsi_cls_session *session)
 {
 	ISCSI_DBG_TRANS_SESSION(session, "Freeing session\n");
 	iscsi_session_event(session, ISCSI_KEVENT_DESTROY_SESSION);
+	ida_simple_remove(&iscsi_session_id_ida, session->target_id);
 	put_device(&session->dev);
 }
 EXPORT_SYMBOL_GPL(iscsi_free_session);
@@ -4524,8 +4534,6 @@ static __init int iscsi_transport_init(void)
 	printk(KERN_INFO "Loading iSCSI transport class v%s.\n",
 		ISCSI_TRANSPORT_VERSION);
 
-	atomic_set(&iscsi_session_nr, 0);
-
 	err = class_register(&iscsi_transport_class);
 	if (err)
 		return err;
@@ -4598,6 +4606,7 @@ static void __exit iscsi_transport_exit(void)
 	class_unregister(&iscsi_endpoint_class);
 	class_unregister(&iscsi_iface_class);
 	class_unregister(&iscsi_transport_class);
+	ida_destroy(&iscsi_session_id_ida);
 }
 
 module_init(iscsi_transport_init);
-- 
2.1.4

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[James Bottomley]

On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
> The scsi_transport_iscsi module already uses the ida_simple
> routines for managing the target ID, if requested to do
> so. This change replaces an ever-increasing atomic integer
> that tracks the session ID itself with the ida_simple
> family of routines. This means that the session ID
> will be reclaimed and can be reused when the session
> is freed.

Is reusing session ID's really a good idea?  For sequential sessions it
means that the ID of the next session will be re-used, i.e. the same as
the previous sessions, which could lead to target confusion.  I think
local uniqueness of session IDs is more important than wrap around
because sessions are short lived entities and the chances of the same
session being alive by the time we've wrapped is pretty tiny.

If you can demostrate a multi-target problem, perhaps we should rather
fix this by making the next session id a target local quantity?

James

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Mike Christie]

On 02/12/2016 11:54 AM, James Bottomley wrote:
> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
>> The scsi_transport_iscsi module already uses the ida_simple
>> routines for managing the target ID, if requested to do
>> so. This change replaces an ever-increasing atomic integer
>> that tracks the session ID itself with the ida_simple
>> family of routines. This means that the session ID
>> will be reclaimed and can be reused when the session
>> is freed.
> 
> Is reusing session ID's really a good idea?

I think it is if we do it by the iscsi rfc. We have two issues.

1. For iSCSI's iSID we need a 24 bit id. The iSID + initiator name is
the SCSI initiator port ID, so targets want the iSID to be reused for
things like persistent reservation tracking.

There are a couple red hat bugzillas. I am not sure if they are open
though. Here is a KB

https://access.redhat.com/solutions/66861

for more info.

We have been lazy and did not implement this and were just reusing part
of the session->sid.

2. For the kobject/sysfs name we want a unique name/id. Linux boxes stay
up a long time and some users login and logout of sessions a lot. We
roll over and have collisions.

Lee's patch was only meant to fix this. The reuse part was not meant to
fix #1, and so it does not handle every case like you suspect.


To fix everything how about this:

- Use Lee's patch to fix #2. We do not need the iSID to match the sysfs
name. Userspace does not do any type of iSCSI iSID <-> kernel session id
matching.

- For #1, in the userespace node db code we can implement some iSID
allocation/management code that follows the spec. We can also add a
blacklist there in case we find broken targets.

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Chris Leech]

On Fri, Feb 12, 2016 at 09:54:51AM -0800, James Bottomley wrote:
> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
> > The scsi_transport_iscsi module already uses the ida_simple
> > routines for managing the target ID, if requested to do
> > so. This change replaces an ever-increasing atomic integer
> > that tracks the session ID itself with the ida_simple
> > family of routines. This means that the session ID
> > will be reclaimed and can be reused when the session
> > is freed.
> 
> Is reusing session ID's really a good idea?  For sequential sessions it
> means that the ID of the next session will be re-used, i.e. the same as
> the previous sessions, which could lead to target confusion.  I think
> local uniqueness of session IDs is more important than wrap around
> because sessions are short lived entities and the chances of the same
> session being alive by the time we've wrapped is pretty tiny.

I've got a few complaints about target resources being tied up because
we don't reuse session IDs.  The ISID becomes a component in the
I_T nexus identifier, so changing it invalidates persistent reservations.

> If you can demostrate a multi-target problem, perhaps we should rather
> fix this by making the next session id a target local quantity?

Mike's got a good point that we don't really need to base the ISID off
of our local session identifier (kobject name).  I think getting reuse
right may be a bit trickier than being a target local value, because it
needs to be unique across target portal groups.  Which probably furthers
the argument that we should deal with that in the userspace tools.

If we plan to split the protocol ISID cleanly from the kobject name,
I guess the question is if aggressive reuse of the local identifier is
better than dealing with the unlikely collision on rollover?

- Chris

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Mike Christie]

On 02/15/2016 12:26 PM, Chris Leech wrote:
> On Fri, Feb 12, 2016 at 09:54:51AM -0800, James Bottomley wrote:
>> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
>>> The scsi_transport_iscsi module already uses the ida_simple
>>> routines for managing the target ID, if requested to do
>>> so. This change replaces an ever-increasing atomic integer
>>> that tracks the session ID itself with the ida_simple
>>> family of routines. This means that the session ID
>>> will be reclaimed and can be reused when the session
>>> is freed.
>>
>> Is reusing session ID's really a good idea?  For sequential sessions it
>> means that the ID of the next session will be re-used, i.e. the same as
>> the previous sessions, which could lead to target confusion.  I think
>> local uniqueness of session IDs is more important than wrap around
>> because sessions are short lived entities and the chances of the same
>> session being alive by the time we've wrapped is pretty tiny.
> 
> I've got a few complaints about target resources being tied up because
> we don't reuse session IDs.  The ISID becomes a component in the
> I_T nexus identifier, so changing it invalidates persistent reservations.
> 
>> If you can demostrate a multi-target problem, perhaps we should rather
>> fix this by making the next session id a target local quantity?
> 
> Mike's got a good point that we don't really need to base the ISID off
> of our local session identifier (kobject name).  I think getting reuse
> right may be a bit trickier than being a target local value, because it
> needs to be unique across target portal groups.  Which probably furthers
> the argument that we should deal with that in the userspace tools.
> 
> If we plan to split the protocol ISID cleanly from the kobject name,
> I guess the question is if aggressive reuse of the local identifier is
> better than dealing with the unlikely collision on rollover?

I thought Lee's patch to convert the host_no from a atomic_t to ida
based was merged in Martin's tree. If that is going upstream, then I
thought you would want to fix the session id too.

Is the concern similar to /dev/sdX reuse and bad apps? In this case,
some app might do a logout and login, but not update the sysfs mapping.
You could then hit corruption due to the sysfs session id now mapping to
a different target.

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Chris Leech]

On Tue, Feb 16, 2016 at 12:40:12PM -0600, Mike Christie wrote:
> On 02/15/2016 12:26 PM, Chris Leech wrote:
> > On Fri, Feb 12, 2016 at 09:54:51AM -0800, James Bottomley wrote:
> >> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
> >>> The scsi_transport_iscsi module already uses the ida_simple
> >>> routines for managing the target ID, if requested to do
> >>> so. This change replaces an ever-increasing atomic integer
> >>> that tracks the session ID itself with the ida_simple
> >>> family of routines. This means that the session ID
> >>> will be reclaimed and can be reused when the session
> >>> is freed.
> >>
> >> Is reusing session ID's really a good idea?  For sequential sessions it
> >> means that the ID of the next session will be re-used, i.e. the same as
> >> the previous sessions, which could lead to target confusion.  I think
> >> local uniqueness of session IDs is more important than wrap around
> >> because sessions are short lived entities and the chances of the same
> >> session being alive by the time we've wrapped is pretty tiny.
> > 
> > I've got a few complaints about target resources being tied up because
> > we don't reuse session IDs.  The ISID becomes a component in the
> > I_T nexus identifier, so changing it invalidates persistent reservations.
> > 
> >> If you can demostrate a multi-target problem, perhaps we should rather
> >> fix this by making the next session id a target local quantity?
> > 
> > Mike's got a good point that we don't really need to base the ISID off
> > of our local session identifier (kobject name).  I think getting reuse
> > right may be a bit trickier than being a target local value, because it
> > needs to be unique across target portal groups.  Which probably furthers
> > the argument that we should deal with that in the userspace tools.
> > 
> > If we plan to split the protocol ISID cleanly from the kobject name,
> > I guess the question is if aggressive reuse of the local identifier is
> > better than dealing with the unlikely collision on rollover?
> 
> I thought Lee's patch to convert the host_no from a atomic_t to ida
> based was merged in Martin's tree. If that is going upstream, then I
> thought you would want to fix the session id too.
> 
> Is the concern similar to /dev/sdX reuse and bad apps? In this case,
> some app might do a logout and login, but not update the sysfs mapping.
> You could then hit corruption due to the sysfs session id now mapping to
> a different target.

OK, I don't want to cause a rehash of the same concerns. I took a look
at whatever code I could think to check in that builds on top of the
Open-iSCSI tools, and I don't think this will break anything.

I'm OK with this.

- Chris

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Lee Duncan]

On 02/12/2016 09:54 AM, James Bottomley wrote:
> On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
>> The scsi_transport_iscsi module already uses the ida_simple
>> routines for managing the target ID, if requested to do
>> so. This change replaces an ever-increasing atomic integer
>> that tracks the session ID itself with the ida_simple
>> family of routines. This means that the session ID
>> will be reclaimed and can be reused when the session
>> is freed.
> 
> Is reusing session ID's really a good idea?  For sequential sessions it
> means that the ID of the next session will be re-used, i.e. the same as
> the previous sessions, which could lead to target confusion.  I think
> local uniqueness of session IDs is more important than wrap around
> because sessions are short lived entities and the chances of the same
> session being alive by the time we've wrapped is pretty tiny.
> 
> If you can demostrate a multi-target problem, perhaps we should rather
> fix this by making the next session id a target local quantity?
> 
> James
> 

It looks like Mike and Chris are good with it. And I'd really like to
get rid of yet another atomic int.

Are you satisfied with this one?
-- 
Lee

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Martin K. Petersen]

>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:

Lee> It looks like Mike and Chris are good with it.

However, it received no formal reviews or acked-bys...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Chris Leech]

On Fri, Feb 12, 2016 at 09:38:53AM -0800, Lee Duncan wrote:
> The scsi_transport_iscsi module already uses the ida_simple
> routines for managing the target ID, if requested to do
> so. This change replaces an ever-increasing atomic integer
> that tracks the session ID itself with the ida_simple
> family of routines. This means that the session ID
> will be reclaimed and can be reused when the session
> is freed.
> 
> Note that no maximum is placed on this value, though
> user-space currently only seems to use the lower 24-bits.
> It seems better to handle this in user space, though,
> than to limit the value range for the session ID here.
> 
> Signed-off-by: Lee Duncan <lduncan@suse.com>

Acked-by: Chris Leech <cleech@redhat.com>

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

[Johannes Thumshirn]

On Fri, Feb 12, 2016 at 09:38:53AM -0800, Lee Duncan wrote:
> The scsi_transport_iscsi module already uses the ida_simple
> routines for managing the target ID, if requested to do
> so. This change replaces an ever-increasing atomic integer
> that tracks the session ID itself with the ida_simple
> family of routines. This means that the session ID
> will be reclaimed and can be reused when the session
> is freed.
> 
> Note that no maximum is placed on this value, though
> user-space currently only seems to use the lower 24-bits.
> It seems better to handle this in user space, though,
> than to limit the value range for the session ID here.
> 
> Signed-off-by: Lee Duncan <lduncan@suse.com>

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850