From 2cd9b2d8eed84c8a9de86a881ee60fbd15176562 Mon Sep 17 00:00:00 2001 From: Daniel Veillard Date: Fri, 17 Apr 2009 19:12:37 +0000 Subject: [PATCH] drop the pool lock when allocating fs volumes * src/libvirt_private.syms src/storage_backend.h src/storage_backend_fs.c src/storage_conf.h src/storage_driver.c: drop the pool lock when allocating fs volumes, patch by Cole Robinson daniel --- ChangeLog | 6 +++ src/libvirt_private.syms | 1 + src/storage_backend.h | 2 + src/storage_backend_fs.c | 54 +++++++++++-------- src/storage_conf.h | 3 ++ src/storage_driver.c | 111 ++++++++++++++++++++++++++++++++++----- 6 files changed, 142 insertions(+), 35 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6acc36eed6..daaab1b6eb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Fri Apr 17 21:10:28 CEST 2009 Daniel Veillard + + * src/libvirt_private.syms src/storage_backend.h + src/storage_backend_fs.c src/storage_conf.h src/storage_driver.c: + drop the pool lock when allocating fs volumes, patch by Cole Robinson + Fri Apr 17 18:05:52 CEST 2009 Daniel Veillard * configure.in include/libvirt/virterror.h src/Makefile.am diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 350a931eb3..b6ac8e00c4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -46,6 +46,7 @@ virGetDomain; virGetNetwork; virGetStoragePool; virGetStorageVol; +virUnrefStorageVol; virGetNodeDevice; virUnrefDomain; virUnrefConnect; diff --git a/src/storage_backend.h b/src/storage_backend.h index 7fab384bd0..c9c1e35af2 100644 --- a/src/storage_backend.h +++ b/src/storage_backend.h @@ -34,6 +34,7 @@ typedef int (*virStorageBackendRefreshPool)(virConnectPtr conn, virStoragePoolOb typedef int (*virStorageBackendStopPool)(virConnectPtr conn, virStoragePoolObjPtr pool); typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags); +typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol); typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); @@ -52,6 +53,7 @@ struct _virStorageBackend { virStorageBackendStopPool stopPool; virStorageBackendDeletePool deletePool; + virStorageBackendBuildVol buildVol; virStorageBackendCreateVol createVol; virStorageBackendRefreshVol refreshVol; virStorageBackendDeleteVol deleteVol; diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c index 5000f4366f..7a1bba83dc 100644 --- a/src/storage_backend_fs.c +++ b/src/storage_backend_fs.c @@ -982,16 +982,16 @@ virStorageBackendFileSystemDelete(virConnectPtr conn, /** - * Allocate a new file as a volume. This is either done directly - * for raw/sparse files, or by calling qemu-img/qcow-create for - * special kinds of files + * Set up a volume definition to be added to a pool's volume list, but + * don't do any file creation or allocation. By separating the two processes, + * we allow allocation progress reporting (by polling the volume's 'info' + * function), and can drop the parent pool lock during the (slow) allocation. */ static int virStorageBackendFileSystemVolCreate(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - int fd; if (VIR_ALLOC_N(vol->target.path, strlen(pool->def->target.path) + 1 + strlen(vol->name) + 1) < 0) { @@ -1008,6 +1008,20 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, return -1; } + return 0; +} + +/** + * Allocate a new file as a volume. This is either done directly + * for raw/sparse files, or by calling qemu-img/qcow-create for + * special kinds of files + */ +static int +virStorageBackendFileSystemVolBuild(virConnectPtr conn, + virStorageVolDefPtr vol) +{ + int fd; + if (vol->target.format == VIR_STORAGE_VOL_FILE_RAW) { if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, vol->target.perms.mode)) < 0) { @@ -1017,6 +1031,16 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, return -1; } + /* Seek to the final size, so the capacity is available upfront + * for progress reporting */ + if (ftruncate(fd, vol->capacity) < 0) { + virReportSystemError(conn, errno, + _("cannot extend file '%s'"), + vol->target.path); + close(fd); + return -1; + } + /* Pre-allocate any data if requested */ /* XXX slooooooooooooooooow on non-extents-based file systems */ /* FIXME: Add in progress bars & bg thread if progress bar requested */ @@ -1039,7 +1063,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, r, _("cannot fill file '%s'"), vol->target.path); - unlink(vol->target.path); close(fd); return -1; } @@ -1052,22 +1075,12 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, r, _("cannot fill file '%s'"), vol->target.path); - unlink(vol->target.path); close(fd); return -1; } } } - /* Now seek to final size, possibly making the file sparse */ - if (ftruncate(fd, vol->capacity) < 0) { - virReportSystemError(conn, errno, - _("cannot extend file '%s'"), - vol->target.path); - unlink(vol->target.path); - close(fd); - return -1; - } } else if (vol->target.format == VIR_STORAGE_VOL_FILE_DIR) { if (mkdir(vol->target.path, vol->target.perms.mode) < 0) { virReportSystemError(conn, errno, @@ -1127,7 +1140,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, snprintf(size, sizeof(size), "%llu", vol->capacity/1024); if (virRun(conn, imgargv, NULL) < 0) { - unlink(vol->target.path); return -1; } @@ -1135,7 +1147,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, errno, _("cannot read path '%s'"), vol->target.path); - unlink(vol->target.path); return -1; } #elif HAVE_QCOW_CREATE @@ -1168,7 +1179,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, imgargv[3] = NULL; if (virRun(conn, imgargv, NULL) < 0) { - unlink(vol->target.path); return -1; } @@ -1176,7 +1186,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, errno, _("cannot read path '%s'"), vol->target.path); - unlink(vol->target.path); return -1; } #else @@ -1193,7 +1202,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, errno, _("cannot set file owner '%s'"), vol->target.path); - unlink(vol->target.path); close(fd); return -1; } @@ -1202,7 +1210,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, errno, _("cannot set file mode '%s'"), vol->target.path); - unlink(vol->target.path); close(fd); return -1; } @@ -1211,7 +1218,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, if (virStorageBackendUpdateVolTargetInfoFD(conn, &vol->target, fd, &vol->allocation, NULL) < 0) { - unlink(vol->target.path); close(fd); return -1; } @@ -1220,7 +1226,6 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, virReportSystemError(conn, errno, _("cannot close file '%s'"), vol->target.path); - unlink(vol->target.path); return -1; } @@ -1268,6 +1273,7 @@ virStorageBackend virStorageBackendDirectory = { .buildPool = virStorageBackendFileSystemBuild, .refreshPool = virStorageBackendFileSystemRefresh, .deletePool = virStorageBackendFileSystemDelete, + .buildVol = virStorageBackendFileSystemVolBuild, .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, @@ -1282,6 +1288,7 @@ virStorageBackend virStorageBackendFileSystem = { .refreshPool = virStorageBackendFileSystemRefresh, .stopPool = virStorageBackendFileSystemStop, .deletePool = virStorageBackendFileSystemDelete, + .buildVol = virStorageBackendFileSystemVolBuild, .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, @@ -1295,6 +1302,7 @@ virStorageBackend virStorageBackendNetFileSystem = { .refreshPool = virStorageBackendFileSystemRefresh, .stopPool = virStorageBackendFileSystemStop, .deletePool = virStorageBackendFileSystemDelete, + .buildVol = virStorageBackendFileSystemVolBuild, .createVol = virStorageBackendFileSystemVolCreate, .refreshVol = virStorageBackendFileSystemVolRefresh, .deleteVol = virStorageBackendFileSystemVolDelete, diff --git a/src/storage_conf.h b/src/storage_conf.h index 4e35ccbc16..8a4fed2361 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -84,6 +84,8 @@ struct _virStorageVolDef { char *key; int type; /* virStorageVolType enum */ + unsigned int building; + unsigned long long allocation; unsigned long long capacity; @@ -238,6 +240,7 @@ struct _virStoragePoolObj { char *autostartLink; int active; int autostart; + unsigned int asyncjobs; virStoragePoolDefPtr def; virStoragePoolDefPtr newDef; diff --git a/src/storage_driver.c b/src/storage_driver.c index 97fcf69a7c..8c4a03a78d 100644 --- a/src/storage_driver.c +++ b/src/storage_driver.c @@ -564,6 +564,13 @@ storagePoolUndefine(virStoragePoolPtr obj) { goto cleanup; } + if (pool->asyncjobs > 0) { + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR, + _("pool '%s' has asynchronous jobs running."), + pool->def->name); + goto cleanup; + } + if (virStoragePoolObjDeleteDef(obj->conn, pool) < 0) goto cleanup; @@ -696,6 +703,13 @@ storagePoolDestroy(virStoragePoolPtr obj) { goto cleanup; } + if (pool->asyncjobs > 0) { + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR, + _("pool '%s' has asynchronous jobs running."), + pool->def->name); + goto cleanup; + } + if (backend->stopPool && backend->stopPool(obj->conn, pool) < 0) goto cleanup; @@ -745,6 +759,13 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; } + if (pool->asyncjobs > 0) { + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR, + _("pool '%s' has asynchronous jobs running."), + pool->def->name); + goto cleanup; + } + if (!backend->deletePool) { virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT, "%s", _("pool does not support volume delete")); @@ -788,6 +809,13 @@ storagePoolRefresh(virStoragePoolPtr obj, goto cleanup; } + if (pool->asyncjobs > 0) { + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR, + _("pool '%s' has asynchronous jobs running."), + pool->def->name); + goto cleanup; + } + virStoragePoolObjClearVols(pool); if (backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) @@ -1161,6 +1189,8 @@ cleanup: return ret; } +static int storageVolumeDelete(virStorageVolPtr obj, unsigned int flags); + static virStorageVolPtr storageVolumeCreateXML(virStoragePoolPtr obj, const char *xmldesc, @@ -1168,8 +1198,8 @@ storageVolumeCreateXML(virStoragePoolPtr obj, virStorageDriverStatePtr driver = obj->conn->storagePrivateData; virStoragePoolObjPtr pool; virStorageBackendPtr backend; - virStorageVolDefPtr vol = NULL; - virStorageVolPtr ret = NULL; + virStorageVolDefPtr voldef = NULL; + virStorageVolPtr ret = NULL, volobj = NULL; storageDriverLock(driver); pool = virStoragePoolObjFindByUUID(&driver->pools, obj->uuid); @@ -1190,11 +1220,11 @@ storageVolumeCreateXML(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; - vol = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL); - if (vol == NULL) + voldef = virStorageVolDefParse(obj->conn, pool->def, xmldesc, NULL); + if (voldef == NULL) goto cleanup; - if (virStorageVolDefFindByName(pool, vol->name)) { + if (virStorageVolDefFindByName(pool, voldef->name)) { virStorageReportError(obj->conn, VIR_ERR_INVALID_STORAGE_POOL, "%s", _("storage vol already exists")); goto cleanup; @@ -1208,22 +1238,72 @@ storageVolumeCreateXML(virStoragePoolPtr obj, if (!backend->createVol) { virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT, - "%s", _("storage pool does not support volume creation")); + "%s", _("storage pool does not support volume " + "creation")); goto cleanup; } - /* XXX ought to drop pool lock while creating instance */ - if (backend->createVol(obj->conn, pool, vol) < 0) { + if (backend->createVol(obj->conn, pool, voldef) < 0) { goto cleanup; } - pool->volumes.objs[pool->volumes.count++] = vol; + pool->volumes.objs[pool->volumes.count++] = voldef; + volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name, + voldef->key); - ret = virGetStorageVol(obj->conn, pool->def->name, vol->name, vol->key); - vol = NULL; + if (0) { + printf("after vol lookup.\n"); + virReportOOMError(obj->conn); + goto cleanup; + } + + if (volobj && backend->buildVol) { + int buildret; + virStorageVolDefPtr buildvoldef = NULL; + + if (VIR_ALLOC(buildvoldef) < 0) { + virReportOOMError(obj->conn); + voldef = NULL; + 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); + + buildret = backend->buildVol(obj->conn, buildvoldef); + + virStoragePoolObjLock(pool); + voldef->building = 0; + pool->asyncjobs--; + + voldef = NULL; + VIR_FREE(buildvoldef); + + if (buildret < 0) { + virStoragePoolObjUnlock(pool); + storageVolumeDelete(volobj, 0); + pool = NULL; + goto cleanup; + } + + } + + ret = volobj; + volobj = NULL; + voldef = NULL; cleanup: - virStorageVolDefFree(vol); + if (volobj) + virUnrefStorageVol(volobj); + virStorageVolDefFree(voldef); if (pool) virStoragePoolObjUnlock(pool); return ret; @@ -1266,6 +1346,13 @@ storageVolumeDelete(virStorageVolPtr obj, goto cleanup; } + if (vol->building) { + virStorageReportError(obj->conn, VIR_ERR_INTERNAL_ERROR, + _("volume '%s' is still being allocated."), + vol->name); + goto cleanup; + } + if (!backend->deleteVol) { virStorageReportError(obj->conn, VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support vol deletion"));