Re: [libvirt] [PATCH 1/2] storage: Fix autostart of pool with "fc_host" type adapter
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.
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