Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1332976 > unrolled thread

[PATCH] Use ida_simple for SCSI iSCSI transport session id

Back to article view | Back to linux.kernel

Contents

  [PATCH] Use ida_simple for SCSI iSCSI transport session id Lee Duncan <lduncan@suse.com> - 2016-02-12 18:50 +0100
    Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id James Bottomley <James.Bottomley@HansenPartnership.com> - 2016-02-12 19:00 +0100
      Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id Mike Christie <michaelc@cs.wisc.edu> - 2016-02-12 23:20 +0100
      Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id Chris Leech <cleech@redhat.com> - 2016-02-15 19:30 +0100
        Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id Mike Christie <michaelc@cs.wisc.edu> - 2016-02-16 19:50 +0100
          Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id Chris Leech <cleech@redhat.com> - 2016-02-17 23:40 +0100

#1332976 — [PATCH] Use ida_simple for SCSI iSCSI transport session id

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

[toc] | [next] | [standalone]

#1332979

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

[toc] | [prev] | [next] | [standalone]

#1333175

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.

[toc] | [prev] | [next] | [standalone]

#1334700

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

[toc] | [prev] | [next] | [standalone]

#1335721

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.

[toc] | [prev] | [next] | [standalone]

#1336830

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web