[libvirt] [PATCH 04/11] nodedev: Add the ability to create vHBA by parent wwnn/wwpn or fabric_wwn

[Boris Fiuczynski]

On 12/13/2016 08:30 PM, John Ferlan wrote:
>
>
> On 12/13/2016 10:58 AM, Boris Fiuczynski wrote:
>> John,
>> I have a concern regarding usage of the parent_fabric_name.
>> As far I have been told there are a lot of fc_host attributes in Linux
>> that are optional and left to the low-level driver to decide if
>> implemented. fabric_name apparently happens to be one of these
>> attributes and I know an architecture that has a driver (zfcp) that does
>> not provide it.
>>
>> Up until this patch the fabric_name was collected on the host during
>> detection of the fc_host capability and provided during a nodedev dumpxml.
>
> How does this patch change that?  All this patch does is allow one to
> provide a "parent_wwnn/parent_wwpn" or "parent_fabric_name" for the
> input XML to be passed to nodedev-create. So rather than just:
>
> <device>
>   <parent>scsi_host3</parent>
>   <capability type='scsi_host'>
>     <capability type='fc_host'>
>     </capability>
>   </capability>
> </device>
>
> One could provide:
>
> <device>
>   <name>vhba</name>
>   <parent_wwnn>20000000c9848140</parent_wwnn>
>   <parent_wwpn>10000000c9848140</parent_wwpn>
>   <capability type='scsi_host'>
>     <capability type='fc_host'>
>     </capability>
>   </capability>
> </device>
>
> or
>
> <device>
>   <name>vhba</name>
>   <parent_fabric_wwn>2002000573de9a81</parent_fabric_wwn>
>   <capability type='scsi_host'>
>     <capability type='fc_host'>
>     </capability>
>   </capability>
> </device>
>
> for the 'virsh nodedev-create $FILE' command.
>
>> I would like to propose making the existence of fabric_name option in
>> the detection of the fc_host capability. In doing so it would also be
>> required make the fabric_name xml tag optional in the xml of the nodedev
>> dump. I am aware that this is a creating a slightly incompatible output
>> but how else could that be fixed than making the tag optional? I can
>> send a patch for this if you agree to the solution.
>
> The biggest hurdle I face with NPIV/vHBA is that there are many
> different options. I keep learning as I go on this stuff. It does seem
> from what you're indicating that the fabric_name file just doesn't
> exist. Although without having such a system available to me I have no
> way of knowing for sure.
>
> In any case, if "fabric_name" is not read, then 'scsi_host.fabric_wwn'
> is left blank (see nodeDeviceSysfsGetSCSIHostCaps). There isn't an error
> path that would cause a failure (just a debug message). If it is NULL,
> then since the Format code uses virBufferEscapeString which shouldn't
> format the XML for it. So given that, I suppose it's reasonable to
> provide an "<optional>" tag for the RNG file (although similarly
> wwnn/wwpn are also then optional). That seems reasonable (but unrelated
> to this code).
In virReadFCHost the method virFileReadAll is used to read the file 
fabric_name. This method triggers
virReportSystemError(errno, _("Failed to open file '%s'"), path);
if the file cannot be opened. As a result virReadFCHost returns NULL 
into nodeDeviceSysfsGetSCSIHostCaps which causes cleanup to be called 
resulting in the removal of the fc_host capability.

I will post a patch for fixing this issue.

>
> Caveat - there is a udev bug/issue
> (https://bugzilla.redhat.com/show_bug.cgi?id=1319544) where the
> scsi_host for the vHBA is created before the files we look at have valid
> data. It's one of those where the QE is doing repeated create/destroy
> and udev hasn't really finished updating things before it sends the
> create event.
>
>>
>> The implications regarding the search by parent_fabric_wwn should be
>> obvious.
>>
>
> For the purpose of this patch set though, parent_fabric_wwn is a 'read
> only' type value. Unless I messed up in the code, if it's not available
> to the HBA/scsi_host on the host, then regardless of what's provided in
> parent_fabric_wwn the parent won't be found by fabric_wwn/fabric_name. I
> suppose I could add checks in virNodeDeviceDefParseXML that the provided
> string "looks like" a WWN (eg if virValidateWWN call) as a crude form of
> XML validation.

My point is that your patch starts using the optional fabric_name as 
(optional) input parameter in the API. It seems that the wwnn/wwpn pair 
provides the most specific search option anyway and always works where 
as the fabric_wwn might work only for some.
Anyway, you are right, that it can be done if it is all optional and the 
fix for the fc_host capability is mostly independent of this series 
especially since the fabric_wwn tag is only written out and never read 
in from xml anyway!

>
>
> John
>
> [...]
>


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294