| Message ID | 1455298733-18651-1-git-send-email-lduncan@suse.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" == 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...
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> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>
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);
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(-)