9:27 a.m.
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list