[libvirt] [PATCH] storage: Don't do wait loops from VolLookupByPath
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. -- 1.7.11.7
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
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
participants (2)
-
Cole Robinson -
Osier Yang