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

[John Ferlan]

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?

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().

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
>