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

[John Ferlan]

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.

Truly all that's not done prior to refreshPool being called is the
'virFileWaitForDevices();'... So move that into createVport().

Or am I missing something?

If the pool isn't autostarted, then the next start will call
getAdapterName() which determines the pool is started (or not) and be happy.

John

> 
> That's why I said previously I don't want to do any creation work in
> "checkPool", it should only check something.
> 
> So, I'm going forward to push my patch, with the last error reset in
> "checkPool".
> 
> Osier
>