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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
