| Message ID | 20200528163625.110184-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: use xarray for devices and targets | expand |
On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: > Use xarray for device lookup by target. LUNs below 256 are linear, > and can be used directly as the index into the xarray. > LUNs above 256 have a distinct LUN format, and are not necessarily > linear. They'll be stored in indices above 256 in the xarray, with > the next free index in the xarray. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > drivers/scsi/scsi.c | 32 +++++++++++++++++++------------- > drivers/scsi/scsi_lib.c | 9 ++++----- > drivers/scsi/scsi_scan.c | 3 +-- > drivers/scsi/scsi_sysfs.c | 17 +++++++++++++++-- > include/scsi/scsi_device.h | 4 ++-- > 5 files changed, 41 insertions(+), 24 deletions(-) > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index d601424e32b2..9dbbc51a1eb5 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, > void starget_for_each_device(struct scsi_target *starget, void *data, > void (*fn)(struct scsi_device *, void *)) > { > - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > struct scsi_device *sdev; > + unsigned long lun_id = 0; > > - shost_for_each_device(sdev, shost) { > - if ((sdev->channel == starget->channel) && > - (sdev->id == starget->id)) > - fn(sdev, data); > + xa_for_each(&starget->devices, lun_id, sdev) { > + if (scsi_device_get(sdev)) > + continue; > + fn(sdev, data); > + scsi_device_put(sdev); > } > } > EXPORT_SYMBOL(starget_for_each_device); > @@ -629,14 +630,11 @@ EXPORT_SYMBOL(starget_for_each_device); > void __starget_for_each_device(struct scsi_target *starget, void *data, > void (*fn)(struct scsi_device *, void *)) > { > - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > struct scsi_device *sdev; > + unsigned long lun_id = 0; > > - __shost_for_each_device(sdev, shost) { > - if ((sdev->channel == starget->channel) && > - (sdev->id == starget->id)) > - fn(sdev, data); > - } > + xa_for_each(&starget->devices, lun_id, sdev) > + fn(sdev, data); > } > EXPORT_SYMBOL(__starget_for_each_device); > > @@ -659,11 +657,19 @@ struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget, > u64 lun) > { > struct scsi_device *sdev; > + unsigned long lun_idx = 256; > + > + if (lun < lun_idx) { > + sdev = xa_load(&starget->devices, lun); > + if (sdev && sdev->sdev_state == SDEV_DEL) > + sdev = NULL; > + return sdev; > + } > > - list_for_each_entry(sdev, &starget->devices, same_target_siblings) { > + xa_for_each(&starget->devices, lun_idx, sdev) { > if (sdev->sdev_state == SDEV_DEL) > continue; > - if (sdev->lun ==lun) > + if (sdev->lun == lun) > return sdev; > } > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index c163fa22267c..f9829fc0d84e 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -361,9 +361,9 @@ static void scsi_kick_queue(struct request_queue *q) > static void scsi_single_lun_run(struct scsi_device *current_sdev) > { > struct Scsi_Host *shost = current_sdev->host; > - struct scsi_device *sdev, *tmp; > + struct scsi_device *sdev; > struct scsi_target *starget = scsi_target(current_sdev); > - unsigned long flags; > + unsigned long flags, lun_id = 0; > > spin_lock_irqsave(shost->host_lock, flags); > starget->starget_sdev_user = NULL; > @@ -380,8 +380,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) > spin_lock_irqsave(shost->host_lock, flags); > if (starget->starget_sdev_user) > goto out; > - list_for_each_entry_safe(sdev, tmp, &starget->devices, > - same_target_siblings) { > + xa_for_each(&starget->devices, lun_id, sdev) { > if (sdev == current_sdev) > continue; > if (scsi_device_get(sdev)) > @@ -390,7 +389,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) > spin_unlock_irqrestore(shost->host_lock, flags); > scsi_kick_queue(sdev->request_queue); > spin_lock_irqsave(shost->host_lock, flags); > - > + > scsi_device_put(sdev); > } > out: > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index dc2656df495b..c7aba9ba5c0c 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -235,7 +235,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, > mutex_init(&sdev->state_mutex); > sdev->sdev_state = SDEV_CREATED; > INIT_LIST_HEAD(&sdev->siblings); > - INIT_LIST_HEAD(&sdev->same_target_siblings); > INIT_LIST_HEAD(&sdev->starved_entry); > INIT_LIST_HEAD(&sdev->event_list); > spin_lock_init(&sdev->list_lock); > @@ -417,7 +416,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, > starget->id = id; > starget->channel = channel; > starget->can_queue = 0; > - INIT_LIST_HEAD(&starget->devices); > + xa_init(&starget->devices); > starget->state = STARGET_CREATED; > starget->scsi_level = SCSI_2; > starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED; > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index 95aaa96ce03b..27c19232f175 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -434,6 +434,7 @@ static void scsi_device_cls_release(struct device *class_dev) > static void scsi_device_dev_release_usercontext(struct work_struct *work) > { > struct scsi_device *sdev; > + struct scsi_target *starget; > struct device *parent; > struct list_head *this, *tmp; > struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL; > @@ -441,6 +442,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) > unsigned long flags; > > sdev = container_of(work, struct scsi_device, ew.work); > + starget = sdev->sdev_target; > > scsi_dh_release_device(sdev); > > @@ -448,7 +450,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) > > spin_lock_irqsave(sdev->host->host_lock, flags); > list_del(&sdev->siblings); > - list_del(&sdev->same_target_siblings); > + xa_erase(&starget->devices, sdev->lun_idx); > list_del(&sdev->starved_entry); > spin_unlock_irqrestore(sdev->host->host_lock, flags); > > @@ -1621,7 +1623,18 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) > > transport_setup_device(&sdev->sdev_gendev); > spin_lock_irqsave(shost->host_lock, flags); > - list_add_tail(&sdev->same_target_siblings, &starget->devices); > + if (sdev->lun < 256) { > + sdev->lun_idx = sdev->lun; > + WARN_ON(xa_insert(&starget->devices, sdev->lun_idx, > + sdev, GFP_KERNEL) < 0); You have interrupts masked again, so GFP_KERNEL is a non-no. Also, like most modern kernel libraries, xarray returns 0 for good and a negated errno for an error. If you are going to WARN, I would like to know what that negated errno is. > + } else { > + struct xa_limit scsi_lun_limit = { > + .min = 256, > + .max = UINT_MAX, > + }; > + WARN_ON(xa_alloc(&starget->devices, &sdev->lun_idx, > + sdev, scsi_lun_limit, GFP_KERNEL) < 0); dito > + } > list_add_tail(&sdev->siblings, &shost->__devices); > spin_unlock_irqrestore(shost->host_lock, flags); > /* > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index 28034cc0fce5..2c6b9d8bc40e 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -104,7 +104,6 @@ struct scsi_device { > > /* the next two are protected by the host->host_lock */ > struct list_head siblings; /* list of all devices on this host */ > - struct list_head same_target_siblings; /* just the devices sharing same target id */ > > atomic_t device_busy; /* commands actually active on LLDD */ > atomic_t device_blocked; /* Device returned QUEUE_FULL. */ > @@ -123,6 +122,7 @@ struct scsi_device { > > unsigned int id, channel; > u64 lun; Is 'lun' a T10 LUN as in a big-endian array of bytes, converted to a 64 bit native unsigned integer, or is that the Linux 8 no, 16 no, 32 no, 64 bit hack? /* A comment might help there. */ > + unsigned int lun_idx; /* Index into target device xarray */ Xarray gives you _two_ choices for the type of an index :-) It is either u32 (as used in xa_alloc() and its variants) _or_ unsigned long (as used everywhere else in xarray where an index is needed). So it's definitely 32 bits for xa_alloc() and either 32 or 64 bits for the rest of xarray depending on the machine architecture. Matthew Wilcox may care to explain why this is ... By using 'unsigned int lun_idx;' you make a confusing picture even more so. A comment wouldn't hurt there either since the splat the compiler spits out it if you use 'unsigned int' for the second argument to the xa_for_each() macro [which is called 'index'] is not very instructive. [That macro takes the address of the given index and feeds it to xa_find_after() which expects 'unsigned long *' .] > unsigned int manufacturer; /* Manufacturer of device, for using > * vendor-specific cmd's */ > unsigned sector_size; /* size in bytes */ > @@ -285,7 +285,7 @@ enum scsi_target_state { > struct scsi_target { > struct scsi_device *starget_sdev_user; > struct list_head siblings; > - struct list_head devices; > + struct xarray devices; > struct device dev; > struct kref reap_ref; /* last put renders target invisible */ > unsigned int channel; >
On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: > On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: > > Use xarray for device lookup by target. LUNs below 256 are linear, > > and can be used directly as the index into the xarray. > > LUNs above 256 have a distinct LUN format, and are not necessarily > > linear. They'll be stored in indices above 256 in the xarray, with > > the next free index in the xarray. I don't understand why you think this is an improvement over just using an allocating XArray for all LUNs. It seems like more code, and doesn't actually save you any storage space ... ? > > +++ b/drivers/scsi/scsi.c > > @@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, > > void starget_for_each_device(struct scsi_target *starget, void *data, > > void (*fn)(struct scsi_device *, void *)) > > { > > - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > > struct scsi_device *sdev; > > + unsigned long lun_id = 0; > > - shost_for_each_device(sdev, shost) { > > - if ((sdev->channel == starget->channel) && > > - (sdev->id == starget->id)) > > - fn(sdev, data); > > + xa_for_each(&starget->devices, lun_id, sdev) { > > + if (scsi_device_get(sdev)) > > + continue; > > + fn(sdev, data); > > + scsi_device_put(sdev); > > } > > } I appreciate you're implementing the API that's currently in use, but I would recommend open-coding this. Looking at the functions called, lots of them are two-three liners, eg: static void __scsi_report_device_reset(struct scsi_device *sdev, void *data) { sdev->was_reset = 1; sdev->expecting_cc_ua = 1; } which would just be more readable: if (rtn == SUCCESS) { struct scsi_target *starget = scsi_target(scmd->device); struct scsi_device *sdev; unsigned long index; spin_lock_irqsave(host->host_lock, flags); xa_for_each(&starget->devices, index, sdev) { sdev->was_reset = 1; sdev->expecting_cc_ua = 1; } spin_unlock_irqrestore(host->host_lock, flags); } And then maybe you could make a convincing argument that the spin_lock there could be turned into an xa_lock since that will prevent sdevs from coming & going during the iteration of the device list. > > @@ -1621,7 +1623,18 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) > > transport_setup_device(&sdev->sdev_gendev); > > spin_lock_irqsave(shost->host_lock, flags); > > - list_add_tail(&sdev->same_target_siblings, &starget->devices); > > + if (sdev->lun < 256) { > > + sdev->lun_idx = sdev->lun; > > + WARN_ON(xa_insert(&starget->devices, sdev->lun_idx, > > + sdev, GFP_KERNEL) < 0); > > You have interrupts masked again, so GFP_KERNEL is a non-no. Actually, I would say this is a great place to not use the host_lock. + int err; ... + if (sdev->lun < 256) { + sdev->lun_idx = sdev->lun; + err = xa_insert_irq(&starget->devices, sdev->lun_idx, sdev, + GFP_KERNEL); + } else { + err = xa_alloc_irq(&starget->devices, &sdev->lun_idx, sdev, + scsi_lun_limit, GFP_KERNEL); + } + ... maybe you want to convert scsi_sysfs_device_initialize() + to return an errno, although honestly this should never fail ... spin_lock_irqsave(shost->host_lock, flags); - list_add_tail(&sdev->same_target_siblings, &starget->devices); list_add_tail(&sdev->siblings, &shost->__devices); spin_unlock_irqrestore(shost->host_lock, flags); > > + unsigned int lun_idx; /* Index into target device xarray */ > > Xarray gives you _two_ choices for the type of an index :-) > It is either u32 (as used in xa_alloc() and its variants) _or_ > unsigned long (as used everywhere else in xarray where an index > is needed). So it's definitely 32 bits for xa_alloc() and either > 32 or 64 bits for the rest of xarray depending on the machine > architecture. > Matthew Wilcox may care to explain why this is ... Saving space in data structures, mostly. It's not really useful to have 4 billion things in an allocating XArray. Indeed, it's not really useful to have 4 billion of almost anything. So we have XArrays which are indexed by something sensible like page index in file, and they're nice and well-behaved (most people don't create files in excess of 16TB, and those who do don't expect amazing performance when seeking around in them at random because they've lost all caching effects). And then you have people who probably want one scsi device. Maybe a dozen. Possibly a thousand. Definitely not a million. If you get 4 billion scsi devices, something's gone very wrong. You've run out of drive letters, for a start (/dev/sdmzabcd anybody?) It doesn't cost us anything to just use unsigned long as the index of an XArray, due to how it's constructed. Except in this one place where we need to point to somewhere to store the index so that we can update the index within an sdev before anybody iterating the data structure under RCU can find the uninitialised index. This patch seems decent enough ... I just think the decision to optimise the layout for LUNs < 256 is a bit odd. But hey, your subsystem, not mine.
On 2020-05-28 2:54 p.m., Matthew Wilcox wrote: > On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: >> On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: >>> Use xarray for device lookup by target. LUNs below 256 are linear, >>> and can be used directly as the index into the xarray. >>> LUNs above 256 have a distinct LUN format, and are not necessarily >>> linear. They'll be stored in indices above 256 in the xarray, with >>> the next free index in the xarray. > > I don't understand why you think this is an improvement over just > using an allocating XArray for all LUNs. It seems like more code, > and doesn't actually save you any storage space ... ? > >>> +++ b/drivers/scsi/scsi.c >>> @@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, >>> void starget_for_each_device(struct scsi_target *starget, void *data, >>> void (*fn)(struct scsi_device *, void *)) >>> { >>> - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); >>> struct scsi_device *sdev; >>> + unsigned long lun_id = 0; >>> - shost_for_each_device(sdev, shost) { >>> - if ((sdev->channel == starget->channel) && >>> - (sdev->id == starget->id)) >>> - fn(sdev, data); >>> + xa_for_each(&starget->devices, lun_id, sdev) { >>> + if (scsi_device_get(sdev)) >>> + continue; >>> + fn(sdev, data); >>> + scsi_device_put(sdev); >>> } >>> } > > I appreciate you're implementing the API that's currently in use, but I would recommend open-coding this. Looking at the functions called, lots of them > are two-three liners, eg: > > static void __scsi_report_device_reset(struct scsi_device *sdev, void *data) > { > sdev->was_reset = 1; > sdev->expecting_cc_ua = 1; > } > > which would just be more readable: > > if (rtn == SUCCESS) { > struct scsi_target *starget = scsi_target(scmd->device); > struct scsi_device *sdev; > unsigned long index; > > spin_lock_irqsave(host->host_lock, flags); > xa_for_each(&starget->devices, index, sdev) { > sdev->was_reset = 1; > sdev->expecting_cc_ua = 1; > } > spin_unlock_irqrestore(host->host_lock, flags); > } > > And then maybe you could make a convincing argument that the spin_lock > there could be turned into an xa_lock since that will prevent sdevs from > coming & going during the iteration of the device list. I tried that but ran into problems. The xarray model is clear enough, but there is a (non-atomic) enumerated state in each sdev (struct scsi_device object (pointer)) that is protected by a mutex. I was unable to escape those pesky (but very useful) warnings out of the depths of xarray that the locking was awry. When I've burrowed into xarray.c I have usually found that it was correct. So now I regard it as a pesky feature :-) That said, in my work on the sg driver rewrite, I got rid of all the explicit spinlocks in favour of xa_locks and that works fine, IMO. The locking in the SCSI ML is a more complex beast. Maybe it doesn't need to be ... Hannes, why not make sdev_state an atomic and get rid of that mutex? Virtually none of this code is on the command fastpath, but Ming Lei found that trimming down the size and position of members in sdev could lead to performance improvements. These changes may improve the time taken to stabilize a system with large disk arrays attached at startup, shutdown and when there are disruptions or errors. >>> @@ -1621,7 +1623,18 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) >>> transport_setup_device(&sdev->sdev_gendev); >>> spin_lock_irqsave(shost->host_lock, flags); >>> - list_add_tail(&sdev->same_target_siblings, &starget->devices); >>> + if (sdev->lun < 256) { >>> + sdev->lun_idx = sdev->lun; >>> + WARN_ON(xa_insert(&starget->devices, sdev->lun_idx, >>> + sdev, GFP_KERNEL) < 0); >> >> You have interrupts masked again, so GFP_KERNEL is a non-no. > > Actually, I would say this is a great place to not use the host_lock. > > + int err; > ... > + if (sdev->lun < 256) { > + sdev->lun_idx = sdev->lun; > + err = xa_insert_irq(&starget->devices, sdev->lun_idx, sdev, > + GFP_KERNEL); > + } else { > + err = xa_alloc_irq(&starget->devices, &sdev->lun_idx, sdev, > + scsi_lun_limit, GFP_KERNEL); > + } > + ... maybe you want to convert scsi_sysfs_device_initialize() > + to return an errno, although honestly this should never fail ... > spin_lock_irqsave(shost->host_lock, flags); > - list_add_tail(&sdev->same_target_siblings, &starget->devices); > list_add_tail(&sdev->siblings, &shost->__devices); > spin_unlock_irqrestore(shost->host_lock, flags); > > >>> + unsigned int lun_idx; /* Index into target device xarray */ >> >> Xarray gives you _two_ choices for the type of an index :-) >> It is either u32 (as used in xa_alloc() and its variants) _or_ >> unsigned long (as used everywhere else in xarray where an index >> is needed). So it's definitely 32 bits for xa_alloc() and either >> 32 or 64 bits for the rest of xarray depending on the machine >> architecture. >> Matthew Wilcox may care to explain why this is ... > > Saving space in data structures, mostly. It's not really useful to > have 4 billion things in an allocating XArray. Indeed, it's not really > useful to have 4 billion of almost anything. So we have XArrays which > are indexed by something sensible like page index in file, and they're > nice and well-behaved (most people don't create files in excess of 16TB, > and those who do don't expect amazing performance when seeking around > in them at random because they've lost all caching effects). And then > you have people who probably want one scsi device. Maybe a dozen. > Possibly a thousand. Definitely not a million. If you get 4 billion scsi > devices, something's gone very wrong. You've run out of drive letters, > for a start (/dev/sdmzabcd anybody?) no_uld=1 # RAID cards might do this on its physicals so smartmontools # can check them Then you only have to worry about /dev/sg<n> and /dev/bsg/<hctl) numbers or tuples going through the roof :-) > It doesn't cost us anything to just use unsigned long as the index of an > XArray, due to how it's constructed. Except in this one place where we > need to point to somewhere to store the index so that we can update the > index within an sdev before anybody iterating the data structure under > RCU can find the uninitialised index. > > This patch seems decent enough ... I just think the decision to optimise > the layout for LUNs < 256 is a bit odd. But hey, your subsystem, > not mine. Hannes has the most experience in that area. He has only been using xarray for a week or so (I think). There is something important missing from his patchset: any xa_destroy() calls :-) Doug Gilbert
On Thu, May 28, 2020 at 03:44:37PM -0400, Douglas Gilbert wrote: > > And then maybe you could make a convincing argument that the spin_lock > > there could be turned into an xa_lock since that will prevent sdevs from > > coming & going during the iteration of the device list. > > I tried that but ran into problems. The xarray model is clear enough, > but there is a (non-atomic) enumerated state in each sdev (struct > scsi_device object (pointer)) that is protected by a mutex. > I was unable to escape those pesky (but very useful) warnings out of > the depths of xarray that the locking was awry. When I've burrowed > into xarray.c I have usually found that it was correct. So now I regard > it as a pesky feature :-) Fascinating. So much of SCSI has to be IRQ-safe, and yet you can't look at the sdev state outside process context ... > > This patch seems decent enough ... I just think the decision to optimise > > the layout for LUNs < 256 is a bit odd. But hey, your subsystem, > > not mine. > > Hannes has the most experience in that area. He has only been using xarray > for a week or so (I think). There is something important missing from his > patchset: any xa_destroy() calls :-) They're not necessarily needed. If the XArray is empty, there's nothing to do.
On 5/28/20 8:54 PM, Matthew Wilcox wrote: > On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: >> On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: >>> Use xarray for device lookup by target. LUNs below 256 are linear, >>> and can be used directly as the index into the xarray. >>> LUNs above 256 have a distinct LUN format, and are not necessarily >>> linear. They'll be stored in indices above 256 in the xarray, with >>> the next free index in the xarray. > > I don't understand why you think this is an improvement over just > using an allocating XArray for all LUNs. It seems like more code, > and doesn't actually save you any storage space ... ? > The LUN range is 64 bit. I was under the impression that xarray can only handle up to unsigned long; which probably would work for 64bit systems, but there _are_ users running on 32 bit, and they get patently unhappy when we have to tell them 'sorry, not for you'. At least I _know_ that I'll get complaints if I try a stunt like this. If you can make xarray 64 bit on all systems, sure, I'm game. Cheers, Hannes
On Thu, May 28, 2020 at 10:58:31PM +0200, Hannes Reinecke wrote: > On 5/28/20 8:54 PM, Matthew Wilcox wrote: > > On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: > > > On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: > > > > Use xarray for device lookup by target. LUNs below 256 are linear, > > > > and can be used directly as the index into the xarray. > > > > LUNs above 256 have a distinct LUN format, and are not necessarily > > > > linear. They'll be stored in indices above 256 in the xarray, with > > > > the next free index in the xarray. > > > > I don't understand why you think this is an improvement over just > > using an allocating XArray for all LUNs. It seems like more code, > > and doesn't actually save you any storage space ... ? > > > The LUN range is 64 bit. > I was under the impression that xarray can only handle up to unsigned long; > which probably would work for 64bit systems, but there _are_ users running > on 32 bit, and they get patently unhappy when we have to tell them 'sorry, > not for you'. I meant just use xa_alloc() for everything instead of using xa_insert for 0-255.
On 5/28/20 9:44 PM, Douglas Gilbert wrote: > On 2020-05-28 2:54 p.m., Matthew Wilcox wrote: >> On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: >>> On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: >>>> Use xarray for device lookup by target. LUNs below 256 are linear, >>>> and can be used directly as the index into the xarray. >>>> LUNs above 256 have a distinct LUN format, and are not necessarily >>>> linear. They'll be stored in indices above 256 in the xarray, with >>>> the next free index in the xarray. >> >> I don't understand why you think this is an improvement over just >> using an allocating XArray for all LUNs. It seems like more code, >> and doesn't actually save you any storage space ... ? >> >>>> +++ b/drivers/scsi/scsi.c >>>> @@ -601,13 +601,14 @@ static struct scsi_target >>>> *__scsi_target_lookup(struct Scsi_Host *shost, >>>> void starget_for_each_device(struct scsi_target *starget, void >>>> *data, >>>> void (*fn)(struct scsi_device *, void *)) >>>> { >>>> - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); >>>> struct scsi_device *sdev; >>>> + unsigned long lun_id = 0; >>>> - shost_for_each_device(sdev, shost) { >>>> - if ((sdev->channel == starget->channel) && >>>> - (sdev->id == starget->id)) >>>> - fn(sdev, data); >>>> + xa_for_each(&starget->devices, lun_id, sdev) { >>>> + if (scsi_device_get(sdev)) >>>> + continue; >>>> + fn(sdev, data); >>>> + scsi_device_put(sdev); >>>> } >>>> } >> >> I appreciate you're implementing the API that's currently in use, but >> I would recommend open-coding this. Looking at the functions called, >> lots of them >> are two-three liners, eg: >> >> static void __scsi_report_device_reset(struct scsi_device *sdev, void >> *data) >> { >> sdev->was_reset = 1; >> sdev->expecting_cc_ua = 1; >> } >> >> which would just be more readable: >> >> if (rtn == SUCCESS) { >> struct scsi_target *starget = scsi_target(scmd->device); >> struct scsi_device *sdev; >> unsigned long index; >> >> spin_lock_irqsave(host->host_lock, flags); >> xa_for_each(&starget->devices, index, sdev) { >> sdev->was_reset = 1; >> sdev->expecting_cc_ua = 1; >> } >> spin_unlock_irqrestore(host->host_lock, flags); >> } >> >> And then maybe you could make a convincing argument that the spin_lock >> there could be turned into an xa_lock since that will prevent sdevs from >> coming & going during the iteration of the device list. > > I tried that but ran into problems. The xarray model is clear enough, > but there is a (non-atomic) enumerated state in each sdev (struct > scsi_device object (pointer)) that is protected by a mutex. > I was unable to escape those pesky (but very useful) warnings out of > the depths of xarray that the locking was awry. When I've burrowed > into xarray.c I have usually found that it was correct. So now I regard > it as a pesky feature :-) > > That said, in my work on the sg driver rewrite, I got rid of all > the explicit spinlocks in favour of xa_locks and that works fine, IMO. > The locking in the SCSI ML is a more complex beast. Maybe it doesn't > need to be ... > This whole code will need to be updated once we decided to move to xarrays. At this time I'm just trying to get the minimal patches in; cleanups can (and will) follow later. > > Hannes, why not make sdev_state an atomic and get rid of that mutex? > > Virtually none of this code is on the command fastpath, but Ming Lei > found that trimming down the size and position of members in sdev > could lead to performance improvements. > > These changes may improve the time taken to stabilize a system with large > disk arrays attached at startup, shutdown and when there are disruptions > or errors. > Virtually none is not identical to none. And locking is an audit unto itself, so I would _not_ attempt to revise the locking model at the same time when changing the lookup. Plan is to first change the lookup to xarray, and _then_ audit the locking such than we can move the xa_lock() in appropriate places. >>>> @@ -1621,7 +1623,18 @@ void scsi_sysfs_device_initialize(struct >>>> scsi_device *sdev) >>>> transport_setup_device(&sdev->sdev_gendev); >>>> spin_lock_irqsave(shost->host_lock, flags); >>>> - list_add_tail(&sdev->same_target_siblings, &starget->devices); >>>> + if (sdev->lun < 256) { >>>> + sdev->lun_idx = sdev->lun; >>>> + WARN_ON(xa_insert(&starget->devices, sdev->lun_idx, >>>> + sdev, GFP_KERNEL) < 0); >>> >>> You have interrupts masked again, so GFP_KERNEL is a non-no. >> >> Actually, I would say this is a great place to not use the host_lock. >> >> + int err; >> ... >> + if (sdev->lun < 256) { >> + sdev->lun_idx = sdev->lun; >> + err = xa_insert_irq(&starget->devices, sdev->lun_idx, sdev, >> + GFP_KERNEL); >> + } else { >> + err = xa_alloc_irq(&starget->devices, &sdev->lun_idx, sdev, >> + scsi_lun_limit, GFP_KERNEL); >> + } >> + ... maybe you want to convert scsi_sysfs_device_initialize() >> + to return an errno, although honestly this should never fail ... >> spin_lock_irqsave(shost->host_lock, flags); >> - list_add_tail(&sdev->same_target_siblings, &starget->devices); >> list_add_tail(&sdev->siblings, &shost->__devices); >> spin_unlock_irqrestore(shost->host_lock, flags); >> >> See above. Yes, it is worth revisiting. But in a next step, not now. >>>> + unsigned int lun_idx; /* Index into target device xarray */ >>> >>> Xarray gives you _two_ choices for the type of an index :-) >>> It is either u32 (as used in xa_alloc() and its variants) _or_ >>> unsigned long (as used everywhere else in xarray where an index >>> is needed). So it's definitely 32 bits for xa_alloc() and either >>> 32 or 64 bits for the rest of xarray depending on the machine >>> architecture. >>> Matthew Wilcox may care to explain why this is ... >> >> Saving space in data structures, mostly. It's not really useful to >> have 4 billion things in an allocating XArray. Indeed, it's not really >> useful to have 4 billion of almost anything. So we have XArrays which >> are indexed by something sensible like page index in file, and they're >> nice and well-behaved (most people don't create files in excess of 16TB, >> and those who do don't expect amazing performance when seeking around >> in them at random because they've lost all caching effects). And then >> you have people who probably want one scsi device. Maybe a dozen. >> Possibly a thousand. Definitely not a million. If you get 4 billion >> scsi >> devices, something's gone very wrong. You've run out of drive letters, >> for a start (/dev/sdmzabcd anybody?) > > no_uld=1 # RAID cards might do this on its physicals so > smartmontools > # can check them > > Then you only have to worry about /dev/sg<n> and /dev/bsg/<hctl) numbers > or tuples going through the roof :-) > >> It doesn't cost us anything to just use unsigned long as the index of an >> XArray, due to how it's constructed. Except in this one place where we >> need to point to somewhere to store the index so that we can update the >> index within an sdev before anybody iterating the data structure under >> RCU can find the uninitialised index. >> >> This patch seems decent enough ... I just think the decision to optimise >> the layout for LUNs < 256 is a bit odd. But hey, your subsystem, >> not mine. > > Hannes has the most experience in that area. He has only been using xarray > for a week or so (I think). There is something important missing from his > patchset: any xa_destroy() calls :-) > Yes! I know! Working on it. Cheers, Hannes
On 5/29/20 2:20 AM, Matthew Wilcox wrote: > On Thu, May 28, 2020 at 10:58:31PM +0200, Hannes Reinecke wrote: >> On 5/28/20 8:54 PM, Matthew Wilcox wrote: >>> On Thu, May 28, 2020 at 01:50:02PM -0400, Douglas Gilbert wrote: >>>> On 2020-05-28 12:36 p.m., Hannes Reinecke wrote: >>>>> Use xarray for device lookup by target. LUNs below 256 are linear, >>>>> and can be used directly as the index into the xarray. >>>>> LUNs above 256 have a distinct LUN format, and are not necessarily >>>>> linear. They'll be stored in indices above 256 in the xarray, with >>>>> the next free index in the xarray. >>> >>> I don't understand why you think this is an improvement over just >>> using an allocating XArray for all LUNs. It seems like more code, >>> and doesn't actually save you any storage space ... ? >>> >> The LUN range is 64 bit. >> I was under the impression that xarray can only handle up to unsigned long; >> which probably would work for 64bit systems, but there _are_ users running >> on 32 bit, and they get patently unhappy when we have to tell them 'sorry, >> not for you'. > > I meant just use xa_alloc() for everything instead of using xa_insert for > 0-255. > But then I'll have to use xa_find() to get to the actual element as the 1:1 mapping between SCSI LUN and array index is lost. And seeing that most storage arrays will expose only up to 256 LUNs I thought this was a good improvement on lookup. Of course, this only makes sense if xa_load() is more efficient than xa_find(). If not then of course it's a bit futile. Cheers, Hannes
On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote: > > I meant just use xa_alloc() for everything instead of using xa_insert for > > 0-255. > > > But then I'll have to use xa_find() to get to the actual element as the 1:1 > mapping between SCSI LUN and array index is lost. > And seeing that most storage arrays will expose only up to 256 LUNs I > thought this was a good improvement on lookup. > Of course, this only makes sense if xa_load() is more efficient than > xa_find(). If not then of course it's a bit futile. xa_load() is absolutely more efficient than xa_find(). It's just a question of whether it matters ;-) Carry on ...
On 5/29/20 1:21 PM, Matthew Wilcox wrote: > On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote: >>> I meant just use xa_alloc() for everything instead of using xa_insert for >>> 0-255. >>> >> But then I'll have to use xa_find() to get to the actual element as the 1:1 >> mapping between SCSI LUN and array index is lost. >> And seeing that most storage arrays will expose only up to 256 LUNs I >> thought this was a good improvement on lookup. >> Of course, this only makes sense if xa_load() is more efficient than >> xa_find(). If not then of course it's a bit futile. > > xa_load() is absolutely more efficient than xa_find(). It's just a > question of whether it matters ;-) Carry on ... > Thanks. I will. BTW, care to update the documentation? * Return: 0 on success, -ENOMEM if memory could not be allocated or * -EBUSY if there are no free entries in @limit. */ int __xa_alloc(struct xarray *xa, u32 *id, void *entry, struct xa_limit limit, gfp_t gfp) { XA_STATE(xas, xa, 0); if (WARN_ON_ONCE(xa_is_advanced(entry))) return -EINVAL; if (WARN_ON_ONCE(!xa_track_free(xa))) return -EINVAL; So looks as if the documentation is in need of updating to cover -EINVAL. And, please, state somewhere that whenever you want to use xa_alloc() you _need_ to specify XA_ALLOC_FLAGS in xa_init_flags(), otherwise you trip across the above. It's not entirely obvious, and took me the better half of a day to figure out. Cheers, Hannes
On Fri, May 29, 2020 at 02:46:48PM +0200, Hannes Reinecke wrote: > On 5/29/20 1:21 PM, Matthew Wilcox wrote: > > On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote: > > > > I meant just use xa_alloc() for everything instead of using xa_insert for > > > > 0-255. > > > > > > > But then I'll have to use xa_find() to get to the actual element as the 1:1 > > > mapping between SCSI LUN and array index is lost. > > > And seeing that most storage arrays will expose only up to 256 LUNs I > > > thought this was a good improvement on lookup. > > > Of course, this only makes sense if xa_load() is more efficient than > > > xa_find(). If not then of course it's a bit futile. > > > > xa_load() is absolutely more efficient than xa_find(). It's just a > > question of whether it matters ;-) Carry on ... > > > Thanks. I will. > > BTW, care to update the documentation? > > * Return: 0 on success, -ENOMEM if memory could not be allocated or > * -EBUSY if there are no free entries in @limit. > */ > int __xa_alloc(struct xarray *xa, u32 *id, void *entry, > struct xa_limit limit, gfp_t gfp) > { > XA_STATE(xas, xa, 0); > > if (WARN_ON_ONCE(xa_is_advanced(entry))) > return -EINVAL; > if (WARN_ON_ONCE(!xa_track_free(xa))) > return -EINVAL; > > So looks as if the documentation is in need of updating to cover -EINVAL. > And, please, state somewhere that whenever you want to use xa_alloc() you > _need_ to specify XA_ALLOC_FLAGS in xa_init_flags(), otherwise > you trip across the above. Like this? Allocating XArrays ------------------ If you use DEFINE_XARRAY_ALLOC() to define the XArray, or initialise it by passing ``XA_FLAGS_ALLOC`` to xa_init_flags(), the XArray changes to track whether entries are in use or not. You can call xa_alloc() to store the entry at an unused index in the XArray. If you need to modify the array from interrupt context, you can use xa_alloc_bh() or xa_alloc_irq() to disable interrupts while allocating the ID. > It's not entirely obvious, and took me the better half of a day to figure > out. Really? You get a nice WARN_ON and backtrace so you can see where you went wrong ... What could I change to make this easier?
On 5/29/20 2:50 PM, Matthew Wilcox wrote: > On Fri, May 29, 2020 at 02:46:48PM +0200, Hannes Reinecke wrote: >> On 5/29/20 1:21 PM, Matthew Wilcox wrote: >>> On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote: >>>>> I meant just use xa_alloc() for everything instead of using xa_insert for >>>>> 0-255. >>>>> >>>> But then I'll have to use xa_find() to get to the actual element as the 1:1 >>>> mapping between SCSI LUN and array index is lost. >>>> And seeing that most storage arrays will expose only up to 256 LUNs I >>>> thought this was a good improvement on lookup. >>>> Of course, this only makes sense if xa_load() is more efficient than >>>> xa_find(). If not then of course it's a bit futile. >>> >>> xa_load() is absolutely more efficient than xa_find(). It's just a >>> question of whether it matters ;-) Carry on ... >>> >> Thanks. I will. >> >> BTW, care to update the documentation? >> >> * Return: 0 on success, -ENOMEM if memory could not be allocated or >> * -EBUSY if there are no free entries in @limit. >> */ >> int __xa_alloc(struct xarray *xa, u32 *id, void *entry, >> struct xa_limit limit, gfp_t gfp) >> { >> XA_STATE(xas, xa, 0); >> >> if (WARN_ON_ONCE(xa_is_advanced(entry))) >> return -EINVAL; >> if (WARN_ON_ONCE(!xa_track_free(xa))) >> return -EINVAL; >> >> So looks as if the documentation is in need of updating to cover -EINVAL. >> And, please, state somewhere that whenever you want to use xa_alloc() you >> _need_ to specify XA_ALLOC_FLAGS in xa_init_flags(), otherwise >> you trip across the above. > > Like this? > > Allocating XArrays > ------------------ > > If you use DEFINE_XARRAY_ALLOC() to define the XArray, or > initialise it by passing ``XA_FLAGS_ALLOC`` to xa_init_flags(), > the XArray changes to track whether entries are in use or not. > > You can call xa_alloc() to store the entry at an unused index > in the XArray. If you need to modify the array from interrupt context, > you can use xa_alloc_bh() or xa_alloc_irq() to disable > interrupts while allocating the ID. > > What I'm missing is the connection between the first paragraph and the second. It starts with 'If you use ...', making no indication what would happen if you don't. And the second paragraph just says '... to store the entry ...'; is never said anything about tracking entries. Why not simply append this sentenct to the second paragraph: In order to use xa_alloc() you need to pass the XA_FLAGS_ALLOC when calling xa_init_flags()l >> It's not entirely obvious, and took me the better half of a day to figure >> out. > > Really? You get a nice WARN_ON and backtrace so you can see where > you went wrong ... What could I change to make this easier? > It _could_ have said 'You forgot to pass XA_ALLOC_FLAGS in xa_init_flags()'. Then it would've been immediately obvious without having to delve into xarray code. Cheers, Hannes
On 2020-05-29 8:46 a.m., Hannes Reinecke wrote: > On 5/29/20 1:21 PM, Matthew Wilcox wrote: >> On Fri, May 29, 2020 at 08:50:21AM +0200, Hannes Reinecke wrote: >>>> I meant just use xa_alloc() for everything instead of using xa_insert for >>>> 0-255. >>>> >>> But then I'll have to use xa_find() to get to the actual element as the 1:1 >>> mapping between SCSI LUN and array index is lost. >>> And seeing that most storage arrays will expose only up to 256 LUNs I >>> thought this was a good improvement on lookup. >>> Of course, this only makes sense if xa_load() is more efficient than >>> xa_find(). If not then of course it's a bit futile. >> >> xa_load() is absolutely more efficient than xa_find(). It's just a >> question of whether it matters ;-) Carry on ... >> > Thanks. I will. > > BTW, care to update the documentation? > > * Return: 0 on success, -ENOMEM if memory could not be allocated or > * -EBUSY if there are no free entries in @limit. > */ > int __xa_alloc(struct xarray *xa, u32 *id, void *entry, > struct xa_limit limit, gfp_t gfp) > { > XA_STATE(xas, xa, 0); > > if (WARN_ON_ONCE(xa_is_advanced(entry))) > return -EINVAL; > if (WARN_ON_ONCE(!xa_track_free(xa))) > return -EINVAL; > > So looks as if the documentation is in need of updating to cover -EINVAL. > And, please, state somewhere that whenever you want to use xa_alloc() you _need_ > to specify XA_ALLOC_FLAGS in xa_init_flags(), otherwise > you trip across the above. > > It's not entirely obvious, and took me the better half of a day to figure out. Ditto for me. At the time I did relay my frustration to Matthew who did address it in the documentation. Another one is that xa_find*() requires the XA_PRESENT mark, even when you are not using marks (or haven't yet learnt about them ...). A clearer demarcation of the two xarray modes (i.e. either the user supplies the index, or xarray does) would be helpful. That mode impacts which parts of the xarray interface are used, for example in the sg driver rewrite, xarrays are used for all collections but if memory serves, there isn't a single xa_load() or xa_store() call. But writing technical documentation is difficult. Very few third parties step up to help, leaving the designer/implementer to do it. And it is extremely difficult for someone who knows the code backwards (and where the skeletons are buried) to stand on their heads and present the information in a pedagogic manner. Also traditional code documentation uses 7 bit ASCII text and "ACSII art" in a sort of nod to earlier generations of programmers. Surely more modern techniques, including colour diagrams and even animations, should now be encouraged. Maybe when compilers start accepting html :-) Doug Gilbert P.S. I have tried to practice what I preach: http://sg.danny.cz/sg/sg_v40.html
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index d601424e32b2..9dbbc51a1eb5 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, void starget_for_each_device(struct scsi_target *starget, void *data, void (*fn)(struct scsi_device *, void *)) { - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); struct scsi_device *sdev; + unsigned long lun_id = 0; - shost_for_each_device(sdev, shost) { - if ((sdev->channel == starget->channel) && - (sdev->id == starget->id)) - fn(sdev, data); + xa_for_each(&starget->devices, lun_id, sdev) { + if (scsi_device_get(sdev)) + continue; + fn(sdev, data); + scsi_device_put(sdev); } } EXPORT_SYMBOL(starget_for_each_device); @@ -629,14 +630,11 @@ EXPORT_SYMBOL(starget_for_each_device); void __starget_for_each_device(struct scsi_target *starget, void *data, void (*fn)(struct scsi_device *, void *)) { - struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); struct scsi_device *sdev; + unsigned long lun_id = 0; - __shost_for_each_device(sdev, shost) { - if ((sdev->channel == starget->channel) && - (sdev->id == starget->id)) - fn(sdev, data); - } + xa_for_each(&starget->devices, lun_id, sdev) + fn(sdev, data); } EXPORT_SYMBOL(__starget_for_each_device); @@ -659,11 +657,19 @@ struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget, u64 lun) { struct scsi_device *sdev; + unsigned long lun_idx = 256; + + if (lun < lun_idx) { + sdev = xa_load(&starget->devices, lun); + if (sdev && sdev->sdev_state == SDEV_DEL) + sdev = NULL; + return sdev; + } - list_for_each_entry(sdev, &starget->devices, same_target_siblings) { + xa_for_each(&starget->devices, lun_idx, sdev) { if (sdev->sdev_state == SDEV_DEL) continue; - if (sdev->lun ==lun) + if (sdev->lun == lun) return sdev; } diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index c163fa22267c..f9829fc0d84e 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -361,9 +361,9 @@ static void scsi_kick_queue(struct request_queue *q) static void scsi_single_lun_run(struct scsi_device *current_sdev) { struct Scsi_Host *shost = current_sdev->host; - struct scsi_device *sdev, *tmp; + struct scsi_device *sdev; struct scsi_target *starget = scsi_target(current_sdev); - unsigned long flags; + unsigned long flags, lun_id = 0; spin_lock_irqsave(shost->host_lock, flags); starget->starget_sdev_user = NULL; @@ -380,8 +380,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) spin_lock_irqsave(shost->host_lock, flags); if (starget->starget_sdev_user) goto out; - list_for_each_entry_safe(sdev, tmp, &starget->devices, - same_target_siblings) { + xa_for_each(&starget->devices, lun_id, sdev) { if (sdev == current_sdev) continue; if (scsi_device_get(sdev)) @@ -390,7 +389,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev) spin_unlock_irqrestore(shost->host_lock, flags); scsi_kick_queue(sdev->request_queue); spin_lock_irqsave(shost->host_lock, flags); - + scsi_device_put(sdev); } out: diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index dc2656df495b..c7aba9ba5c0c 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -235,7 +235,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, mutex_init(&sdev->state_mutex); sdev->sdev_state = SDEV_CREATED; INIT_LIST_HEAD(&sdev->siblings); - INIT_LIST_HEAD(&sdev->same_target_siblings); INIT_LIST_HEAD(&sdev->starved_entry); INIT_LIST_HEAD(&sdev->event_list); spin_lock_init(&sdev->list_lock); @@ -417,7 +416,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, starget->id = id; starget->channel = channel; starget->can_queue = 0; - INIT_LIST_HEAD(&starget->devices); + xa_init(&starget->devices); starget->state = STARGET_CREATED; starget->scsi_level = SCSI_2; starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED; diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 95aaa96ce03b..27c19232f175 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -434,6 +434,7 @@ static void scsi_device_cls_release(struct device *class_dev) static void scsi_device_dev_release_usercontext(struct work_struct *work) { struct scsi_device *sdev; + struct scsi_target *starget; struct device *parent; struct list_head *this, *tmp; struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL; @@ -441,6 +442,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) unsigned long flags; sdev = container_of(work, struct scsi_device, ew.work); + starget = sdev->sdev_target; scsi_dh_release_device(sdev); @@ -448,7 +450,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work) spin_lock_irqsave(sdev->host->host_lock, flags); list_del(&sdev->siblings); - list_del(&sdev->same_target_siblings); + xa_erase(&starget->devices, sdev->lun_idx); list_del(&sdev->starved_entry); spin_unlock_irqrestore(sdev->host->host_lock, flags); @@ -1621,7 +1623,18 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) transport_setup_device(&sdev->sdev_gendev); spin_lock_irqsave(shost->host_lock, flags); - list_add_tail(&sdev->same_target_siblings, &starget->devices); + if (sdev->lun < 256) { + sdev->lun_idx = sdev->lun; + WARN_ON(xa_insert(&starget->devices, sdev->lun_idx, + sdev, GFP_KERNEL) < 0); + } else { + struct xa_limit scsi_lun_limit = { + .min = 256, + .max = UINT_MAX, + }; + WARN_ON(xa_alloc(&starget->devices, &sdev->lun_idx, + sdev, scsi_lun_limit, GFP_KERNEL) < 0); + } list_add_tail(&sdev->siblings, &shost->__devices); spin_unlock_irqrestore(shost->host_lock, flags); /* diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 28034cc0fce5..2c6b9d8bc40e 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -104,7 +104,6 @@ struct scsi_device { /* the next two are protected by the host->host_lock */ struct list_head siblings; /* list of all devices on this host */ - struct list_head same_target_siblings; /* just the devices sharing same target id */ atomic_t device_busy; /* commands actually active on LLDD */ atomic_t device_blocked; /* Device returned QUEUE_FULL. */ @@ -123,6 +122,7 @@ struct scsi_device { unsigned int id, channel; u64 lun; + unsigned int lun_idx; /* Index into target device xarray */ unsigned int manufacturer; /* Manufacturer of device, for using * vendor-specific cmd's */ unsigned sector_size; /* size in bytes */ @@ -285,7 +285,7 @@ enum scsi_target_state { struct scsi_target { struct scsi_device *starget_sdev_user; struct list_head siblings; - struct list_head devices; + struct xarray devices; struct device dev; struct kref reap_ref; /* last put renders target invisible */ unsigned int channel;