Restart scsi_device_lookup_by_target

drivers/scsi/scsi.c +1 -1

@@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
 						 uint lun)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
 

Quoted 9 lines · Hannes Reinecke

drivers/scsi/scsi.c +1 -1

@@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
 						 uint lun)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev = NULL;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
 

drivers/scsi/scsi.c EXPORT_SYMBOL(__scsi_device_lookup_by_target); · line 1151

Quoted 10 lines

drivers/scsi/scsi.c +3 -2

 	spin_lock_irqsave(shost->host_lock, flags);
-	sdev = __scsi_device_lookup_by_target(starget, lun);
+ restart:
+	sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
 	if (sdev && scsi_device_get(sdev))
-		sdev = NULL;
+		goto restart;
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	return sdev;

Backwards jumps are generally disapproved of. How about:

	spin_lock_irqsave(shost->host_lock, flags);
	for (;;) {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
		if (!sdev || !scsi_device_get(sdev))
			break;
	}
	spin_unlock_irqrestore(shost->host_lock, flags);

Quoted 24 lines · Matthew Wilcox

Quoted 19 lines · Hannes Reinecke

drivers/scsi/scsi.c +4 -3

@@ -1131,14 +1137,15 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target 
 *starget,
                          uint lun)
 {
-    struct scsi_device *sdev;
+    struct scsi_device *sdev = NULL;
     struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
     unsigned long flags;
 
     spin_lock_irqsave(shost->host_lock, flags);
-    sdev = __scsi_device_lookup_by_target(starget, lun);
+ restart:
+    sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
     if (sdev && scsi_device_get(sdev))
-        sdev = NULL;
+        goto restart;
     spin_unlock_irqrestore(shost->host_lock, flags);
 

Quoted 1 line

     return sdev;

Backwards jumps are generally disapproved of. How about:

Quoted 9 lines

    spin_lock_irqsave(shost->host_lock, flags);
    for (;;) {
        sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
        if (!sdev || !scsi_device_get(sdev))
            break;
    }
    spin_unlock_irqrestore(shost->host_lock, flags);

I must say I don't really like the for(;;) construct. And it's really confusing as we want to find an sdev, so breaking if it's _not_ found is ... weird.

Quoted 9 lines · Matthew Wilcox

Backwards jumps are generally disapproved of. How about:

	spin_lock_irqsave(shost->host_lock, flags);
	for (;;) {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
		if (!sdev || !scsi_device_get(sdev))
			break;
	}
	spin_unlock_irqrestore(shost->host_lock, flags);

I must say I don't really like the for(;;) construct.

And it's really confusing as we want to find an sdev, so breaking if it's _not_ found is ... weird.

Quoted 11 lines · Hannes Reinecke

Quoted 9 lines · Matthew Wilcox

Backwards jumps are generally disapproved of. How about:

	spin_lock_irqsave(shost->host_lock, flags);
	for (;;) {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
		if (!sdev || !scsi_device_get(sdev))
			break;
	}
	spin_unlock_irqrestore(shost->host_lock, flags);

Quoted 1 line

I must say I don't really like the for(;;) construct.

I'd be fine with:

	do {
		sdev = __scsi_device_lookup_by_target(starget, sdev, lun);
	} while (sdev && scsi_device_get(sdev));

though I find that slightly less clear than the for (;;) construct.

Quoted 2 lines

And it's really confusing as we want to find an sdev, so breaking if it's _not_ found is ... weird.

Those are the two conditions when we want to stop trying -- if there's no device or if the device we've found is bad. I can definitely see an argument for splitting the two conditions to make that more obvious. I can also see an argument for not returning an sdev in the _DEL state from __scsi_device_lookup_by_target in the first place.

Hi all,

currently we have this:

struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
uint lun)

{
        struct scsi_device *sdev;
        struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
        unsigned long flags;

        spin_lock_irqsave(shost->host_lock, flags);
        sdev = __scsi_device_lookup_by_target(starget, lun);
        if (sdev && scsi_device_get(sdev))
                sdev = NULL;
        spin_unlock_irqrestore(shost->host_lock, flags);

        return sdev;
}

now consider an sdev list with two entries for LUN 0, the first being in SDEV_DEL and the second being in SDEV_RUNNING.

scsi_device_lookup_by_target will always return NULL here, as it will never be able to skip past the first (unuseable) entry. So scsi_report_lun_scan will happily create duplicate sdevs for LUN 0 and we'll be getting the infamous

sysfs: duplicate filename 'xxxx' can not be created

errors.

What we should be doing here is to restart
__scsi_device_lookup_by_target() if we find an
sdev but cannot use it (ie is in SDEV_DEL).

James, please apply.

Cheers,

Hannes
plain text document attachment (scsi-restart-lookup-by-target) Restart scsi_device_lookup_by_target()

