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

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



ping?

Tks,

John

On 12/18/18 5:46 PM, 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
> +    virCommandPtr cmd = virCommandNewArgList(
> +        "/lib/udev/scsi_id",
> +        "--replace-whitespace",
> +        "--whitelisted",
> +        "--export",
> +        "--device", dev,
> +        NULL
> +        );
> +
> +    /* Run the program and capture its output */
> +    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);
> +    }
> +
> +#ifdef WITH_UDEV
> + cleanup:
> +    virCommandFree(cmd);
> +#endif
> +
> +    return target_port;
> +}
> +
> +
>  static char *
>  virStorageBackendSCSISerial(const char *dev)
>  {
> @@ -3813,6 +3856,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>      virStorageVolDefPtr vol = NULL;
>      char *devpath = NULL;
> +    char *key = NULL;
> +    char *target_port = NULL;
>      int retval = -1;
>  
>      /* Check if the pool is using a stable target path. The call to
> @@ -3877,9 +3922,21 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>                                                   VIR_STORAGE_VOL_READ_NOERROR)) < 0)
>          goto cleanup;
>  
> -    if (!(vol->key = virStorageBackendSCSISerial(vol->target.path)))
> +    if (!(key = virStorageBackendSCSISerial(vol->target.path)))
>          goto cleanup;
>  
> +    if (def->source.adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST &&
> +        STRNEQ(key, vol->target.path)) {
> +        /* NPIV based LUNs use the same "serial" key. In order to distinguish
> +         * we need to append a port value */
> +        if (!(target_port = virStorageBackendSCSITargetPort(vol->target.path)))
> +            goto cleanup;
> +        if (virAsprintf(&vol->key, "%s_PORT%s", key, target_port) < 0)
> +            goto cleanup;
> +    } else {
> +        VIR_STEAL_PTR(vol->key, key);
> +    }
> +
>      def->capacity += vol->target.capacity;
>      def->allocation += vol->target.allocation;
>  
> @@ -3892,6 +3949,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>   cleanup:
>      virStorageVolDefFree(vol);
>      VIR_FREE(devpath);
> +    VIR_FREE(target_port);
> +    VIR_FREE(key);
>      return retval;
>  }
>  
> 


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