[libvirt] [PATCH 0/4] Fix crash resuling from script doing FC/SCSI pool-create destroy
Details in the patches - essentially testing found an issue creating and destroying an FC/SCSI pool due to the refresh thread created for bz 1152382 running after the destroy. Seen using script which has for i in 1 2 3 ; \ do virsh pool-create /tmp/test.xml; \ sleep 1; \ virsh pool-destroy p1; \ sleep 1; \ done John Ferlan (4): storage: Make active boolean storage: Change cbdata scsi refresh thread field name storage: Introduce virStoragePoolObjFindPoolByUUID storage: Don't assume storage pool exists for scsi refresh thread src/conf/storage_conf.h | 2 +- src/storage/storage_backend_scsi.c | 30 +++++++++++++++++------------- src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++----- src/storage/storage_driver.h | 3 +++ 4 files changed, 47 insertions(+), 19 deletions(-) -- 2.1.0
Since we treat it like a boolean, let's store it that way. At least one path had already treated as true/false anyway. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.h | 2 +- src/storage/storage_driver.c | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 7471006..ec59c17 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -266,7 +266,7 @@ struct _virStoragePoolObj { char *configFile; char *autostartLink; - int active; + bool active; int autostart; unsigned int asyncjobs; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ee8b263..aeb67a7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -205,7 +205,7 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool); continue; } - pool->active = 1; + pool->active = true; } virStoragePoolObjUnlock(pool); } @@ -715,7 +715,7 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; } VIR_INFO("Creating storage pool '%s'", pool->def->name); - pool->active = 1; + pool->active = true; ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, NULL, NULL); @@ -881,7 +881,7 @@ storagePoolCreate(virStoragePoolPtr obj, goto cleanup; } - pool->active = 1; + pool->active = true; ret = 0; cleanup: @@ -978,7 +978,7 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool); - pool->active = 0; + pool->active = false; if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -1100,7 +1100,7 @@ storagePoolRefresh(virStoragePoolPtr obj, if (backend->stopPool) backend->stopPool(obj->conn, pool); - pool->active = 0; + pool->active = false; if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); -- 2.1.0
Change the field name from 'name' to 'fchost_name' to better id it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index a593a2b..f246a05 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -43,7 +43,7 @@ VIR_LOG_INIT("storage.storage_backend_scsi"); typedef struct _virStoragePoolFCRefreshInfo virStoragePoolFCRefreshInfo; typedef virStoragePoolFCRefreshInfo *virStoragePoolFCRefreshInfoPtr; struct _virStoragePoolFCRefreshInfo { - char *name; + char *fchost_name; virStoragePoolObjPtr pool; }; @@ -565,7 +565,7 @@ virStoragePoolFCRefreshDataFree(void *opaque) { virStoragePoolFCRefreshInfoPtr cbdata = opaque; - VIR_FREE(cbdata->name); + VIR_FREE(cbdata->fchost_name); VIR_FREE(cbdata); } @@ -593,7 +593,7 @@ static void virStoragePoolFCRefreshThread(void *opaque) { virStoragePoolFCRefreshInfoPtr cbdata = opaque; - const char *name = cbdata->name; + const char *fchost_name = cbdata->fchost_name; virStoragePoolObjPtr pool = cbdata->pool; unsigned int host; int found = 0; @@ -606,10 +606,10 @@ virStoragePoolFCRefreshThread(void *opaque) * rescan, and find LUN's, then we are happy */ VIR_DEBUG("Attempt FC Refresh for pool='%s' name='%s' tries='%d'", - pool->def->name, name, tries); + pool->def->name, fchost_name, tries); virStoragePoolObjLock(pool); if (virStoragePoolObjIsActive(pool) && - virGetSCSIHostNumber(name, &host) == 0 && + virGetSCSIHostNumber(fchost_name, &host) == 0 && virStorageBackendSCSITriggerRescan(host) == 0) { virStoragePoolObjClearVols(pool); found = virStorageBackendSCSIFindLUs(pool, host); @@ -799,7 +799,7 @@ createVport(virConnectPtr conn, adapter->data.fchost.wwpn))) { if (VIR_ALLOC(cbdata) == 0) { cbdata->pool = pool; - cbdata->name = name; + cbdata->fchost_name = name; name = NULL; if (virThreadCreate(&thread, false, virStoragePoolFCRefreshThread, -- 2.1.0
Add a new API to search the currently defined pool list for a pool with a matching UUID and return the locked pool object pointer. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 21 +++++++++++++++++++++ src/storage/storage_driver.h | 3 +++ 2 files changed, 24 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index aeb67a7..bbf21f6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3370,3 +3370,24 @@ virStorageTranslateDiskSourcePool(virConnectPtr conn, virStoragePoolDefFree(pooldef); return ret; } + + +/* + * virStoragePoolObjFindPoolByUUID + * @uuid: The uuid to lookup + * + * Using the passed @uuid, search the driver pools for a matching uuid. + * If found, then lock the pool + * + * Returns NULL if pool is not found or a locked pool object pointer + */ +virStoragePoolObjPtr +virStoragePoolObjFindPoolByUUID(const unsigned char *uuid) +{ + virStoragePoolObjPtr pool; + + storageDriverLock(); + pool = virStoragePoolObjFindByUUID(&driver->pools, uuid); + storageDriverUnlock(); + return pool; +} diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 4f28dc1..912c232 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -57,6 +57,9 @@ int virStorageFileGetMetadata(virStorageSourcePtr src, int virStorageTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def); +virStoragePoolObjPtr virStoragePoolObjFindPoolByUUID(const unsigned char *uuid) + ATTRIBUTE_NONNULL(1); + virStoragePoolPtr storagePoolLookupByTargetPath(virConnectPtr conn, const char *path) -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1277781 The virStoragePoolFCRefreshThread had passed a pointer to the pool obj in the virStoragePoolFCRefreshInfoPtr; however, we cannot assume that the pool exists still since we don't keep the pool lock throughout the duration of the thread. Therefore, instead of passing the pool obj pointer, pass the UUID of the pool and perform a lookup. If found, then we can perform the refresh using the locked pool obj pointer; otherwise, we just exit the thread since the pool is now gone. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_scsi.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index f246a05..7dd7674 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -44,7 +44,7 @@ typedef struct _virStoragePoolFCRefreshInfo virStoragePoolFCRefreshInfo; typedef virStoragePoolFCRefreshInfo *virStoragePoolFCRefreshInfoPtr; struct _virStoragePoolFCRefreshInfo { char *fchost_name; - virStoragePoolObjPtr pool; + unsigned char pool_uuid[VIR_UUID_BUFLEN]; }; /* Function to check if the type file in the given sysfs_path is a @@ -594,7 +594,8 @@ virStoragePoolFCRefreshThread(void *opaque) { virStoragePoolFCRefreshInfoPtr cbdata = opaque; const char *fchost_name = cbdata->fchost_name; - virStoragePoolObjPtr pool = cbdata->pool; + const unsigned char *pool_uuid = cbdata->pool_uuid; + virStoragePoolObjPtr pool = NULL; unsigned int host; int found = 0; int tries = 2; @@ -602,12 +603,15 @@ virStoragePoolFCRefreshThread(void *opaque) do { sleep(5); /* Give it time */ - /* Lock the pool, if active, we can get the host number, successfully - * rescan, and find LUN's, then we are happy + /* Let's see if the pool still exists - */ + if (!(pool = virStoragePoolObjFindPoolByUUID(pool_uuid))) + break; + + /* Return with pool lock, if active, we can get the host number, + * successfully, rescan, and find LUN's, then we are happy */ VIR_DEBUG("Attempt FC Refresh for pool='%s' name='%s' tries='%d'", pool->def->name, fchost_name, tries); - virStoragePoolObjLock(pool); if (virStoragePoolObjIsActive(pool) && virGetSCSIHostNumber(fchost_name, &host) == 0 && virStorageBackendSCSITriggerRescan(host) == 0) { @@ -617,7 +621,7 @@ virStoragePoolFCRefreshThread(void *opaque) virStoragePoolObjUnlock(pool); } while (!found && --tries); - if (!found) + if (pool && !found) VIR_DEBUG("FC Refresh Thread failed to find LU's"); virStoragePoolFCRefreshDataFree(cbdata); @@ -798,7 +802,7 @@ createVport(virConnectPtr conn, if ((name = virGetFCHostNameByWWN(NULL, adapter->data.fchost.wwnn, adapter->data.fchost.wwpn))) { if (VIR_ALLOC(cbdata) == 0) { - cbdata->pool = pool; + memcpy(cbdata->pool_uuid, pool->def->uuid, VIR_UUID_BUFLEN); cbdata->fchost_name = name; name = NULL; -- 2.1.0
On Wed, Nov 04, 2015 at 04:58:45PM -0500, John Ferlan wrote:
Details in the patches - essentially testing found an issue creating and destroying an FC/SCSI pool due to the refresh thread created for bz 1152382 running after the destroy.
Seen using script which has for i in 1 2 3 ; \ do virsh pool-create /tmp/test.xml; \ sleep 1; \ virsh pool-destroy p1; \ sleep 1; \ done
John Ferlan (4): storage: Make active boolean storage: Change cbdata scsi refresh thread field name storage: Introduce virStoragePoolObjFindPoolByUUID storage: Don't assume storage pool exists for scsi refresh thread
src/conf/storage_conf.h | 2 +- src/storage/storage_backend_scsi.c | 30 +++++++++++++++++------------- src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++----- src/storage/storage_driver.h | 3 +++ 4 files changed, 47 insertions(+), 19 deletions(-)
ACK series, Pavel
participants (2)
-
John Ferlan -
Pavel Hrdina