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

[Peter Krempa]

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 884 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140729/695a7588/attachment-0001.sig>