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

Fri Jan 18 00:40:36 UTC 2019

On 1/17/19 8:45 AM, Ján Tomko wrote:
> On Tue, Jan 15, 2019 at 12:15:36PM -0500, John Ferlan wrote:
>>
>>
>> On 1/15/19 11:32 AM, Ján Tomko wrote:
>>> On Tue, Dec 18, 2018 at 05:46:53PM -0500, John Ferlan wrote:
>>>> +    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.
>>
>> I honestly cannot imagine this working without udev. Whether having NPIV
>> volumes exposed "could" happen with node_device_hal is, well, highly
>> doubtful.  Maybe someone would be brave enough to remove the hal code.
>>
> 
> There is difference between a system with working udev (I assume that
> would be equivalent to having /lib/udev/scsi_id) and a system with
> WITH_UDEV, i.e. having devel headers for libudev.

I was merely pointing out, the relationship between existing code and
what I generated.

> 
> A compile guard here is not necessary, since the virCommand APIs are
> compiled unconditionally and I think the specific error message we get
> (scsi_id command not found) is better than the vague error message.
> Also, it's less code.

It's not that important that /scsi_id doesn't exist - that's not the
problem. The "STRNEQ(key, vol->target.path)" took care of that check
detail when adapter.type == VIR_STORAGE_ADAPTER_TYPE_FC_HOST in the patch.

> 
> If you need to use #ifdef, conditionally compiling different functions
> makes the control flow easier to follow.
> 

I will change to a different model.

>> Updating the existing virStorageBackendSCSISerial to parse output with
>> the --export option is possible, but affects more than just NPIV, so
>> this patch attempts to limit the scope.
>>
>>>
>>> Erroring out anyway makes the whole #ifdef even more pointless.
>>
>> Well... If one doesn't error out then it leads to the problem seen
>> hidden in the weeds of the commit message, a:
>>
>>    virHashAddOrUpdateEntry:341 : internal error: Duplicate key
>>
> 
> In the absence of the "WITH_UDEV" guards, the error would be reported
> by virCommandRun.
> 

What error? Certainly not the duplicate key.

As the code stands now it's really fetching a duplicate key. Why this
worked before the hash tables is that the vol->key was duplicated in a
forward linked list and no one cared or knew unless they were trying to
get volumes by key in which key only 1 would return even though for NPIV
there were multiple LUN's with the same serial/key value. I have this
really vague recollection of a bug/problem mentioning this sometime long
ago, but cannot find the reference.

The WITH_UDEV is just there to follow the existing model of calling the
scsi_id.  The problem is that scsi_id without the --export only comes
back with the/a SERIAL_ID value that isn't unique enough for NPIV
volumes.  Maybe it'll help to "see" things in play...

# lsscsi -tg
...
[5:0:0:0]    enclosu fc:0x217800c0ffd79b2a,0x1001ef  -          /dev/sg19
[5:0:1:0]    enclosu fc:0x217000c0ffd79b2a,0x1002ef  -          /dev/sg20
[5:0:2:0]    enclosu fc:0x217800c0ffd7821d,0x1003ef  -          /dev/sg21
[5:0:3:0]    enclosu fc:0x217000c0ffd7821d,0x1004ef  -          /dev/sg22
[5:0:4:0]    disk    fc:0x5006016844602198,0x101f00  /dev/sdh   /dev/sg23
[5:0:5:0]    disk    fc:0x5006016044602198,0x102000  /dev/sdi   /dev/sg24
...

The processLU code will pass the /dev/sdh, /dev/sdi, (etc).  That get's
sent to the existing code to get the key (or serial), which returns:

#  /lib/udev/scsi_id --device /dev/sdh --whitelisted
--replace-whitespace /dev/sdh
350060160c460219850060160c4602198

#  /lib/udev/scsi_id --device /dev/sdh --whitelisted
--replace-whitespace /dev/sdi
350060160c460219850060160c4602198

