On 01/07/2014 06:07 AM, Osier Yang wrote:
> On 07/01/14 01:21, John Ferlan wrote:
>>
>> On 01/06/2014 05:19 AM, Osier Yang wrote:
>>> The "checkPool" is a bit different for pool with "fc_host"
>>> type source adapter, since the vHBA it's based on might be
>>> not created yet (it's created by "startPool", which is
>>> involked after "checkPool" in storageDriverAutostart). So it
>>> should not fail, otherwise the "autostart" of the pool will
>>> fail either.
>>>
>>> The problem is easy to reproduce:
>>> * Enable "autostart" for the pool
>>> * Restart libvirtd service
>>> * Check the pool's state
>>> ---
>>> src/storage/storage_backend_scsi.c | 14 ++++++++++++--
>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>> Not sure this is the right thing to do. With this change it doesn't
>> matter what getAdapterName() returns for fc_host's...
>
> It matters with if "*isActive" is true or false in the end. We don't
> need to try to start the pool if it's already active.
>
Fair enough; however, let's consider the failure case. On failure, a
message is reported and name == NULL. Do we need to clear that error now?
I think my original thoughts were along the lines of having
getAdapterName do more of the heavy lifting. Have both check and start
call it. It would call the createVport which would be mostly unchanged,
except for the virFileWaitForDevices() which could move to start...
Thus getAdapterName is (and moves below deleteVport)
if !fc_host
vir_STRDUP(name)
else
name = createVport
return name
CheckPool doesn't change
createVport
changes to return the name
that means after the ManageVport the name would need to be fabricated
or the NameByWWN() called again
the virFileWaitForDevices() moves to the start function
startPool
calls getAdapterName()
if it doesn't get a name back - fail with error message that was in
getAdapterName when NameByWWN first failed.
free the name
call virFileWaitForDevices()
I've attached a format-patch output for this logic - your call as to
whether or not you want to use it... If you keep your logic, then just
decide upon how to handle the error message... It won't be necessary for
the check case, but would be for the refresh case.
I am OK with things as they are, it just looks odd in the CheckPool
function to be special casing FC_HOST just because it's not created yet
and then to have the start function do the create anyway. It just seems
we could be smarter/better.
John
>>
>> The getAdapterName() already has some code to specifically handle
>> VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST. The call to
>> virGetFCHostNameByWWN() is similar to createVport() which is called by
>> the 'start' path, except that the createVport() path will be happy if
>> the name already exists.
>>
>> Since it seems the checkPool is meant to initialize things (from reading
>> the error message in the calling function), why not create the vPort in
>> the check function? It's a bit more refactoring that perhaps initially
>> desired, although having a vport hanging around unused may not be quite
>> right either.
>
> No, "checkPool" is only involked when autostarting the pools. Not
> by poolCreate, poolCreateXML, etc. That's why creating pool must
> fall into "startPool".
>
>>
>> Another option would be to have the check function perform "enough
>> initialization" or "checks" to make it more likely that the start path
>> will succeed.
>
> That's actually what "checkPool" *should* do. But for "fc_host" pool,
> it's a bit special, since it's unlike other pool types, the underlying
> physical stuffs must be already existing on host.
>
>>
>> Looking at the code does cause me to wonder what happens in the
>> "existing" code if the vport was created when the CheckPool function was
>> called returning the NameByWWN() in the 'name' field. The subsequent
>> getHostNumber() call uses the 'name' instead of what the start path does
>> using the parent value.
>
> It's right. "getHostNumber" in "checkPool" is to get the SCSI host
> number of the vHBA. And then checking if the SCSI device shows
> up in the sysfs tree with the got host number.
>
> "getHostNumber" in "startPool" should get the parent's host number,
> since it should know the sys file path "/.../.../create_vport". And write
> command to it.
>
>> It seems the "check" for fc_host and non-fc_host is different enough
>> that the distinguishment needs to be in the check routine rather than
>> hidden in the getAdapterName() function.
>
> Not quite clear about your meaning. But getAdapterName is just
> a wrapper to get the adapter name with regarding to different
> adapter type. Any relationship of the "check" difference ?
>
> Regards,
> Osier
>
>From 0418cd273ca591122ca149ff3a5b9356adcaee39 Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan@xxxxxxxxxx>
Date: Wed, 22 Jan 2014 08:17:04 -0500
Subject: [PATCH] Adjust autostart logic
Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
---
src/storage/storage_backend_scsi.c | 61 +++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 31 deletions(-)
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index 6f86ffc..d3d14ce 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -603,41 +603,18 @@ getHostNumber(const char *adapter_name,
}
static char *
-getAdapterName(virStoragePoolSourceAdapter adapter)
-{
- char *name = NULL;
-
- if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
- ignore_value(VIR_STRDUP(name, adapter.data.name));
- return name;
- }
-
- if (!(name = virGetFCHostNameByWWN(NULL,
- adapter.data.fchost.wwnn,
- adapter.data.fchost.wwpn))) {
- virReportError(VIR_ERR_XML_ERROR,
- _("Failed to find SCSI host with wwnn='%s', "
- "wwpn='%s'"), adapter.data.fchost.wwnn,
- adapter.data.fchost.wwpn);
- }
-
- return name;
-}
-
-static int
createVport(virStoragePoolSourceAdapter adapter)
{
unsigned int parent_host;
char *name = NULL;
if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
- return 0;
+ return NULL;
/* This filters either HBA or already created vHBA */
if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
adapter.data.fchost.wwpn))) {
- VIR_FREE(name);
- return 0;
+ return name;
}
if (!adapter.data.fchost.parent &&
@@ -645,18 +622,19 @@ createVport(virStoragePoolSourceAdapter adapter)
virReportError(VIR_ERR_XML_ERROR, "%s",
_("'parent' for vHBA not specified, and "
"cannot find one on this host"));
- return -1;
+ return NULL;
}
if (getHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
- return -1;
+ return NULL;
if (virManageVport(parent_host, adapter.data.fchost.wwpn,
adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
- return -1;
+ return NULL;
- virFileWaitForDevices();
- return 0;
+ name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
+ adapter.data.fchost.wwpn);
+ return name;
}
static int
@@ -689,6 +667,17 @@ deleteVport(virStoragePoolSourceAdapter adapter)
return 0;
}
+static char *
+getAdapterName(virStoragePoolSourceAdapter adapter)
+{
+ char *name = NULL;
+
+ if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
+ ignore_value(VIR_STRDUP(name, adapter.data.name));
+ else
+ name = createVport(adapter);
+ return name;
+}
static int
virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
@@ -753,8 +742,18 @@ static int
virStorageBackendSCSIStartPool(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool)
{
+ char *name = NULL;
virStoragePoolSourceAdapter adapter = pool->def->source.adapter;
- return createVport(adapter);
+ if (!(name = getAdapterName(pool->def->source.adapter))) {
+ virReportError(VIR_ERR_XML_ERROR,
+ _("Failed to find SCSI host with wwnn='%s', "
+ "wwpn='%s'"), adapter.data.fchost.wwnn,
+ adapter.data.fchost.wwpn);
+ return -1;
+ }
+ VIR_FREE(name);
+ virFileWaitForDevices();
+ return 0;
}
static int
--
1.8.4.2
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list