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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]