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@xxxxxxxxxx> >>>> --- >>>> 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list >
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list