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@xxxxxxxxxx> > >>> --- > >>> 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list