When scsi_device_lookup_by_target() will always return the first sdev with a matching LUN, regardless of the state. However, when this sdev is in SDEV_DEL it's quite possible to have additional sdevs in the list with the same LUN which are active.
So we should restart __scsi_device_lookup_by_target() whenever we find an sdev with state SDEV_DEL.

Signed-off-by Hannes Reinecke <[email protected]>

Quoted 52 lines · Hannes Reinecke

Hi all,

currently we have this:

struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
uint lun)

{
        struct scsi_device *sdev;
        struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
        unsigned long flags;

        spin_lock_irqsave(shost->host_lock, flags);
        sdev = __scsi_device_lookup_by_target(starget, lun);
        if (sdev && scsi_device_get(sdev))
                sdev = NULL;
        spin_unlock_irqrestore(shost->host_lock, flags);

        return sdev;
}

now consider an sdev list with two entries for LUN 0, the first being in SDEV_DEL and the second being in SDEV_RUNNING.

scsi_device_lookup_by_target will always return NULL here, as it will never be able to skip past the first (unuseable) entry. So scsi_report_lun_scan will happily create duplicate sdevs for LUN 0 and we'll be getting the infamous

sysfs: duplicate filename 'xxxx' can not be created

errors.

What we should be doing here is to restart
__scsi_device_lookup_by_target() if we find an
sdev but cannot use it (ie is in SDEV_DEL).

James, please apply.

Cheers,

Hannes
plain text document attachment (scsi-restart-lookup-by-target) Restart scsi_device_lookup_by_target()

When scsi_device_lookup_by_target() will always return the first sdev with a matching LUN, regardless of the state. However, when this sdev is in SDEV_DEL it's quite possible to have additional sdevs in the list with the same LUN which are active.
So we should restart __scsi_device_lookup_by_target() whenever we find an sdev with state SDEV_DEL.

Quoted 1 line

Signed-off-by Hannes Reinecke <[email protected]>

That's brilliant, thanks for finding this bug.

rather than go to the trouble of restarting the scan looking for duplicates, since none of the users of __scsi_device_lookup_by_target actually want invisible dying devices in SDEV_DEL, why not just make it skip over them. If we ignore devices in SDEV_DEL, the target id of the visible devices should all be unique, so alter the

if (sdev->lun == lun)

to

if (sdev->lun == lun && sdev->sdev_state != SDEV_DEL)

And everything should work.

I think this is the correct fix because scsi_device_get() is also programmed to ignore devices in SDEV_DEL (for the same reason).

Hi James,

Quoted 69 lines · James Bottomley

Quoted 52 lines · Hannes Reinecke

Hi all,

currently we have this:

struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
uint lun)

{
        struct scsi_device *sdev;
        struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
        unsigned long flags;

        spin_lock_irqsave(shost->host_lock, flags);
        sdev = __scsi_device_lookup_by_target(starget, lun);
        if (sdev && scsi_device_get(sdev))
                sdev = NULL;
        spin_unlock_irqrestore(shost->host_lock, flags);

        return sdev;
}

now consider an sdev list with two entries for LUN 0, the first being in SDEV_DEL and the second being in SDEV_RUNNING.

scsi_device_lookup_by_target will always return NULL here, as it will never be able to skip past the first (unuseable) entry. So scsi_report_lun_scan will happily create duplicate sdevs for LUN 0 and we'll be getting the infamous

sysfs: duplicate filename 'xxxx' can not be created

errors.

What we should be doing here is to restart
__scsi_device_lookup_by_target() if we find an
sdev but cannot use it (ie is in SDEV_DEL).

James, please apply.

Cheers,

Hannes
plain text document attachment (scsi-restart-lookup-by-target) Restart scsi_device_lookup_by_target()

When scsi_device_lookup_by_target() will always return the first sdev with a matching LUN, regardless of the state. However, when this sdev is in SDEV_DEL it's quite possible to have additional sdevs in the list with the same LUN which are active.
So we should restart __scsi_device_lookup_by_target() whenever we find an sdev with state SDEV_DEL.

Quoted 1 line

Signed-off-by Hannes Reinecke <[email protected]>

That's brilliant, thanks for finding this bug.

rather than go to the trouble of restarting the scan looking for duplicates, since none of the users of __scsi_device_lookup_by_target actually want invisible dying devices in SDEV_DEL, why not just make it skip over them. If we ignore devices in SDEV_DEL, the target id of the visible devices should all be unique, so alter the

if (sdev->lun == lun)

to

if (sdev->lun == lun && sdev->sdev_state != SDEV_DEL)

Quoted 5 lines

And everything should work.

I think this is the correct fix because scsi_device_get() is also programmed to ignore devices in SDEV_DEL (for the same reason).

As already mentioned, I'm not sure if esp_scsi.c expects it to be that way. If someone confirms that we won't break it with this change, then of course it's the easier way.