Re: [libvirt] [PATCH] storage: Refresh storage pool after upload

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(a)redhat.com&gt; >> --- >> 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