[libvirt] [PATCH] storage: Don't do wait loops from VolLookupByPath

[Cole Robinson]

On 10/22/2012 03:25 AM, Osier Yang wrote:
> On 2012年10月22日 01:11, Cole Robinson wrote:
>> virStorageVolLookupByPath is an API call that virt-manager uses
>> quite a bit when dealing with storage. This call use BackendStablePath
>> which has several usleep() heuristics that can be tripped up
>> and hang virt-manager for a while.
>>
>> Current example: an empty mpath pool pointing to /dev/mapper makes
>> _any_ calls to virStorageVolLookupByPath take 5 seconds.
>>
>> The sleep heuristics are actually only needed in certain cases
>> when we are waiting for new storage to appear, so let's skip the
>> timeout steps when calling from LookupByPath.
>> ---
>>   src/storage/storage_backend.c      | 12 ++++++++----
>>   src/storage/storage_backend.h      |  3 ++-
>>   src/storage/storage_backend_disk.c |  2 +-
>>   src/storage/storage_backend_scsi.c |  3 ++-
>>   src/storage/storage_driver.c       |  3 ++-
>>   5 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
>> index aea70e2..85b8287 100644
>> --- a/src/storage/storage_backend.c
>> +++ b/src/storage/storage_backend.c
>> @@ -1338,10 +1338,14 @@
>> virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target,
>>    *
>>    * Typically target.path is one of the /dev/disk/by-XXX dirs
>>    * with stable paths.
>> + *
>> + * If 'wait' is true, we use a timeout loop to give dynamic paths
>> + * a change to appear.
>>    */
>>   char *
>>   virStorageBackendStablePath(virStoragePoolObjPtr pool,
>> -                            const char *devpath)
>> +                            const char *devpath,
>> +                            bool wait)
>>   {
>>       DIR *dh;
>>       struct dirent *dent;
>> @@ -1372,7 +1376,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
>>    reopen:
>>       if ((dh = opendir(pool->def->target.path)) == NULL) {
>>           opentries++;
>> -        if (errno == ENOENT&&  opentries<  50) {
>> +        if (wait&&  errno == ENOENT&&  opentries<  50) {
>>               usleep(100 * 1000);
>>               goto reopen;
>>           }
>> @@ -1387,7 +1391,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
>>        * the target directory and figure out which one points
>>        * to this device node.
>>        *
>> -     * And it might need some time till the stabe path shows
>> +     * And it might need some time till the stable path shows
>>        * up, so add timeout to retry here.
>>        */
>>    retry:
>> @@ -1411,7 +1415,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool,
>>           VIR_FREE(stablepath);
>>       }
>>
>> -    if (++retry<  100) {
>> +    if (wait&&  ++retry<  100) {
>>           usleep(100 * 1000);
>>           goto retry;
>>       }
>> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
>> index a67eeb7..71935a7 100644
>> --- a/src/storage/storage_backend.h
>> +++ b/src/storage/storage_backend.h
>> @@ -130,7 +130,8 @@
>> virStorageBackendDetectBlockVolFormatFD(virStorageVolTargetPtr target,
>>                                           int fd);
>>
>>   char *virStorageBackendStablePath(virStoragePoolObjPtr pool,
>> -                                  const char *devpath);
>> +                                  const char *devpath,
>> +                                  bool wait);
>>
>>   typedef int (*virStorageBackendListVolRegexFunc)(virStoragePoolObjPtr pool,
>>                                                    char **const groups,
>> diff --git a/src/storage/storage_backend_disk.c
>> b/src/storage/storage_backend_disk.c
>> index 5d9e72f..3cd4362 100644
>> --- a/src/storage/storage_backend_disk.c
>> +++ b/src/storage/storage_backend_disk.c
>> @@ -83,7 +83,7 @@ virStorageBackendDiskMakeDataVol(virStoragePoolObjPtr pool,
>>            * dir every time its run. Should figure out a more efficient
>>            * way of doing this...
>>            */
>> -        vol->target.path = virStorageBackendStablePath(pool, devpath);
>> +        vol->target.path = virStorageBackendStablePath(pool, devpath, true);
>>           VIR_FREE(devpath);
>>           if (vol->target.path == NULL)
>>               return -1;
>> diff --git a/src/storage/storage_backend_scsi.c
>> b/src/storage/storage_backend_scsi.c
>> index 27dcbb6..4e832eb 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -246,7 +246,8 @@ virStorageBackendSCSINewLun(virStoragePoolObjPtr pool,
>>        * way of doing this...
>>        */
>>       if ((vol->target.path = virStorageBackendStablePath(pool,
>> -                                                        devpath)) == NULL) {
>> +                                                        devpath,
>> +                                                        true)) == NULL) {
>>           retval = -1;
>>           goto free_vol;
>>       }
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 28829d3..4fbc0c0 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -1318,7 +1318,8 @@ storageVolumeLookupByPath(virConnectPtr conn,
>>               const char *stable_path;
>>
>>               stable_path = virStorageBackendStablePath(driver->pools.objs[i],
>> -                                                      cleanpath);
>> +                                                      cleanpath,
>> +                                                      false);
>>               if (stable_path == NULL) {
>>                   /* Don't break the whole lookup process if it fails on
>>                    * getting the stable path for some of the pools.
> 
> Makes sense. ACK

Thanks Osier, pushed now.

- Cole