[libvirt] [PATCH] storage: Refresh storage pool after upload
https://bugzilla.redhat.com/show_bug.cgi?id=1072653 Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb; # libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState; static int storageStateCleanup(void); +typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, } +/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState); + if (!(pool = virStoragePoolObjFindByName(&driverState->pools, + cbdata->pool_name))) + goto cleanup; + + if (!(backend = virStorageBackendForType(pool->def->type))) + goto cleanup; + + virStoragePoolObjClearVols(pool); + if (backend->refreshPool(NULL, pool) < 0) + VIR_DEBUG("Failed to refresh storage pool"); + + cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driverState); +} + static int storageVolUpload(virStorageVolPtr obj, virStreamPtr stream, @@ -1966,6 +2018,7 @@ storageVolUpload(virStorageVolPtr obj, virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; + virStorageVolStreamInfoPtr cbdata = NULL; int ret = -1; virCheckFlags(0, -1); @@ -1996,11 +2049,36 @@ storageVolUpload(virStorageVolPtr obj, goto cleanup; } + /* If we have a refreshPool, use the callback routine in order to + * refresh the pool after the volume upload stream closes. This way + * we make sure the volume and pool data are refreshed without user + * interaction and we can just lookup the backend in the callback + * routine in order to call the refresh API. + */ + if (backend->refreshPool) { + if (VIR_ALLOC(cbdata) < 0 || + VIR_STRDUP(cbdata->pool_name, pool->def->name) < 0) + goto cleanup; + } + ret = backend->uploadVol(obj->conn, pool, vol, stream, offset, length, flags); + /* Add cleanup callback - call after uploadVol since the stream + * is then fully set up + */ + if (cbdata) { + virFDStreamSetInternalCloseCb(stream, + virStorageVolFDStreamCloseCb, + cbdata, + virStorageVolFDStreamCloseCbFree); + cbdata = NULL; + } + cleanup: virStoragePoolObjUnlock(pool); + if (cbdata) + virStorageVolFDStreamCloseCbFree(cbdata); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 849ae31..e377df9 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2981,6 +2981,9 @@ of the amount of data to be uploaded. A negative value is interpreted as an unsigned long long value to essentially include everything from the offset to the end of the volume. An error will occur if the I<local-file> is greater than the specified length. +See the description for the libvirt virStorageVolUpload API for details +regarding possible target volume and pool changes as a result of the +pool refresh when the upload is attempted. =item B<vol-download> [I<--pool> I<pool-or-uuid>] [I<--offset> I<bytes>] [I<--length> I<bytes>] I<vol-name-or-key-or-path> I<local-file> -- 1.9.3
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
+ if (!(pool = virStoragePoolObjFindByName(&driverState->pools, + cbdata->pool_name))) + goto cleanup; + + if (!(backend = virStorageBackendForType(pool->def->type))) + goto cleanup; + + virStoragePoolObjClearVols(pool);
Hmmm, having a single volume update func would be nice here.
+ if (backend->refreshPool(NULL, pool) < 0) + VIR_DEBUG("Failed to refresh storage pool"); + + cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driverState); +} + static int storageVolUpload(virStorageVolPtr obj, virStreamPtr stream,
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it. Peter
On Tue, Jul 29, 2014 at 03:28:33PM +0200, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
The risk would be that some part of the storage driver holds the lock, and needs to wait on some event from the event loop. That could lead to a deadlock situation. In fact we could deadlock ourselves if the 'refreshPool' function uses the event loop, which is not entirely unlikely. Also, refreshing the storage pool size can be a somewhat heavy (ie slow) operation for some of the storage backends. I think I'd be inclined to say that the stream close callback should spawn a throw-away thread to do the refresh work. This avoids the deadlock risk and avoids blocking the event loop for prolonged time.
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.
I think we need to use a thread before pushing this[ Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/29/2014 09:39 AM, Daniel P. Berrange wrote:
On Tue, Jul 29, 2014 at 03:28:33PM +0200, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
The risk would be that some part of the storage driver holds the lock, and needs to wait on some event from the event loop. That could lead to a deadlock situation. In fact we could deadlock ourselves if the 'refreshPool' function uses the event loop, which is not entirely unlikely. Also, refreshing the storage pool size can be a somewhat heavy (ie slow) operation for some of the storage backends.
Would narrowing the window to just the virStoragePoolObjFindByName() work better? Don't think we need the lock once we have the pool by the passed name.
I think I'd be inclined to say that the stream close callback should spawn a throw-away thread to do the refresh work. This avoids the deadlock risk and avoids blocking the event loop for prolonged time.
The only other "consumer" of this stream close path is virChrdevOpen() and I believe that was for console support. So, by adding a throw-away thread that console code is impacted - something I'm hoping to avoid... John
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.
I think we need to use a thread before pushing this[
Regards, Daniel
On 07/29/14 16:12, John Ferlan wrote:
On 07/29/2014 09:39 AM, Daniel P. Berrange wrote:
On Tue, Jul 29, 2014 at 03:28:33PM +0200, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
The risk would be that some part of the storage driver holds the lock, and needs to wait on some event from the event loop. That could lead to a deadlock situation. In fact we could deadlock ourselves if the 'refreshPool' function uses the event loop, which is not entirely unlikely. Also, refreshing the storage pool size can be a somewhat heavy (ie slow) operation for some of the storage backends.
Would narrowing the window to just the virStoragePoolObjFindByName() work better? Don't think we need the lock once we have the pool by the passed name.
I think I'd be inclined to say that the stream close callback should spawn a throw-away thread to do the refresh work. This avoids the deadlock risk and avoids blocking the event loop for prolonged time.
The only other "consumer" of this stream close path is virChrdevOpen() and I believe that was for console support. So, by adding a throw-away thread that console code is impacted - something I'm hoping to avoid...
Well, you could spawn the thread in the handler you've introduced thus the console code would remain un-impacted, while the "long-running" update of the pool data would be processed in the thread.
John
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.
I think we need to use a thread before pushing this[
Regards, Daniel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 29, 2014 at 10:12:00AM -0400, John Ferlan wrote:
On 07/29/2014 09:39 AM, Daniel P. Berrange wrote:
On Tue, Jul 29, 2014 at 03:28:33PM +0200, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
The risk would be that some part of the storage driver holds the lock, and needs to wait on some event from the event loop. That could lead to a deadlock situation. In fact we could deadlock ourselves if the 'refreshPool' function uses the event loop, which is not entirely unlikely. Also, refreshing the storage pool size can be a somewhat heavy (ie slow) operation for some of the storage backends.
Would narrowing the window to just the virStoragePoolObjFindByName() work better? Don't think we need the lock once we have the pool by the passed name.
It would affect risk of deadlock wrt other threads, but regardless of how long the lock is held in this codepath, we've got the same potential for self-deadlock, as well as the performance problem I mention above.
I think I'd be inclined to say that the stream close callback should spawn a throw-away thread to do the refresh work. This avoids the deadlock risk and avoids blocking the event loop for prolonged time.
The only other "consumer" of this stream close path is virChrdevOpen() and I believe that was for console support. So, by adding a throw-away thread that console code is impacted - something I'm hoping to avoid...
No, you mis-understand what I mean. Don't change the common stream code at all. Simply change 'virStorageVolFDStreamCloseCb' so that it calls virThreadCreate(), and everything currently in virStorageVolFDStreamCloseCb gets moved into your thread's main func. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/29/2014 09:28 AM, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
+ if (!(pool = virStoragePoolObjFindByName(&driverState->pools, + cbdata->pool_name))) + goto cleanup; + + if (!(backend = virStorageBackendForType(pool->def->type))) + goto cleanup; + + virStoragePoolObjClearVols(pool);
Hmmm, having a single volume update func would be nice here.
Right, but a change to the volume could mean the pool data is off (allocation, capacity), right? or is that calculated "on the fly" as a result of each volume? It's possible to pass the volume name along through here as well and I did so for my initial iterations; however, refreshVol() is implemented in far less storage pools than refreshPool(). John
+ if (backend->refreshPool(NULL, pool) < 0) + VIR_DEBUG("Failed to refresh storage pool"); + + cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driverState); +} + static int storageVolUpload(virStorageVolPtr obj, virStreamPtr stream,
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.
Peter
On 07/29/14 16:05, John Ferlan wrote:
On 07/29/2014 09:28 AM, Peter Krempa wrote:
On 07/28/14 15:24, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1072653
Upon successful upload of a volume, the target volume and storage pool were not updated to reflect any changes as a result of the upload. Make use of the existing stream close callback mechanism to force a backend pool refresh to occur once the stream closes.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt.c | 8 +++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 78 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 3 ++ 4 files changed, 90 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 143d319..992e4f2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -13960,6 +13960,14 @@ virStorageVolDownload(virStorageVolPtr vol, * detect any errors. The results will be unpredictable if * another active stream is writing to the storage volume. * + * When the data stream is closed whether the upload is successful + * or not the target storage pool will be refreshed to reflect pool + * and volume changes as a result of the upload. Depending on + * the target volume storage backend and the source stream type + * for a successful upload, the target volume may take on the + * characteristics from the source stream such as format type, + * capacity, and allocation. + * * Returns 0, or -1 upon error. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b1fb7c9..7ef68a1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virFDStreamCreateFile; virFDStreamOpen; virFDStreamOpenFile; virFDStreamOpenPTY; +virFDStreamSetInternalCloseCb;
# libvirt_internal.h diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index efbe5ff..5fd5514 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -59,6 +59,12 @@ static virStorageDriverStatePtr driverState;
static int storageStateCleanup(void);
+typedef struct _virStorageVolStreamInfo virStorageVolStreamInfo; +typedef virStorageVolStreamInfo *virStorageVolStreamInfoPtr; +struct _virStorageVolStreamInfo { + char *pool_name; +}; + static void storageDriverLock(virStorageDriverStatePtr driver) { virMutexLock(&driver->lock); @@ -1956,6 +1962,52 @@ storageVolDownload(virStorageVolPtr obj, }
+/** + * Frees opaque data provided for the stream closing callback + * + * @opaque Data to be freed. + */ +static void virStorageVolFDStreamCloseCbFree(void *opaque) +{ + virStorageVolStreamInfoPtr cbdata = opaque; + + VIR_FREE(cbdata->pool_name); + VIR_FREE(cbdata); +} + +/** + * Callback being called if a FDstream is closed. Frees device entries + * from data structures and removes lockfiles. + * + * @st Pointer to stream being closed. + * @opaque Domain's device information structure. + */ +static void virStorageVolFDStreamCloseCb(virStreamPtr st ATTRIBUTE_UNUSED, + void *opaque) +{ + + virStorageVolStreamInfoPtr cbdata = opaque; + virStoragePoolObjPtr pool = NULL; + virStorageBackendPtr backend; + + storageDriverLock(driverState);
I'm a bit concerned about locking the storage driver here. This code will be called by the stream close callback which is handled from the event loop. While I don't currently see any problem, this instance could bite us in the future.
+ if (!(pool = virStoragePoolObjFindByName(&driverState->pools, + cbdata->pool_name))) + goto cleanup; + + if (!(backend = virStorageBackendForType(pool->def->type))) + goto cleanup; + + virStoragePoolObjClearVols(pool);
Hmmm, having a single volume update func would be nice here.
Right, but a change to the volume could mean the pool data is off (allocation, capacity), right? or is that calculated "on the fly" as a result of each volume?
AFAIK that is refreshed before retrieval as running VMs with storage in the pool may change that anyways.
It's possible to pass the volume name along through here as well and I did so for my initial iterations; however, refreshVol() is implemented in far less storage pools than refreshPool().
Hmm by using the thread as suggested by Dan you still may use refreshPool even if it does a ton of stuff.
John
+ if (backend->refreshPool(NULL, pool) < 0) + VIR_DEBUG("Failed to refresh storage pool"); + + cleanup: + if (pool) + virStoragePoolObjUnlock(pool); + storageDriverUnlock(driverState); +} + static int storageVolUpload(virStorageVolPtr obj, virStreamPtr stream,
ACK, but please push this after the release so that we can give it a cycle of testing before releasing it.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa