On Tue, Jul 29, 2014 at 10:12:00AM -0400, 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.
It would affect risk of deadlock wrt other threads, but regardless of
how long the lock is held in this codepath, we've got the same potential
for self-deadlock, as well as the performance problem I mention above.
> 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...
No, you mis-understand what I mean. Don't change the common stream code
at all. Simply change 'virStorageVolFDStreamCloseCb' so that it calls
virThreadCreate(), and everything currently in virStorageVolFDStreamCloseCb
gets moved into your thread's main func.
Regards,
Daniel
--
|:
http://berrange.com -o-
http://www.flickr.com/photos/dberrange/ :|
|:
http://libvirt.org -o-
http://virt-manager.org :|
|:
http://autobuild.org -o-
http://search.cpan.org/~danberr/ :|
|:
http://entangle-photo.org -o-
http://live.gnome.org/gtk-vnc :|