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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux