Re: [PATCH][RFC] scsi: Use W_LUN for scanning

[Steffen Maier <maier@linux.vnet.ibm.com>]

just a small addendum regarding the naming of the w_lun variable

On 03/15/2013 04:54 PM, Steffen Maier wrote:
> While we're at it: I recently figured that there are targets responding
> to inquiry with PQ=1 && PDT=31 for LUN0 if LUN0 has no backing device
> (e.g. no disk mapped for the initiator host). While this is likely to
> work with in-kernel lun scanning, the kernel does not even allocate an
> sg dev in this case.
> If the target (such do exist) now also does not support report_luns on
> WLUN, this effectively prevents any user space lun discovery.
> E.g. non-NPIV zfcp-attached LUNs cannot be discovered, because we cannot
> do in-kernel scanning due to the lack of initiator lun masking.
>
> The reason for Linux ignoring LUNs with PDT=31 and PQ=1 is:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=84961f28e9d13a4b193d0c8545f3c060c1890ff3
> [SCSI] Don't add scsi_device for devices that return PQ=1, PDT=0x1f
> from kernel 2.6.19.
> Since Linux on System z could no longer perform report luns with IBM
> DS8000, the following workaround was implemented:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=01b291bd66564b4bd826326af6bd0b6d17e99439
> [SCSI] fix check of PQ and PDT bits for WLUNs
> in kernel 2.6.27.
> However, this only works for WLUNs reporting PDT=31 and PQ=1 but not for
> LUN0.
> What made it worse is that, the attached LUN looks perfect from a zfcp
> status point of view but is still missing any user visible handle for
> the scsi midlayer, so I was puzzled as a user despite the otherwise
> clear scsi log message:
> "scsi scan: peripheral device type of 31, ***no device added***".
>
> On 03/15/2013 10:46 AM, Hannes Reinecke wrote:
>> SAM advertises the use of a Well-known LUN (W_LUN) for scanning.
>> As this avoids exposing LUN 0 (which might be a valid LUN) for
>> all initiators it is the preferred method for LUN scanning on
>> some arrays.
>> So we should be using W_LUN for scanning, too. If the W_LUN is
>> not supported we'll fall back to use LUN 0.
>> For broken W_LUN implementations a new blacklist flag
>> 'BLIST_NO_WLUN' is added.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 3e58b22..f4ccdea 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1312,6 +1312,7 @@ static int scsi_report_lun_scan(struct
>> scsi_target *starget, int bflags,
>>       unsigned int num_luns;
>>       unsigned int retries;
>>       int result;
>> +    int w_lun = SCSI_W_LUN_REPORT_LUNS;

Meanwhile I realized why I sometimes had to think twice to understand 
below code: The naming of the identifier "w_lun" is misleading to me 
because it seems to imply that the variable (always) contains the value 
of the WLUN while in fact it could also contain LUN0. Maybe the variable 
should just be called "lun"?

>>       struct scsi_lun *lunp, *lun_data;
>>       u8 *data;
>>       struct scsi_sense_hdr sshdr;
>> @@ -1337,11 +1338,20 @@ static int scsi_report_lun_scan(struct
>> scsi_target *starget, int bflags,
>>           return 0;
>>       if (starget->no_report_luns)
>>           return 1;
>> +    if (bflags & BLIST_NO_WLUN)
>> +        w_lun = 0;
>>
>> +retry_report_lun_scan:
>>       if (!(sdev = scsi_device_lookup_by_target(starget, 0))) {
>> -        sdev = scsi_alloc_sdev(starget, 0, NULL);
>> -        if (!sdev)
>> -            return 0;
>> +        sdev = scsi_alloc_sdev(starget, w_lun, NULL);
>> +        if (!sdev) {
>> +            if (w_lun != 0) {
>> +                w_lun = 0;
>> +                sdev = scsi_alloc_sdev(starget, w_lun, NULL);
>> +            }
>
>> +            if (!sdev)
>> +                return 0;
>
> Semantically this belongs to the 2nd scsi_alloc_sdev(starget, w_lun,
> NULL) i.e. inside (at the end of) the body of if (w_lun != 0).
> Then you can even merge the outer and inner if statement to
> if (!sdev && wlun != 0)
> Semantics: WLUN did not work and we haven't already tried LUN0.
> Maybe it's just me but I found it more difficult to read if the 2nd
> check is on its own inside the outer if statement with exactly the same
> boolean expression.
>
>
>> +        }
>>           if (scsi_device_get(sdev)) {
>>               __scsi_remove_device(sdev);
>>               return 0;
>
> Now that we not only use LUN0, it would be nice to reflect the currently
> used LUN in the corresponding scsi logging statements for debugging and
> problem determination purposes.
> It seems as if devname only contains HCT but not L, which can now be
> either WLUN or LUN0, e.g. here:
>
>>     for (retries = 0; retries < 3; retries++) {
>>         SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "scsi scan: Sending"
>>                 " REPORT LUNS to %s (try %d)\n", devname,
>>                 retries));
>>
>>         result = scsi_execute_req(sdev, scsi_cmd, DMA_FROM_DEVICE,
>>                       lun_data, length, &sshdr,
>>                       SCSI_TIMEOUT + 4 * HZ, 3, NULL);
>>
>>         SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "scsi scan: REPORT LUNS"
>>                 " %s (try %d) result 0x%x\n", result
>>                 ?  "failed" : "successful", retries, result));
>
>> @@ -1418,6 +1428,18 @@ static int scsi_report_lun_scan(struct
>> scsi_target *starget, int bflags,
>>       }
>>
>>       if (result) {
>> +        if (w_lun != 0 && scsi_device_created(sdev)) {
>
> Did you mean _!_scsi_device_created(sdev), i.e. we tried WLUN but could
> not attach it?
>
>> +            /*
>> +             * W_LUN probably not supported, try with LUN 0
>> +             */
>
> Does this only check for an sg dev having been created or does it check
> for the response to the report_luns cmnd? It should be the latter since
> there are targets allowing attachment of WLUN (with PQ=1 && PDT=31) but
> refuse to answer report_luns, e.g. IBM SVC / Storwize V7000.
> Remember your changes to zfcp_san_disc reacting on sg_luns return code?
> [Novell bug 742352]
> It would be nice if we could avoid start using BLIST_NO_WLUN right away.
> ;-)
>
>> +            SCSI_LOG_SCAN_BUS(3, printk (KERN_INFO "scsi scan:"
>> +                    "W_LUN not supported, try LUN 0\n"));
>> +            kfree(lun_data);
>> +            scsi_device_put(sdev);
>> +            __scsi_remove_device(sdev);
>> +            w_lun = 0;
>> +            goto retry_report_lun_scan;
>> +        }
>>           /*
>>            * The device probably does not support a REPORT LUN command
>>            */
>> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
>> index cc1f3e7..ffb42b1 100644
>> --- a/include/scsi/scsi_devinfo.h
>> +++ b/include/scsi/scsi_devinfo.h
>> @@ -31,4 +31,5 @@
>>   #define BLIST_MAX_512        0x800000 /* maximum 512 sector cdb
>> length */
>>   #define BLIST_ATTACH_PQ3    0x1000000 /* Scan: Attach to PQ3 devices */
>>   #define BLIST_NO_DIF        0x2000000 /* Disable T10 PI (DIF) */
>> +#define BLIST_NO_WLUN        0x4000000 /* Disable W_LUN scanning */
>>   #endif
>>
>
> The overall idea seems reasonable to me.

Steffen

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html