7:18 a.m.
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