Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
On 03/09/2016 09:05 PM, Mike Christie wrote: > On 03/08/2016 11:21 AM, Chris Leech wrote: >> 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 >> >> Acked-by: Chris Leech >> > > Dropping lkml and linux-scsi for a moment for some userspace stuff. > > If we do this patch, then we need Chris's sysfs attr cache removal and > in another patch I think we also need to drop the dev_list cache from > open-iscsi/usr/sysfs.c? > > For example, I think we could hit a bug because we could match a session > id but the cached device's parent could be different than before. This will cause problems if I want to use current userspace open-iscsi with a newer kernel version once the kernel patch is merged, right? Example use case: I install a stable version of any long term supported distro (RHEL, SLES, Ubuntu, Debian), but use a newer kernel because I want to install it on newer server hardware. OTOH, I get why you want to make this change in the kernel and I agree with the rationale for it. What about this logic? - still keep an atomic int additionally - pass that atomic int as the start parameter to ida_simple_get - upon return, increment the atomic int (replace value with MAX(current atomic int value, new id) atomically) finally: - if atomic int >= 0x7ff (the max for ida) just use 0 as start value and don't modify the atomic int anymore (then we have wrap around anyway, and then the current code doesn't handle that well anyway, so it doesn't deteriorate) (I'm not a kernel hacker, and I'm assuming that ida_simple_get will use low numbers by default that increase slowly - otherwise this obviously won't work very well.) This is more complicated, but it will preserve current semantics until a wrap-around occurs and not break the current userspace code. Then, in a couple of years or so, we can drop the additional atomic int complication in the kernel and just use ida for ID allocation, once we know that the removal of the cacvhe has been in userspace for long enough. Thoughts? Regards, Christian -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
On 03/08/2016 11:21 AM, Chris Leech wrote: > 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 > > Acked-by: Chris Leech > Dropping lkml and linux-scsi for a moment for some userspace stuff. If we do this patch, then we need Chris's sysfs attr cache removal and in another patch I think we also need to drop the dev_list cache from open-iscsi/usr/sysfs.c? For example, I think we could hit a bug because we could match a session id but the cached device's parent could be different than before. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 Reviewed-by: Johannes Thumshirn -- Johannes Thumshirn Storage jthumsh...@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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 Acked-by: Chris Leech -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
> "Lee" == Lee Duncan 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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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. -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id
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 -- You received this message because you are subscribed to the Google Groups "open-iscsi" group. To unsubscribe from this group and stop receiving emails from it, send an email to open-iscsi+unsubscr...@googlegroups.com. To post to this group, send email to open-iscsi@googlegroups.com. Visit this group at https://groups.google.com/group/open-iscsi. For more options, visit https://groups.google.com/d/optout.
