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

[John Ferlan]

ping^2?

Any brave soul want to take a look?

Tks -

John

On 1/5/19 9:56 AM, John Ferlan wrote:
> 
> 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 at target_port", where
>> @target_port is just the value of the ID_TARGET_PORT for the LUN.
>>
>> Signed-off-by: John Ferlan <jferlan at 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;
>>  }
>>  
>>
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>