[libvirt] [PATCH 1/2] storage: Fix autostart of pool with "fc_host" type adapter

[Osier Yang]

On 23/01/14 21:35, John Ferlan wrote:
>
> On 01/23/2014 04:45 AM, Osier Yang wrote:
>> On 22/01/14 21:36, John Ferlan wrote:
>>> 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...
>>>
>> Found your method *might* break one logic.
>>
>> Assuming the pool is not marked as "autostart", and the vHBA was not
>> created yet. Since with your method "checkPool" will create the vHBA now,
>> then it's possible for "checkPool" to return "*isActive" as true, as long as
>> the device path for the created vHBA can show up in host very quickly
>> (like what we talked a lot in PATCH 2/2, how long it will show up is also
>> depended, the time can be long, but it also can be short), i.e. during the
>> "checkPool" process.   And thus the pool will be marked as "Active". As
>> the result, the problem on one side of the coin is fixed, but it introduces
>> a similar problem on another side. :-)
> Huh?  Not sure I see what problem you're driving at.
>
> Look back at the caller - isActive from _scsi translates to "started" in
> the caller.  The 'started' is used to decide whether to call 'startPool'
> and 'refreshPool'.  If autostart is disabled, then 'startPool' won't be
> called.

Assuming pool->autostart if false, and "started" is happened to be true.

Calling refreshPool is likely to cause pool->active == 1.

<...>
         if (started) {
             if (backend->refreshPool(conn, pool) < 0) {
                 virErrorPtr err = virGetLastError();
                 if (backend->stopPool)
                     backend->stopPool(conn, pool);
                 VIR_ERROR(_("Failed to autostart storage pool '%s': %s"),
                           pool->def->name, err ? err->message :
                           _("no error message found"));
                 virStoragePoolObjUnlock(pool);
                 continue;
             }
             pool->active = 1;
         }
</...>

Since the vHBA was already created in checkPool, then I see not much
reason why refreshPool can not be success.

Osier