...
See how they're the same - well when that volume is attempted to be
added in the Refresh thread later on, it gets dropped because of the
duplicate key.

W/ NPIV, the vHBA is found:

# virsh nodedev-list --cap fc_host
scsi_host3
scsi_host4
scsi_host5

# virsh nodedev-dumpxml scsi_host5
<device>
  <name>scsi_host5</name>

<path>/sys/devices/pci0000:00/0000:00:04.0/0000:10:00.0/host3/vport-3:0-0/host5</path>
  <parent>scsi_host3</parent>
  <capability type='scsi_host'>
    <host>5</host>
    <unique_id>2</unique_id>
    <capability type='fc_host'>
      <wwnn>5001a4a07a4ba8a1</wwnn>
      <wwpn>5001a4aced83bc53</wwpn>
      <fabric_wwn>2002000573de9a81</fabric_wwn>
    </capability>
  </capability>
</device>

>From which I create the vHBA:

# virsh pool-dumpxml poolvhba3
<pool type='scsi'>
  <name>poolvhba3</name>
...
    <adapter type='fc_host' parent='scsi_host3' managed='yes'
wwnn='5001a4a07a4ba8a1' wwpn='5001a4aced83bc53'/>
  </source>
...

And has the volumes listed (after RefreshThread runs):

# virsh vol-list poolvhba3
 Name         Path

------------------------------------------------------------------------------
 unit:0:4:0
/dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0

# ls -al /dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0
lrwxrwxrwx. 1 root root 9 Jan 16 13:49
/dev/disk/by-path/pci-0000:10:00.0-fc-0x5006016844602198-lun-0 -> ../../sdh

So, if I change to use --export I can get the ID_TARGET_PORT in order to
differentiate between the two:

# /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace
/dev/sdh --export
...
ID_SERIAL=350060160c460219850060160c4602198
...
ID_TARGET_PORT=2

# /lib/udev/scsi_id --device /dev/sdh --whitelisted --replace-whitespace
/dev/sdi --export
...
ID_SERIAL=350060160c460219850060160c4602198
...
ID_TARGET_PORT=2
...

Like pointed out before, it's walk through the weeds to get the answer.

>> is issued during Refresh processing (virStoragePoolFCRefreshThread)
>> which is (and must be) done in thread. It has to be done in a thread
>> because sync'ing with udev in order to "wait" for any NPIV LUNs to be
>> generated during storage pool creation is not an acceptable option
>> especially since we don't know how many exist.
>>
>> So what we have now is any multiport volume (e.g. NPIV) having only one
>> volume (more than likely port=1) being reported/added to the storage
>> pool's volumes list.
>>
>> If you'd prefer to see the same fallback as virStorageBackendSCSISerial,
>> then I'm not opposed to that, but I'm also not clear from your response
>> if that's what you'd prefer/expect/accept.
>>
> 
> In both cases (AFAIK we only build SCSI and ISCSI drivers on Linux),
> I think the fallback is pointless due to the minimum of users that would
> be using it.

Cannot disagree with this. I'm not sure who uses vHBA/NPIV. It seems to
be less and less important. Historically I recall two - HP and IBM and
maybe a few of their partners that were "active".

Still I think fixing this because it's a regression to list less LUNs
than previously is something that needs to be done.

I'll rework things to make the WITH_UDEV less apparent at least in the
storage_util code. Look up virStorageFileGetSCSIKey - it's similar, I'll
go from there.

John

> 
> Jano
> 
>> Please be more specific how you you'd like to see this code organized
>> such that you won't feel so lost in the weeds.  And yes, the processing
>> of an NPIV device is very much so like a Rube Goldberg project. Your
>> link also has a remarkable similarity to the mouse trap game
>> [https://en.wikipedia.org/wiki/Mouse_Trap_(game)] ;-).
>>
>> Tks -
>>
>> John