[libvirt] [PATCH v2 0/5] delete volumes while it is building
This is the version 2 of patches for deleting volume while it is being built. based on the first version: https://www.redhat.com/archives/libvir-list/2011-July/msg00535.html. The patches is an attempt to spawn a new thread for volume building in storageVolumeCreateXML(), then, the initial thread is waiting for the cancelling signal from storageVolumeDelete(). When it gets the signal, it uses pthread_cancel() to stop the process of volume building, then tell storageVolumeDelete() to continue its job. I go through the codes in other parts of libvirt briefly, such as nodedevice, network. It is rare to spawn a new thread, so this worth a shot or probably not. Any idea is appreciated.
--- src/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 54b1ca0..90c4393 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -878,7 +878,7 @@ libvirt_driver_storage_la_SOURCES = libvirt_driver_storage_la_CFLAGS = \ -I@top_srcdir@/src/conf $(AM_CFLAGS) libvirt_driver_storage_la_LDFLAGS = $(AM_LDFLAGS) -libvirt_driver_storage_la_LIBADD = +libvirt_driver_storage_la_LIBADD = libvirt_util.la if WITH_SECDRIVER_SELINUX libvirt_driver_storage_la_LIBADD += $(SELINUX_LIBS) endif -- 1.7.1
On Sun, Jul 17, 2011 at 06:44:57PM +0800, Guannan Ren wrote:
--- src/Makefile.am | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am index 54b1ca0..90c4393 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -878,7 +878,7 @@ libvirt_driver_storage_la_SOURCES = libvirt_driver_storage_la_CFLAGS = \ -I@top_srcdir@/src/conf $(AM_CFLAGS) libvirt_driver_storage_la_LDFLAGS = $(AM_LDFLAGS) -libvirt_driver_storage_la_LIBADD = +libvirt_driver_storage_la_LIBADD = libvirt_util.la if WITH_SECDRIVER_SELINUX libvirt_driver_storage_la_LIBADD += $(SELINUX_LIBS) endif
NACK, this is not allowed. It sounds like you need to add another export to src/libvirt_private.syms 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 :|
--- src/conf/storage_conf.c | 10 ++++++++++ src/conf/storage_conf.h | 2 ++ 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index cc55b80..093b760 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -323,6 +323,7 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj) { VIR_FREE(obj->autostartLink); virMutexDestroy(&obj->lock); + ignore_value(virCondDestroy(&obj->cond)); VIR_FREE(obj); } @@ -1391,6 +1392,15 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, VIR_FREE(pool); return NULL; } + + if (virCondInit(&pool->cond) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot initialize cond")); + virMutexDestroy(&pool->lock); + VIR_FREE(pool); + return NULL; + } + virStoragePoolObjLock(pool); pool->active = 0; pool->def = def; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 271441a..60a1a52 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -275,12 +275,14 @@ typedef virStoragePoolObj *virStoragePoolObjPtr; struct _virStoragePoolObj { virMutex lock; + virCond cond; char *configFile; char *autostartLink; int active; int autostart; unsigned int asyncjobs; + unsigned int jobSignals; virStoragePoolDefPtr def; virStoragePoolDefPtr newDef; -- 1.7.1
--- src/storage/storage_backend.h | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 67ac32c..d713b6d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -62,6 +62,17 @@ virStorageBackendFSImageToolTypeToFunc(int tool_type); typedef struct _virStorageBackend virStorageBackend; typedef virStorageBackend *virStorageBackendPtr; +typedef struct _volBuildThread volBuildThread; +typedef volBuildThread *volBuildThreadPtr; +struct _volBuildThread { + virStorageBackendBuildVol buildvol; + virStoragePoolPtr obj; + virStoragePoolObjPtr pool; + virStorageVolDefPtr vol; + unsigned threadEnd : 1; + int buildret; +}; + struct _virStorageBackend { int type; @@ -96,6 +107,10 @@ enum { VIR_STORAGE_VOL_OPEN_DIR = 1 << 4, /* directories okay */ }; +enum virStorageJobSignals { + VOL_JOB_SIGNAL_CANCEL = 1 << 0, +}; + # define VIR_STORAGE_VOL_OPEN_DEFAULT (VIR_STORAGE_VOL_OPEN_ERROR |\ VIR_STORAGE_VOL_OPEN_REG |\ VIR_STORAGE_VOL_OPEN_CHAR |\ @@ -148,5 +163,5 @@ int virStorageBackendRunProgNul(virStoragePoolObjPtr pool, virStorageBackendListVolNulFunc func, void *data); - +void virStorageBackendVoluCleanup(void *argv); #endif /* __VIR_STORAGE_BACKEND_H__ */ -- 1.7.1
--- src/storage/storage_backend.c | 9 ++++ src/storage/storage_driver.c | 83 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f632edd..bc10933 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1632,3 +1632,12 @@ virStorageBackendRunProgNul(virConnectPtr conn, return -1; } #endif /* WIN32 */ + +void virStorageBackendVoluCleanup(void *arg) +{ + + volBuildThreadPtr data = arg; + + data->buildret = 0; + data->threadEnd = 1; +} diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 997b876..d8ac648 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -48,6 +48,7 @@ #include "files.h" #include "fdstream.h" #include "configmake.h" +#include "threads.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1276,6 +1277,29 @@ cleanup: static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags); +static void virStorageBuildVol(void *arg) +{ + int ret = -1; + volBuildThreadPtr data = arg; + virStoragePoolObjPtr pool = data->pool; + virStorageVolDefPtr vol = data->vol; + + pthread_cleanup_push(virStorageBackendVoluCleanup, data); + + ret = data->buildvol(data->obj->conn, pool, vol); + + pthread_cleanup_pop(0); + + data->buildret = ret; + data->threadEnd = 1; +} + +static int +virStorageProcessJobSignals(virThreadPtr th) +{ + return virThreadCancel(th); +} + static virStorageVolPtr storageVolumeCreateXML(virStoragePoolPtr obj, const char *xmldesc, @@ -1353,28 +1377,63 @@ storageVolumeCreateXML(virStoragePoolPtr obj, goto cleanup; } + volBuildThreadPtr volbuild = NULL; + if (VIR_ALLOC(volbuild) < 0) { + virReportOOMError(); + goto cleanup; + } + /* Make a shallow copy of the 'defined' volume definition, since the * original allocation value will change as the user polls 'info', * but we only need the initial requested values */ memcpy(buildvoldef, voldef, sizeof(*voldef)); - /* Drop the pool lock during volume allocation */ pool->asyncjobs++; voldef->building = 1; - virStoragePoolObjUnlock(pool); + virThread th; + volbuild->buildvol = backend->buildVol; + volbuild->obj = obj; + volbuild->pool = pool; + volbuild->vol = buildvoldef; + volbuild->buildret = -1; + + if (virThreadCreate(&th, true, + virStorageBuildVol, + volbuild) < 0){ + virReportSystemError(errno, "%s", + _("Unable to create migration thread")); + goto buildingend; + + } - buildret = backend->buildVol(obj->conn, pool, buildvoldef); + while(!volbuild->threadEnd){ + if(pool->jobSignals){ + if(virStorageProcessJobSignals(&th) < 0) + goto buildingend; + } + + virStoragePoolObjUnlock(pool); + + storageDriverLock(driver); + virStoragePoolObjLock(pool); + storageDriverUnlock(driver); + } - storageDriverLock(driver); - virStoragePoolObjLock(pool); - storageDriverUnlock(driver); + virThreadJoin(&th); + + buildingend: + buildret = volbuild->buildret; + + pool->jobSignals = 0; voldef->building = 0; + virCondBroadcast(&pool->cond); pool->asyncjobs--; voldef = NULL; VIR_FREE(buildvoldef); + VIR_FREE(volbuild); if (buildret < 0) { virStoragePoolObjUnlock(pool); @@ -1936,10 +1995,14 @@ storageVolumeDelete(virStorageVolPtr obj, } if (vol->building) { - virStorageReportError(VIR_ERR_OPERATION_INVALID, - _("volume '%s' is still being allocated."), - vol->name); - goto cleanup; + pool->jobSignals |= VOL_JOB_SIGNAL_CANCEL; + while(vol->building){ + if(virCondWait(&pool->cond, &pool->lock) < 0){ + virStorageReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to wait on storage condition")); + goto cleanup; + } + } } if (!backend->deleteVol) { -- 1.7.1
On Sun, Jul 17, 2011 at 06:45:00PM +0800, Guannan Ren wrote:
--- src/storage/storage_backend.c | 9 ++++ src/storage/storage_driver.c | 83 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f632edd..bc10933 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1632,3 +1632,12 @@ virStorageBackendRunProgNul(virConnectPtr conn, return -1; } #endif /* WIN32 */ + +void virStorageBackendVoluCleanup(void *arg) +{ + + volBuildThreadPtr data = arg; + + data->buildret = 0; + data->threadEnd = 1; +} diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 997b876..d8ac648 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -48,6 +48,7 @@ #include "files.h" #include "fdstream.h" #include "configmake.h" +#include "threads.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -1276,6 +1277,29 @@ cleanup:
static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags);
+static void virStorageBuildVol(void *arg) +{ + int ret = -1; + volBuildThreadPtr data = arg; + virStoragePoolObjPtr pool = data->pool; + virStorageVolDefPtr vol = data->vol; + + pthread_cleanup_push(virStorageBackendVoluCleanup, data); + + ret = data->buildvol(data->obj->conn, pool, vol); + + pthread_cleanup_pop(0);
NACK, all use of pthread specific APIs must be in src/util/threads-pthread.c Independantly of this, IMHO pthread cancellation handlers are a recipe for trouble because it is incredibly hard to make sure you correctly cleanup all resources in the thread, even with use of cleanup handlers. IMHO, threads should be made to monitor some external "quit" boolean variable (eg see threadpool.c thread termination). 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 :|
On 07/18/2011 06:57 PM, Daniel P. Berrange wrote:
On Sun, Jul 17, 2011 at 06:45:00PM +0800, Guannan Ren wrote:
--- src/storage/storage_backend.c | 9 ++++ src/storage/storage_driver.c | 83 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 82 insertions(+), 10 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index f632edd..bc10933 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1632,3 +1632,12 @@ virStorageBackendRunProgNul(virConnectPtr conn, return -1; } #endif /* WIN32 */ + +void virStorageBackendVoluCleanup(void *arg) +{ + + volBuildThreadPtr data = arg; + + data->buildret = 0; + data->threadEnd = 1; +} diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 997b876..d8ac648 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -48,6 +48,7 @@ #include "files.h" #include "fdstream.h" #include "configmake.h" +#include "threads.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -1276,6 +1277,29 @@ cleanup:
static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags);
+static void virStorageBuildVol(void *arg) +{ + int ret = -1; + volBuildThreadPtr data = arg; + virStoragePoolObjPtr pool = data->pool; + virStorageVolDefPtr vol = data->vol; + + pthread_cleanup_push(virStorageBackendVoluCleanup, data); + + ret = data->buildvol(data->obj->conn, pool, vol); + + pthread_cleanup_pop(0); NACK, all use of pthread specific APIs must be in src/util/threads-pthread.c
Independantly of this, IMHO pthread cancellation handlers are a recipe for trouble because it is incredibly hard to make sure you correctly cleanup all resources in the thread, even with use of cleanup handlers.
IMHO, threads should be made to monitor some external "quit" boolean variable (eg see threadpool.c thread termination).
Daniel yep but the push and pop function couldn't be in different function, according to man page as follows. It is dangerous to use thread handler, I agree with you. Thanks.
"These functions may be implemented as macros. The application shall ensure that they appear as statements, and in pairs within the same lexical scope (that is, the pthread_cleanup_push() macro may be thought to expand to a token list whose first token is ’{’ with pthread_cleanup_pop() expanding to a token list whose last token is the corresponding ’}’ ). "
On 07/18/2011 05:13 AM, Guannan Ren wrote:
Independantly of this, IMHO pthread cancellation handlers are a recipe for trouble because it is incredibly hard to make sure you correctly cleanup all resources in the thread, even with use of cleanup handlers.
IMHO, threads should be made to monitor some external "quit" boolean variable (eg see threadpool.c thread termination).
Daniel yep but the push and pop function couldn't be in different function, according to man page as follows.
The concept of pushing and popping handlers isn't the bottleneck here, rather, it is that _all_ functions in the callstack must reliably use the push/pop handlers correctly, before _any_ function can call pthread_cancel on the entire callstack. Right now, none of our code uses any push/pop handlers. Therefore, it becomes a much larger patch to properly insert push/pop handlers into the entire call chain than it is to just write the worker thread to periodically check if any other thread has requested an early quit. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
--- src/util/threads-pthread.c | 9 +++++++++ src/util/threads.h | 2 ++ 2 files changed, 11 insertions(+), 0 deletions(-) diff --git a/src/util/threads-pthread.c b/src/util/threads-pthread.c index 82ce5c6..791c5a5 100644 --- a/src/util/threads-pthread.c +++ b/src/util/threads-pthread.c @@ -253,3 +253,12 @@ void virThreadLocalSet(virThreadLocalPtr l, void *val) { pthread_setspecific(l->key, val); } + +int virThreadCancel(virThreadPtr thread) +{ + int ret; + if ((ret = pthread_cancel(thread->thread)) != 0){ + return -1; + } + return 0; +} diff --git a/src/util/threads.h b/src/util/threads.h index b72610c..dd456b5 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -45,6 +45,7 @@ int virThreadInitialize(void) ATTRIBUTE_RETURN_CHECK; void virThreadOnExit(void); typedef void (*virThreadFunc)(void *opaque); +typedef void (*virThreadCleanupFunc)(void *opaque); int virThreadCreate(virThreadPtr thread, bool joinable, @@ -99,6 +100,7 @@ int virThreadLocalInit(virThreadLocalPtr l, virThreadLocalCleanup c) ATTRIBUTE_RETURN_CHECK; void *virThreadLocalGet(virThreadLocalPtr l); void virThreadLocalSet(virThreadLocalPtr l, void*); +int virThreadCancel(virThreadPtr thread); # ifdef WIN32 # include "threads-win32.h" -- 1.7.1
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Guannan Ren