3:58 p.m.
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
3:58 p.m.
New subject: [libvirt] [PATCH 1/4] storage: Make active boolean
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(a)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
3:58 p.m.
New subject: [libvirt] [PATCH 2/4] storage: Change cbdata scsi refresh thread field name
Change the field name from 'name' to 'fchost_name' to better id it.
Signed-off-by: John Ferlan <jferlan(a)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
3:58 p.m.
New subject: [libvirt] [PATCH 3/4] storage: Introduce virStoragePoolObjFindPoolByUUID
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(a)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
3:58 p.m.
New subject: [libvirt] [PATCH 4/4] storage: Don't assume storage pool exists for scsi refresh thread
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(a)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
6:29 a.m.
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
3297
days inactive
3306
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Pavel Hrdina