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

Re: [libvirt] [PATCH] storage: More uniquely identify NPIV LUNs



On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1657468

Commit be1bb6c95 changed the way volumes were stored from a forward
linked list to a hash table. In doing so, it required that each vol
object would have 3 unique values as keys into tables - key, name,
and path. Due to how vHBA/NPIV LUNs are created/used this resulted
in a failure to utilize all the LUN's found during processing.

The virStorageBackendSCSINewLun uses virStorageBackendSCSISerial
in order to read/return the unique serial number of the LUN to be
used as a key for the volume.

However, for NPIV based LUNs the logic results in usage of the
same serial value for each path to the LUN. For example,

$ lsscsi -tg
...
[4:0:3:13]   disk    fc:0x207800c0ffd79b2a0xeb02ef   /dev/sde   /dev/sg16
[4:0:4:0]    disk    fc:0x50060169446021980xeb1f00   /dev/sdf   /dev/sg17
[4:0:5:0]    disk    fc:0x50060161446021980xeb2000   /dev/sdg   /dev/sg18
...

/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sde
3600c0ff000d7a2965c603e5401000000
/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdf
350060160c460219850060160c4602198
/lib/udev/scsi_id --replace-whitespace --whitelisted --device /dev/sdg
350060160c460219850060160c4602198

The /dev/sdf and /dev/sdg although separate LUNs would end up with the
same serial number used for the vol->key value. When attempting to add
the LUN via virStoragePoolObjAddVol, the hash table code will indicate
that we have a duplicate:

   virHashAddOrUpdateEntry:341 : internal error: Duplicate key

and thus the LUN is not added to the pool.

Digging deeper into the scsi_id output using the --export option one
will find that the only difference between the two luns is:

   ID_TARGET_PORT=1 for /dev/sdf
   ID_TARGET_PORT=2 for /dev/sdg

So, let's use the ID_TARGET_PORT string value along with the serial
to generate a more unique key using "@serial_PORT target_port", where
@target_port is just the value of the ID_TARGET_PORT for the LUN.

Signed-off-by: John Ferlan <jferlan redhat com>
---
src/storage/storage_util.c | 61 +++++++++++++++++++++++++++++++++++++-
1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c
index a84ee5b600..d6d441c06d 100644
--- a/src/storage/storage_util.c
+++ b/src/storage/storage_util.c
@@ -3755,6 +3755,49 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool)
}


+static char *
+virStorageBackendSCSITargetPort(const char *dev)
+{
+    char *target_port = NULL;
+    const char *id;
+#ifdef WITH_UDEV

This sort of conditional code within a function can lead to confusing
code. Also, why is this even depending on WITH_UDEV if it does not
depend on libudev at all?

+    virCommandPtr cmd = virCommandNewArgList(
+        "/lib/udev/scsi_id",
+        "--replace-whitespace",
+        "--whitelisted",
+        "--export",
+        "--device", dev,
+        NULL
+        );
+
+    /* Run the program and capture its output */

// THIS IS BRIDGE [0]

+    virCommandSetOutputBuffer(cmd, &target_port);
+    if (virCommandRun(cmd, NULL) < 0)
+        goto cleanup;
+#endif
+
+    if (target_port && STRNEQ(target_port, "") &&
+        (id = strstr(target_port, "ID_TARGET_PORT="))) {
+        char *nl = strchr(id, '\n');
+        if (nl)
+            *nl = '\0';
+        id = strrchr(id, '=');
+        memmove(target_port, id + 1, strlen(id));
+    } else {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("unable to uniquely identify target port for '%s'"),
+                       dev);

The other usage of /lib/udev/scsi_id oddly guarded by WITH_UDEV actually
provides a fallback in case we're compiling on a system without udev.

Erroring out anyway makes the whole #ifdef even more pointless.

Jano

+    }
+
+#ifdef WITH_UDEV
+ cleanup:
+    virCommandFree(cmd);
+#endif
+
+    return target_port;
+}
+
+

[0] https://www.abstrusegoose.com/432

Attachment: signature.asc
Description: PGP signature


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