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

[Peter Krempa]

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 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.
>>
>>> +    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 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/d107a69b/attachment-0001.sig>