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