| Message ID | 20200527141400.58087-2-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: use xarray for devices and targets | expand |
On 27/05/2020 16:14, Hannes Reinecke wrote: [...] > @@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target); > struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget, > u64 lun) > { > - struct scsi_device *sdev; > struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); > + struct scsi_device *sdev; > unsigned long flags; This looks unrelated. > > spin_lock_irqsave(shost->host_lock, flags); > @@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target); > * really want to use scsi_device_lookup instead. > **/ > struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost, > - uint channel, uint id, u64 lun) > + u16 channel, u16 id, u64 lun) > { > + struct scsi_target *starget; > struct scsi_device *sdev; > > - list_for_each_entry(sdev, &shost->__devices, siblings) { > - if (sdev->sdev_state == SDEV_DEL) > - continue; > - if (sdev->channel == channel && sdev->id == id && > - sdev->lun ==lun) > - return sdev; > - } > + starget = __scsi_target_lookup(shost, channel, id); > + if (!starget) > + return NULL; > + sdev = __scsi_device_lookup_by_target(starget, lun); > + if (sdev && sdev->sdev_state == SDEV_DEL) > + sdev = NULL; > I think the above if is unneeded as __scsi_device_lookup_by_target() does: list_for_each_entry(sdev, &starget->devices, same_target_siblings) { if (sdev->sdev_state == SDEV_DEL) continue; So 'sdev != NULL && sdev->sdev_state == SDEV_DEL' would never evaluate to true, wouldn't it?
On 5/27/20 4:57 PM, Johannes Thumshirn wrote: > On 27/05/2020 16:14, Hannes Reinecke wrote: > [...] >> @@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target); >> struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget, >> u64 lun) >> { >> - struct scsi_device *sdev; >> struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); >> + struct scsi_device *sdev; >> unsigned long flags; > > This looks unrelated. > >> >> spin_lock_irqsave(shost->host_lock, flags); >> @@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target); >> * really want to use scsi_device_lookup instead. >> **/ >> struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost, >> - uint channel, uint id, u64 lun) >> + u16 channel, u16 id, u64 lun) >> { >> + struct scsi_target *starget; >> struct scsi_device *sdev; >> >> - list_for_each_entry(sdev, &shost->__devices, siblings) { >> - if (sdev->sdev_state == SDEV_DEL) >> - continue; >> - if (sdev->channel == channel && sdev->id == id && >> - sdev->lun ==lun) >> - return sdev; >> - } >> + starget = __scsi_target_lookup(shost, channel, id); >> + if (!starget) >> + return NULL; >> + sdev = __scsi_device_lookup_by_target(starget, lun); >> + if (sdev && sdev->sdev_state == SDEV_DEL) >> + sdev = NULL; >> > > I think the above if is unneeded as __scsi_device_lookup_by_target() does: > list_for_each_entry(sdev, &starget->devices, same_target_siblings) { > if (sdev->sdev_state == SDEV_DEL) > continue; > > So 'sdev != NULL && sdev->sdev_state == SDEV_DEL' would never evaluate to true, > wouldn't it? > Oh. indeed. So that can be simplified. Will do for the next version. Cheers, Hannes
On Wed, May 27, 2020 at 04:13:57PM +0200, Hannes Reinecke wrote: > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev) > { > struct Scsi_Host *shost = dev_to_shost(dev->parent); > struct scsi_target *starget; > + unsigned long tid = 0; > unsigned long flags; > > -restart: > spin_lock_irqsave(shost->host_lock, flags); > - list_for_each_entry(starget, &shost->__targets, siblings) { > + starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT); > + while (starget) { > if (starget->state == STARGET_DEL || > starget->state == STARGET_REMOVE || > - starget->state == STARGET_CREATED_REMOVE) > + starget->state == STARGET_CREATED_REMOVE) { > + starget = xa_find_after(&shost->__targets, &tid, > + ULONG_MAX, XA_PRESENT); > continue; > + } > if (starget->dev.parent == dev || &starget->dev == dev) { > kref_get(&starget->reap_ref); > if (starget->state == STARGET_CREATED) > @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev) > spin_unlock_irqrestore(shost->host_lock, flags); > __scsi_remove_target(starget); > scsi_target_reap(starget); > - goto restart; > + spin_lock_irqsave(shost->host_lock, flags); > + starget = xa_find_after(&shost->__targets, &tid, > + ULONG_MAX, XA_PRESENT); > + continue; > } Is there a special reason why don't use xa_for_each to iterate through all entries?
On 5/28/20 9:24 AM, Daniel Wagner wrote: > On Wed, May 27, 2020 at 04:13:57PM +0200, Hannes Reinecke wrote: >> --- a/drivers/scsi/scsi_sysfs.c >> +++ b/drivers/scsi/scsi_sysfs.c >> @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev) >> { >> struct Scsi_Host *shost = dev_to_shost(dev->parent); >> struct scsi_target *starget; >> + unsigned long tid = 0; >> unsigned long flags; >> >> -restart: >> spin_lock_irqsave(shost->host_lock, flags); >> - list_for_each_entry(starget, &shost->__targets, siblings) { >> + starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT); >> + while (starget) { >> if (starget->state == STARGET_DEL || >> starget->state == STARGET_REMOVE || >> - starget->state == STARGET_CREATED_REMOVE) >> + starget->state == STARGET_CREATED_REMOVE) { >> + starget = xa_find_after(&shost->__targets, &tid, >> + ULONG_MAX, XA_PRESENT); >> continue; >> + } >> if (starget->dev.parent == dev || &starget->dev == dev) { >> kref_get(&starget->reap_ref); >> if (starget->state == STARGET_CREATED) >> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev) >> spin_unlock_irqrestore(shost->host_lock, flags); >> __scsi_remove_target(starget); >> scsi_target_reap(starget); >> - goto restart; >> + spin_lock_irqsave(shost->host_lock, flags); >> + starget = xa_find_after(&shost->__targets, &tid, >> + ULONG_MAX, XA_PRESENT); >> + continue; >> } > > Is there a special reason why don't use xa_for_each to iterate through all > entries? > This entire function is completely daft. It says 'scsi_remove_target()', but for some weird reason fails to pass in the target it want to delete, so it has to go round in circles trying to figure out which target to delete. There probably had been an obscure reason for this, but with the current code it's just pointless. So that's the next thing to fix (after all of this): use a struct scsi_target as argument for this function, then this entire loop can go. Cheers, Hannes
On 2020-05-28 00:52, Hannes Reinecke wrote: > On 5/28/20 9:24 AM, Daniel Wagner wrote: >> Is there a special reason why don't use xa_for_each to iterate through >> all >> entries? >> > This entire function is completely daft. > It says 'scsi_remove_target()', but for some weird reason fails to pass > in the target it want to delete, so it has to go round in circles trying > to figure out which target to delete. > There probably had been an obscure reason for this, but with the current > code it's just pointless. > So that's the next thing to fix (after all of this): use a struct > scsi_target as argument for this function, then this entire loop can go. Please be careful when modifying this code. I remember that it took multiple iterations to get this code right. See e.g. commit 81b6c9998979 ("scsi: core: check for device state in __scsi_remove_target()"). See also commit fbce4d97fd43 ("scsi: fixup kernel warning during rmmod()"). See also commit f9279c968c25 ("scsi: Add STARGET_CREATED_REMOVE state to scsi_target_state"). The only reason I can think of why that loop exists is that @dev may be associated with multiple targets (struct scsi_target) at the same time. Is that something that can happen? Thanks, Bart.
Hi Hannes,
I love your patch! Perhaps something to improve:
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
config: i386-randconfig-s001-20200528 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-240-gf0fe1cd9-dirty
# save the attached .config to linux build tree
make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/scsi/scsi_scan.c:392:27: sparse: sparse: context imbalance in 'scsi_alloc_target' - different lock contexts for basic block
# https://github.com/0day-ci/linux/commit/45b149b239ea9a86968ddbd8ecda1e6c44937b68
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 45b149b239ea9a86968ddbd8ecda1e6c44937b68
vim +/scsi_alloc_target +392 drivers/scsi/scsi_scan.c
e63ed0d7a98014 James Bottomley 2014-01-21 379
884d25cc4fda20 James Bottomley 2006-09-05 380 /**
884d25cc4fda20 James Bottomley 2006-09-05 381 * scsi_alloc_target - allocate a new or find an existing target
884d25cc4fda20 James Bottomley 2006-09-05 382 * @parent: parent of the target (need not be a scsi host)
884d25cc4fda20 James Bottomley 2006-09-05 383 * @channel: target channel number (zero if no channels)
884d25cc4fda20 James Bottomley 2006-09-05 384 * @id: target id number
884d25cc4fda20 James Bottomley 2006-09-05 385 *
884d25cc4fda20 James Bottomley 2006-09-05 386 * Return an existing target if one exists, provided it hasn't already
884d25cc4fda20 James Bottomley 2006-09-05 387 * gone into STARGET_DEL state, otherwise allocate a new target.
884d25cc4fda20 James Bottomley 2006-09-05 388 *
884d25cc4fda20 James Bottomley 2006-09-05 389 * The target is returned with an incremented reference, so the caller
884d25cc4fda20 James Bottomley 2006-09-05 390 * is responsible for both reaping and doing a last put
884d25cc4fda20 James Bottomley 2006-09-05 391 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 @392 static struct scsi_target *scsi_alloc_target(struct device *parent,
^1da177e4c3f41 Linus Torvalds 2005-04-16 393 int channel, uint id)
^1da177e4c3f41 Linus Torvalds 2005-04-16 394 {
^1da177e4c3f41 Linus Torvalds 2005-04-16 395 struct Scsi_Host *shost = dev_to_shost(parent);
^1da177e4c3f41 Linus Torvalds 2005-04-16 396 struct device *dev = NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 397 unsigned long flags;
^1da177e4c3f41 Linus Torvalds 2005-04-16 398 const int size = sizeof(struct scsi_target)
^1da177e4c3f41 Linus Torvalds 2005-04-16 399 + shost->transportt->target_size;
5c44cd2afad3f7 James.Smart@Emulex.Com 2005-06-10 400 struct scsi_target *starget;
^1da177e4c3f41 Linus Torvalds 2005-04-16 401 struct scsi_target *found_target;
e63ed0d7a98014 James Bottomley 2014-01-21 402 int error, ref_got;
45b149b239ea9a Hannes Reinecke 2020-05-27 403 unsigned long tid;
^1da177e4c3f41 Linus Torvalds 2005-04-16 404
24669f75a3231f Jes Sorensen 2006-01-16 405 starget = kzalloc(size, GFP_KERNEL);
^1da177e4c3f41 Linus Torvalds 2005-04-16 406 if (!starget) {
cadbd4a5e36dde Harvey Harrison 2008-07-03 407 printk(KERN_ERR "%s: allocation failure\n", __func__);
^1da177e4c3f41 Linus Torvalds 2005-04-16 408 return NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 409 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 410 dev = &starget->dev;
^1da177e4c3f41 Linus Torvalds 2005-04-16 411 device_initialize(dev);
e63ed0d7a98014 James Bottomley 2014-01-21 412 kref_init(&starget->reap_ref);
^1da177e4c3f41 Linus Torvalds 2005-04-16 413 dev->parent = get_device(parent);
71610f55fa4db6 Kay Sievers 2008-12-03 414 dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
b0ed43360fdca2 Hannes Reinecke 2008-03-18 415 dev->bus = &scsi_bus_type;
b0ed43360fdca2 Hannes Reinecke 2008-03-18 416 dev->type = &scsi_target_type;
^1da177e4c3f41 Linus Torvalds 2005-04-16 417 starget->id = id;
^1da177e4c3f41 Linus Torvalds 2005-04-16 418 starget->channel = channel;
f0c0a376d0fcd4 Mike Christie 2008-08-17 419 starget->can_queue = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 420 INIT_LIST_HEAD(&starget->devices);
643eb2d932c97a James Bottomley 2008-03-22 421 starget->state = STARGET_CREATED;
7c9d6f16f50d3a Alan Stern 2007-01-08 422 starget->scsi_level = SCSI_2;
c53a284f8be237 Edward Goggin 2009-04-09 423 starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
45b149b239ea9a Hannes Reinecke 2020-05-27 424 tid = scsi_target_index(starget);
ffedb4522571ac James Bottomley 2006-02-23 425 retry:
^1da177e4c3f41 Linus Torvalds 2005-04-16 426 spin_lock_irqsave(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds 2005-04-16 427
45b149b239ea9a Hannes Reinecke 2020-05-27 428 found_target = xa_load(&shost->__targets, tid);
^1da177e4c3f41 Linus Torvalds 2005-04-16 429 if (found_target)
^1da177e4c3f41 Linus Torvalds 2005-04-16 430 goto found;
45b149b239ea9a Hannes Reinecke 2020-05-27 431 if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
45b149b239ea9a Hannes Reinecke 2020-05-27 432 dev_printk(KERN_ERR, dev, "target index busy\n");
45b149b239ea9a Hannes Reinecke 2020-05-27 433 kfree(starget);
45b149b239ea9a Hannes Reinecke 2020-05-27 434 return NULL;
45b149b239ea9a Hannes Reinecke 2020-05-27 435 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 436 spin_unlock_irqrestore(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds 2005-04-16 437 /* allocate and add */
a283bd37d00e92 James Bottomley 2005-05-24 438 transport_setup_device(dev);
a283bd37d00e92 James Bottomley 2005-05-24 439 if (shost->hostt->target_alloc) {
32f95792500794 Brian King 2006-02-22 440 error = shost->hostt->target_alloc(starget);
a283bd37d00e92 James Bottomley 2005-05-24 441
a283bd37d00e92 James Bottomley 2005-05-24 442 if(error) {
a283bd37d00e92 James Bottomley 2005-05-24 443 dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
a283bd37d00e92 James Bottomley 2005-05-24 444 /* don't want scsi_target_reap to do the final
a283bd37d00e92 James Bottomley 2005-05-24 445 * put because it will be under the host lock */
643eb2d932c97a James Bottomley 2008-03-22 446 scsi_target_destroy(starget);
a283bd37d00e92 James Bottomley 2005-05-24 447 return NULL;
a283bd37d00e92 James Bottomley 2005-05-24 448 }
a283bd37d00e92 James Bottomley 2005-05-24 449 }
884d25cc4fda20 James Bottomley 2006-09-05 450 get_device(dev);
a283bd37d00e92 James Bottomley 2005-05-24 451
^1da177e4c3f41 Linus Torvalds 2005-04-16 452 return starget;
^1da177e4c3f41 Linus Torvalds 2005-04-16 453
^1da177e4c3f41 Linus Torvalds 2005-04-16 454 found:
e63ed0d7a98014 James Bottomley 2014-01-21 455 /*
e63ed0d7a98014 James Bottomley 2014-01-21 456 * release routine already fired if kref is zero, so if we can still
e63ed0d7a98014 James Bottomley 2014-01-21 457 * take the reference, the target must be alive. If we can't, it must
e63ed0d7a98014 James Bottomley 2014-01-21 458 * be dying and we need to wait for a new target
e63ed0d7a98014 James Bottomley 2014-01-21 459 */
e63ed0d7a98014 James Bottomley 2014-01-21 460 ref_got = kref_get_unless_zero(&found_target->reap_ref);
e63ed0d7a98014 James Bottomley 2014-01-21 461
^1da177e4c3f41 Linus Torvalds 2005-04-16 462 spin_unlock_irqrestore(shost->host_lock, flags);
e63ed0d7a98014 James Bottomley 2014-01-21 463 if (ref_got) {
12fb8c1574d7d0 Alan Stern 2010-03-18 464 put_device(dev);
^1da177e4c3f41 Linus Torvalds 2005-04-16 465 return found_target;
^1da177e4c3f41 Linus Torvalds 2005-04-16 466 }
e63ed0d7a98014 James Bottomley 2014-01-21 467 /*
e63ed0d7a98014 James Bottomley 2014-01-21 468 * Unfortunately, we found a dying target; need to wait until it's
e63ed0d7a98014 James Bottomley 2014-01-21 469 * dead before we can get a new one. There is an anomaly here. We
e63ed0d7a98014 James Bottomley 2014-01-21 470 * *should* call scsi_target_reap() to balance the kref_get() of the
e63ed0d7a98014 James Bottomley 2014-01-21 471 * reap_ref above. However, since the target being released, it's
e63ed0d7a98014 James Bottomley 2014-01-21 472 * already invisible and the reap_ref is irrelevant. If we call
e63ed0d7a98014 James Bottomley 2014-01-21 473 * scsi_target_reap() we might spuriously do another device_del() on
e63ed0d7a98014 James Bottomley 2014-01-21 474 * an already invisible target.
e63ed0d7a98014 James Bottomley 2014-01-21 475 */
ffedb4522571ac James Bottomley 2006-02-23 476 put_device(&found_target->dev);
e63ed0d7a98014 James Bottomley 2014-01-21 477 /*
e63ed0d7a98014 James Bottomley 2014-01-21 478 * length of time is irrelevant here, we just want to yield the CPU
e63ed0d7a98014 James Bottomley 2014-01-21 479 * for a tick to avoid busy waiting for the target to die.
e63ed0d7a98014 James Bottomley 2014-01-21 480 */
e63ed0d7a98014 James Bottomley 2014-01-21 481 msleep(1);
ffedb4522571ac James Bottomley 2006-02-23 482 goto retry;
ffedb4522571ac James Bottomley 2006-02-23 483 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 484
:::::: The code at line 392 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2
:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Hannes,
[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-m001-20200529 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/scsi/scsi_scan.c:482 scsi_alloc_target() warn: inconsistent returns '*shost->host_lock'.
drivers/scsi/scsi_scan.c:482 scsi_alloc_target() warn: inconsistent returns 'flags'.
# https://github.com/0day-ci/linux/commit/45b149b239ea9a86968ddbd8ecda1e6c44937b68
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 45b149b239ea9a86968ddbd8ecda1e6c44937b68
vim +482 drivers/scsi/scsi_scan.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 392 static struct scsi_target *scsi_alloc_target(struct device *parent,
^1da177e4c3f41 Linus Torvalds 2005-04-16 393 int channel, uint id)
^1da177e4c3f41 Linus Torvalds 2005-04-16 394 {
^1da177e4c3f41 Linus Torvalds 2005-04-16 395 struct Scsi_Host *shost = dev_to_shost(parent);
^1da177e4c3f41 Linus Torvalds 2005-04-16 396 struct device *dev = NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 397 unsigned long flags;
^1da177e4c3f41 Linus Torvalds 2005-04-16 398 const int size = sizeof(struct scsi_target)
^1da177e4c3f41 Linus Torvalds 2005-04-16 399 + shost->transportt->target_size;
5c44cd2afad3f7 James.Smart@Emulex.Com 2005-06-10 400 struct scsi_target *starget;
^1da177e4c3f41 Linus Torvalds 2005-04-16 401 struct scsi_target *found_target;
e63ed0d7a98014 James Bottomley 2014-01-21 402 int error, ref_got;
45b149b239ea9a Hannes Reinecke 2020-05-27 403 unsigned long tid;
^1da177e4c3f41 Linus Torvalds 2005-04-16 404
24669f75a3231f Jes Sorensen 2006-01-16 405 starget = kzalloc(size, GFP_KERNEL);
^1da177e4c3f41 Linus Torvalds 2005-04-16 406 if (!starget) {
cadbd4a5e36dde Harvey Harrison 2008-07-03 407 printk(KERN_ERR "%s: allocation failure\n", __func__);
^1da177e4c3f41 Linus Torvalds 2005-04-16 408 return NULL;
^1da177e4c3f41 Linus Torvalds 2005-04-16 409 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 410 dev = &starget->dev;
^1da177e4c3f41 Linus Torvalds 2005-04-16 411 device_initialize(dev);
e63ed0d7a98014 James Bottomley 2014-01-21 412 kref_init(&starget->reap_ref);
^1da177e4c3f41 Linus Torvalds 2005-04-16 413 dev->parent = get_device(parent);
71610f55fa4db6 Kay Sievers 2008-12-03 414 dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
b0ed43360fdca2 Hannes Reinecke 2008-03-18 415 dev->bus = &scsi_bus_type;
b0ed43360fdca2 Hannes Reinecke 2008-03-18 416 dev->type = &scsi_target_type;
^1da177e4c3f41 Linus Torvalds 2005-04-16 417 starget->id = id;
^1da177e4c3f41 Linus Torvalds 2005-04-16 418 starget->channel = channel;
f0c0a376d0fcd4 Mike Christie 2008-08-17 419 starget->can_queue = 0;
^1da177e4c3f41 Linus Torvalds 2005-04-16 420 INIT_LIST_HEAD(&starget->devices);
643eb2d932c97a James Bottomley 2008-03-22 421 starget->state = STARGET_CREATED;
7c9d6f16f50d3a Alan Stern 2007-01-08 422 starget->scsi_level = SCSI_2;
c53a284f8be237 Edward Goggin 2009-04-09 423 starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
45b149b239ea9a Hannes Reinecke 2020-05-27 424 tid = scsi_target_index(starget);
ffedb4522571ac James Bottomley 2006-02-23 425 retry:
^1da177e4c3f41 Linus Torvalds 2005-04-16 426 spin_lock_irqsave(shost->host_lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^1da177e4c3f41 Linus Torvalds 2005-04-16 427
45b149b239ea9a Hannes Reinecke 2020-05-27 428 found_target = xa_load(&shost->__targets, tid);
^1da177e4c3f41 Linus Torvalds 2005-04-16 429 if (found_target)
^1da177e4c3f41 Linus Torvalds 2005-04-16 430 goto found;
45b149b239ea9a Hannes Reinecke 2020-05-27 431 if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
45b149b239ea9a Hannes Reinecke 2020-05-27 432 dev_printk(KERN_ERR, dev, "target index busy\n");
45b149b239ea9a Hannes Reinecke 2020-05-27 433 kfree(starget);
45b149b239ea9a Hannes Reinecke 2020-05-27 434 return NULL;
^^^^^^^^^^^
Need to drop the lock.
45b149b239ea9a Hannes Reinecke 2020-05-27 435 }
^1da177e4c3f41 Linus Torvalds 2005-04-16 436 spin_unlock_irqrestore(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds 2005-04-16 437 /* allocate and add */
a283bd37d00e92 James Bottomley 2005-05-24 438 transport_setup_device(dev);
a283bd37d00e92 James Bottomley 2005-05-24 439 if (shost->hostt->target_alloc) {
32f95792500794 Brian King 2006-02-22 440 error = shost->hostt->target_alloc(starget);
a283bd37d00e92 James Bottomley 2005-05-24 441
a283bd37d00e92 James Bottomley 2005-05-24 442 if(error) {
a283bd37d00e92 James Bottomley 2005-05-24 443 dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
a283bd37d00e92 James Bottomley 2005-05-24 444 /* don't want scsi_target_reap to do the final
a283bd37d00e92 James Bottomley 2005-05-24 445 * put because it will be under the host lock */
643eb2d932c97a James Bottomley 2008-03-22 446 scsi_target_destroy(starget);
a283bd37d00e92 James Bottomley 2005-05-24 447 return NULL;
a283bd37d00e92 James Bottomley 2005-05-24 448 }
a283bd37d00e92 James Bottomley 2005-05-24 449 }
884d25cc4fda20 James Bottomley 2006-09-05 450 get_device(dev);
a283bd37d00e92 James Bottomley 2005-05-24 451
^1da177e4c3f41 Linus Torvalds 2005-04-16 452 return starget;
^1da177e4c3f41 Linus Torvalds 2005-04-16 453
^1da177e4c3f41 Linus Torvalds 2005-04-16 454 found:
e63ed0d7a98014 James Bottomley 2014-01-21 455 /*
e63ed0d7a98014 James Bottomley 2014-01-21 456 * release routine already fired if kref is zero, so if we can still
e63ed0d7a98014 James Bottomley 2014-01-21 457 * take the reference, the target must be alive. If we can't, it must
e63ed0d7a98014 James Bottomley 2014-01-21 458 * be dying and we need to wait for a new target
e63ed0d7a98014 James Bottomley 2014-01-21 459 */
e63ed0d7a98014 James Bottomley 2014-01-21 460 ref_got = kref_get_unless_zero(&found_target->reap_ref);
e63ed0d7a98014 James Bottomley 2014-01-21 461
^1da177e4c3f41 Linus Torvalds 2005-04-16 462 spin_unlock_irqrestore(shost->host_lock, flags);
e63ed0d7a98014 James Bottomley 2014-01-21 463 if (ref_got) {
12fb8c1574d7d0 Alan Stern 2010-03-18 464 put_device(dev);
^1da177e4c3f41 Linus Torvalds 2005-04-16 465 return found_target;
^1da177e4c3f41 Linus Torvalds 2005-04-16 466 }
e63ed0d7a98014 James Bottomley 2014-01-21 467 /*
e63ed0d7a98014 James Bottomley 2014-01-21 468 * Unfortunately, we found a dying target; need to wait until it's
e63ed0d7a98014 James Bottomley 2014-01-21 469 * dead before we can get a new one. There is an anomaly here. We
e63ed0d7a98014 James Bottomley 2014-01-21 470 * *should* call scsi_target_reap() to balance the kref_get() of the
e63ed0d7a98014 James Bottomley 2014-01-21 471 * reap_ref above. However, since the target being released, it's
e63ed0d7a98014 James Bottomley 2014-01-21 472 * already invisible and the reap_ref is irrelevant. If we call
e63ed0d7a98014 James Bottomley 2014-01-21 473 * scsi_target_reap() we might spuriously do another device_del() on
e63ed0d7a98014 James Bottomley 2014-01-21 474 * an already invisible target.
e63ed0d7a98014 James Bottomley 2014-01-21 475 */
ffedb4522571ac James Bottomley 2006-02-23 476 put_device(&found_target->dev);
e63ed0d7a98014 James Bottomley 2014-01-21 477 /*
e63ed0d7a98014 James Bottomley 2014-01-21 478 * length of time is irrelevant here, we just want to yield the CPU
e63ed0d7a98014 James Bottomley 2014-01-21 479 * for a tick to avoid busy waiting for the target to die.
e63ed0d7a98014 James Bottomley 2014-01-21 480 */
e63ed0d7a98014 James Bottomley 2014-01-21 481 msleep(1);
ffedb4522571ac James Bottomley 2006-02-23 @482 goto retry;
ffedb4522571ac James Bottomley 2006-02-23 483 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 7ec91c3a66ca..7109afad0183 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) spin_lock_init(shost->host_lock); shost->shost_state = SHOST_CREATED; INIT_LIST_HEAD(&shost->__devices); - INIT_LIST_HEAD(&shost->__targets); + xa_init(&shost->__targets); INIT_LIST_HEAD(&shost->eh_cmd_q); INIT_LIST_HEAD(&shost->starved_list); init_waitqueue_head(&shost->host_wait); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 56c24a73e0c7..25c815355d1a 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost, } EXPORT_SYMBOL(__scsi_iterate_devices); +/** + * __scsi_target_lookup - find a target based on channel and target id + * @shost: SCSI host pointer + * @channel: channel number of the target + * @id: ID of the target + * + */ +static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost, + u16 channel, u16 id) +{ + return xa_load(&shost->__targets, (channel << 16) | id); +} + /** * starget_for_each_device - helper to walk all devices of a target * @starget: target whose devices we want to iterate over. @@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target); struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget, u64 lun) { - struct scsi_device *sdev; struct Scsi_Host *shost = dev_to_shost(starget->dev.parent); + struct scsi_device *sdev; unsigned long flags; spin_lock_irqsave(shost->host_lock, flags); @@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target); * really want to use scsi_device_lookup instead. **/ struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost, - uint channel, uint id, u64 lun) + u16 channel, u16 id, u64 lun) { + struct scsi_target *starget; struct scsi_device *sdev; - list_for_each_entry(sdev, &shost->__devices, siblings) { - if (sdev->sdev_state == SDEV_DEL) - continue; - if (sdev->channel == channel && sdev->id == id && - sdev->lun ==lun) - return sdev; - } + starget = __scsi_target_lookup(shost, channel, id); + if (!starget) + return NULL; + sdev = __scsi_device_lookup_by_target(starget, lun); + if (sdev && sdev->sdev_state == SDEV_DEL) + sdev = NULL; - return NULL; + return sdev; } EXPORT_SYMBOL(__scsi_device_lookup); @@ -729,7 +742,7 @@ EXPORT_SYMBOL(__scsi_device_lookup); * needs to be released with scsi_device_put once you're done with it. **/ struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost, - uint channel, uint id, u64 lun) + u16 channel, u16 id, u64 lun) { struct scsi_device *sdev; unsigned long flags; diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index f2437a7570ce..c47cddef1839 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget, return NULL; } +#define scsi_target_index(s) \ + ((((unsigned long)(s)->channel) << 16) | (s)->id) + static void scsi_target_destroy(struct scsi_target *starget) { struct device *dev = &starget->dev; struct Scsi_Host *shost = dev_to_shost(dev->parent); unsigned long flags; + unsigned long tid = scsi_target_index(starget); BUG_ON(starget->state == STARGET_DEL); starget->state = STARGET_DEL; @@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget) spin_lock_irqsave(shost->host_lock, flags); if (shost->hostt->target_destroy) shost->hostt->target_destroy(starget); - list_del_init(&starget->siblings); + xa_erase(&shost->__targets, tid); spin_unlock_irqrestore(shost->host_lock, flags); put_device(dev); } @@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev) } EXPORT_SYMBOL(scsi_is_target_device); -static struct scsi_target *__scsi_find_target(struct device *parent, - int channel, uint id) -{ - struct scsi_target *starget, *found_starget = NULL; - struct Scsi_Host *shost = dev_to_shost(parent); - /* - * Search for an existing target for this sdev. - */ - list_for_each_entry(starget, &shost->__targets, siblings) { - if (starget->id == id && - starget->channel == channel) { - found_starget = starget; - break; - } - } - if (found_starget) - get_device(&found_starget->dev); - - return found_starget; -} - /** * scsi_target_reap_ref_release - remove target from visibility * @kref: the reap_ref in the target being released @@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, struct scsi_target *starget; struct scsi_target *found_target; int error, ref_got; + unsigned long tid; starget = kzalloc(size, GFP_KERNEL); if (!starget) { @@ -433,19 +417,22 @@ static struct scsi_target *scsi_alloc_target(struct device *parent, starget->id = id; starget->channel = channel; starget->can_queue = 0; - INIT_LIST_HEAD(&starget->siblings); INIT_LIST_HEAD(&starget->devices); starget->state = STARGET_CREATED; starget->scsi_level = SCSI_2; starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED; + tid = scsi_target_index(starget); retry: spin_lock_irqsave(shost->host_lock, flags); - found_target = __scsi_find_target(parent, channel, id); + found_target = xa_load(&shost->__targets, tid); if (found_target) goto found; - - list_add_tail(&starget->siblings, &shost->__targets); + if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) { + dev_printk(KERN_ERR, dev, "target index busy\n"); + kfree(starget); + return NULL; + } spin_unlock_irqrestore(shost->host_lock, flags); /* allocate and add */ transport_setup_device(dev); diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 163dbcb741c1..95aaa96ce03b 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev) { struct Scsi_Host *shost = dev_to_shost(dev->parent); struct scsi_target *starget; + unsigned long tid = 0; unsigned long flags; -restart: spin_lock_irqsave(shost->host_lock, flags); - list_for_each_entry(starget, &shost->__targets, siblings) { + starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT); + while (starget) { if (starget->state == STARGET_DEL || starget->state == STARGET_REMOVE || - starget->state == STARGET_CREATED_REMOVE) + starget->state == STARGET_CREATED_REMOVE) { + starget = xa_find_after(&shost->__targets, &tid, + ULONG_MAX, XA_PRESENT); continue; + } if (starget->dev.parent == dev || &starget->dev == dev) { kref_get(&starget->reap_ref); if (starget->state == STARGET_CREATED) @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev) spin_unlock_irqrestore(shost->host_lock, flags); __scsi_remove_target(starget); scsi_target_reap(starget); - goto restart; + spin_lock_irqsave(shost->host_lock, flags); + starget = xa_find_after(&shost->__targets, &tid, + ULONG_MAX, XA_PRESENT); + continue; } } spin_unlock_irqrestore(shost->host_lock, flags); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index c3cba2aaf934..28034cc0fce5 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q); extern int __must_check scsi_device_get(struct scsi_device *); extern void scsi_device_put(struct scsi_device *); extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *, - uint, uint, u64); + u16, u16, u64); extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *, - uint, uint, u64); + u16, u16, u64); extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *, u64); extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *, diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 822e8cda8d9b..b9395676c75b 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -521,7 +521,7 @@ struct Scsi_Host { * access this list directly from a driver. */ struct list_head __devices; - struct list_head __targets; + struct xarray __targets; struct list_head starved_list;
Use an xarray instead of lists for holding the scsi targets. I've also shortened the 'channel' and 'id' values to 16 bit as none of the drivers requires a full 32bit range for either of them, and by shortening them we can use them as the index into the xarray for storing the scsi_target pointer. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/hosts.c | 2 +- drivers/scsi/scsi.c | 35 ++++++++++++++++++++++++----------- drivers/scsi/scsi_scan.c | 39 +++++++++++++-------------------------- drivers/scsi/scsi_sysfs.c | 15 +++++++++++---- include/scsi/scsi_device.h | 4 ++-- include/scsi/scsi_host.h | 2 +- 6 files changed, 52 insertions(+), 45 deletions(-)