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

[Osier Yang]

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