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

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.