10:19 a.m.
On 03/15/2017 10:08 AM, John Ferlan wrote:
On 03/12/2017 06:35 PM, Laine Stump wrote:
> On 03/10/2017 04:10 PM, John Ferlan wrote:
>> Move the bulk of the code to the node_device_conf and rename to
>> virNodeDevice{Create|Delete}Vport.
>>
>> Alter the create algorithm slightly in order to return the name of
>> the scsi_host vHBA that was created. The existing code would fetch
>> the name and if it exists would start a thread looking for the LUNs;
>> however, in no way did it validate the device was created for the
>> storage pool even though it would be used thereafter.
>>
>> This slight change allows the code that checks if the node device
>> by wwnn/wwpn already exists and return that name. This fixes a
>> second yet seen issue that if the nodedev was present, but the
>> parent by name wasn't provided (perhaps parent by wwnn/wwpn or
>> by fabric_name), then a failure would be returned. For this path
>> it shouldn't be an error - we should just be happy that something
>> else is managing the device and we don't have to create/delete it.
>>
>> The end result is that the startVport code can now just start the
>> thread once it gets a non NULL name back.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/conf/node_device_conf.c | 211 +++++++++++++++++++++++++++++++++++++
>> src/conf/node_device_conf.h | 9 ++
>> src/libvirt_private.syms | 2 +
>> src/storage/storage_backend_scsi.c | 192 +++------------------------------
>> 4 files changed, 235 insertions(+), 179 deletions(-)
>>
[...]
>> -
>> - /* Since we're creating the vHBA, then we need to manage removing it
>> + /* Since we've created the vHBA, then we need to manage removing it
>> * as well. Since we need this setting to "live" through a
libvirtd
>> * restart, we need to save the persistent configuration. So if not
>> * already defined as YES, then force the issue.
>> @@ -345,28 +244,20 @@ createVport(virConnectPtr conn,
>
>
> Right in this space the old code would set managed = YES and call
virStoragePoolSaveConfig(). That is now done *after calling virVHBAManageVport() (whatever
*that* does) and a few other things from post-saveconfig that have been moed into
virNodeDeviceCreateVport(). I just want a confirmation that this change in ordering
won't cause a problem...
>
Right it was intentional and (famous last words) I don't think there's
going to be a problem in the change of ordering...
One of the things virNodeDeviceCreateVport now does is ensure that the
vHBA was created (whether via using virNodeDeviceCreateXML success or
after the virVHBAManageVport(CREATE) ensuring that the new scsi_hostN by
FCHost wwnn/wwpn can be found and if not, attempt a DELETE.
If you consider the "current" code - it would save the XML, then attempt
the CREATE, but not reset the config file if that failed. I suppose I
could have split across multiple patches too. Of course that's why my
commit message is extra wordy ;-)
>> }
>> }
>>
>> - if (virVHBAManageVport(parent_host, fchost->wwpn, fchost->wwnn,
>> - VPORT_CREATE) < 0)
>> - goto cleanup;
>> -
>> - virWaitForDevices();
>> -
>> /* Creating our own VPORT didn't leave enough time to find any
LUN's,
>> * so, let's create a thread whose job it is to call the FindLU's
with
>> * retry logic set to true. If the thread isn't created, then no big
>> * deal since it's still possible to refresh the pool later.
>> */
>> - if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> - if (VIR_ALLOC(cbdata) == 0) {
>> - memcpy(cbdata->pool_uuid, def->uuid, VIR_UUID_BUFLEN);
>> - VIR_STEAL_PTR(cbdata->fchost_name, name);
>> -
>> - if (virThreadCreate(&thread, false,
virStoragePoolFCRefreshThread,
>> - cbdata) < 0) {
>> - /* Oh well - at least someone can still refresh afterwards */
>> - VIR_DEBUG("Failed to create FC Pool Refresh Thread");
>> - virStoragePoolFCRefreshDataFree(cbdata);
>> - }
>
> Okay, since a failure of virVHBAGetHostByWWN() in virNodeDeviceCreateVport() would
have resulted in an early return, it's okay to move all of this outside the
conditional (which no longer exists!)
>
Right - although if desired I can split this patch up into (at least)
two to make it more obvious.
Nope. It was all decipherable as it is. Of course if you're looking to
get your patch count up, then...
Tks -
John
>
> Everything looks like a simple hatchet and stitch job to me - ACK.
>
>
[...]