Prior to calling the 'refreshPool' during CreatePool or UploadPool operations, we need to clear the pool; otherwise, the pool will have duplicated entries. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/storage/storage_driver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 1d42f24..bf05b19 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -727,6 +727,7 @@ storagePoolCreateXML(virConnectPtr conn, stateFile = virFileBuildPath(driver->stateDir, pool->def->name, ".xml"); + virStoragePoolObjClearVols(pool); if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(conn, pool) < 0) { if (stateFile) @@ -918,6 +919,7 @@ storagePoolCreate(virStoragePoolPtr obj, stateFile = virFileBuildPath(driver->stateDir, pool->def->name, ".xml"); + virStoragePoolObjClearVols(pool); if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || backend->refreshPool(obj->conn, pool) < 0) { if (stateFile) @@ -2356,6 +2358,7 @@ storageVolUpload(virStorageVolPtr obj, * interaction and we can just lookup the backend in the callback * routine in order to call the refresh API. */ + virStoragePoolObjClearVols(pool); if (backend->refreshPool) { if (VIR_ALLOC(cbdata) < 0 || VIR_STRDUP(cbdata->pool_name, pool->def->name) < 0) -- 2.5.5

Commit id 'df1011ca8' modified virStorageBackendDiskDeleteVol to use "dmsetup remove --force" to remove the volume, but left things in an inconsistent state since the partition still existed on the disk and only the device mapper device (/dev/dm-#) was removed. Prior to commit '1895b421' (or '1ffd82bb' and '471e1c4e'), this could go unnoticed since virStorageBackendDiskRefreshPool wasn't called. However, the pool would be unusable since the /dev/dm-# device would be removed even though the partition was not removed unless a multipathd restart reset the link. That would of course make the volume appear again in the pool after a refresh or pool start after libvirt reload. This patch removes the 'dmsetup' logic and re-implements the partition deletion logic for device mapper devices. The removal of the partition via 'parted rm --script #' will cause udev device change logic to allow multipathd to handle removing the dm-* device associated with the partition. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- configure.ac | 15 ++---- src/storage/parthelper.c | 2 + src/storage/storage_backend_disk.c | 99 +++++++++++++++++++++++++------------- 3 files changed, 70 insertions(+), 46 deletions(-) diff --git a/configure.ac b/configure.ac index d19c1a9..c3f0add 100644 --- a/configure.ac +++ b/configure.ac @@ -1957,19 +1957,12 @@ LIBPARTED_LIBS= if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; then AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin]) - AC_PATH_PROG([DMSETUP], [dmsetup], [], [$PATH:/sbin:/usr/sbin]) if test -z "$PARTED" ; then PARTED_FOUND=no else PARTED_FOUND=yes fi - if test -z "$DMSETUP" ; then - DMSETUP_FOUND=no - else - DMSETUP_FOUND=yes - fi - if test "$PARTED_FOUND" = "yes" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES([LIBPARTED], [libparted >= $PARTED_REQUIRED], [], [PARTED_FOUND=no]) @@ -1988,12 +1981,12 @@ if test "$with_storage_disk" = "yes" || fi if test "$with_storage_disk" = "yes" && - test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then - AC_MSG_ERROR([Need both parted and dmsetup for disk storage driver]) + test "$PARTED_FOUND" != "yes"; then + AC_MSG_ERROR([Need parted for disk storage driver]) fi if test "$with_storage_disk" = "check"; then - if test "$PARTED_FOUND:$DMSETUP_FOUND" != "yes:yes"; then + if test "$PARTED_FOUND" != "yes"; then with_storage_disk=no else with_storage_disk=yes @@ -2005,8 +1998,6 @@ if test "$with_storage_disk" = "yes" || [whether Disk backend for storage driver is enabled]) AC_DEFINE_UNQUOTED([PARTED],["$PARTED"], [Location or name of the parted program]) - AC_DEFINE_UNQUOTED([DMSETUP],["$DMSETUP"], - [Location or name of the dmsetup program]) fi fi AM_CONDITIONAL([WITH_STORAGE_DISK], [test "$with_storage_disk" = "yes"]) diff --git a/src/storage/parthelper.c b/src/storage/parthelper.c index d81b177..afdaa1c 100644 --- a/src/storage/parthelper.c +++ b/src/storage/parthelper.c @@ -83,6 +83,8 @@ int main(int argc, char **argv) return 1; } + /* NB: Changes to the following algorithm will need corresponding + * changes to virStorageBackendDiskDeleteVol */ path = argv[1]; if (virIsDevMapperDevice(path)) { /* If the path ends with a number or we explicitly request it for diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index eadf8a3..666ad03 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -799,6 +799,31 @@ virStorageBackendDiskPartBoundaries(virStoragePoolObjPtr pool, } +/* virStorageBackendDiskDeleteVol + * @conn: Pointer to a libvirt connection + * @pool: Pointer to the storage pool + * @vol: Pointer to the volume definition + * @flags: flags (unused for now) + * + * This API will remove the disk volume partition either from direct + * API call or as an error path during creation when the partition + * name provided during create doesn't match the name read from + * virStorageBackendDiskReadPartitions. + * + * For a device mapper device, device respresentation is dependant upon + * device mapper configuration, but the general rule of thumb is that at + * creation if a device name ends with a number, then a partition separator + * "p" is added to the created name; otherwise, if the device name doesn't + * end with a number, then there is no partition separator. This name is + * what ends up in the vol->target.path. This ends up being a link to a + * /dev/mapper/dm-# device which cannot be used in the algorithm to determine + * which partition to remove, but a properly handled target.path can be. + * + * For non device mapper devices, just need to resolve the link of the + * vol->target.path in order to get the path. + * + * Returns 0 on success, -1 on failure with error message set. + */ static int virStorageBackendDiskDeleteVol(virConnectPtr conn, virStoragePoolObjPtr pool, @@ -807,7 +832,9 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn, { char *part_num = NULL; char *devpath = NULL; - char *dev_name, *srcname; + char *dev_name; + char *src_path = pool->def->source.devices[0].path; + char *srcname = last_component(src_path); virCommandPtr cmd = NULL; bool isDevMapperDevice; int rc = -1; @@ -817,56 +844,60 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn, if (!vol->target.path) { virReportError(VIR_ERR_INVALID_ARG, _("volume target path empty for source path '%s'"), - pool->def->source.devices[0].path); + src_path); return -1; } - if (virFileResolveLink(vol->target.path, &devpath) < 0) { - virReportSystemError(errno, - _("Couldn't read volume target path '%s'"), - vol->target.path); - goto cleanup; + /* NB: This is the corollary to the algorithm in libvirt_parthelper + * (parthelper.c) that is used to generate the target.path name + * for use by libvirt. Changes to either, need to be reflected + * in both places */ + isDevMapperDevice = virIsDevMapperDevice(vol->target.path); + if (isDevMapperDevice) { + dev_name = last_component(vol->target.path); + } else { + if (virFileResolveLink(vol->target.path, &devpath) < 0) { + virReportSystemError(errno, + _("Couldn't read volume target path '%s'"), + vol->target.path); + goto cleanup; + } + dev_name = last_component(devpath); } - dev_name = last_component(devpath); - srcname = last_component(pool->def->source.devices[0].path); VIR_DEBUG("dev_name=%s, srcname=%s", dev_name, srcname); - isDevMapperDevice = virIsDevMapperDevice(devpath); - - if (!isDevMapperDevice && !STRPREFIX(dev_name, srcname)) { + if (!STRPREFIX(dev_name, srcname)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Volume path '%s' did not start with parent " "pool source device name."), dev_name); goto cleanup; } - if (!isDevMapperDevice) { - part_num = dev_name + strlen(srcname); + part_num = dev_name + strlen(srcname); - if (*part_num == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse partition number from target " - "'%s'"), dev_name); - goto cleanup; - } + /* For device mapper and we have a partition character 'p' as the + * current character, let's move beyond that before checking part_num */ + if (isDevMapperDevice && *part_num == 'p') + part_num++; - /* eg parted /dev/sda rm 2 */ - cmd = virCommandNewArgList(PARTED, - pool->def->source.devices[0].path, - "rm", - "--script", - part_num, - NULL); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - } else { - cmd = virCommandNewArgList(DMSETUP, "remove", "--force", devpath, NULL); - - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + if (*part_num == 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse partition number from target " + "'%s'"), dev_name); + goto cleanup; } + /* eg parted /dev/sda rm 2 or /dev/mapper/mpathc rm 2 */ + cmd = virCommandNewArgList(PARTED, + src_path, + "rm", + "--script", + part_num, + NULL); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + /* Refreshing the pool is the easiest option as LOGICAL and EXTENDED * partition allocation/capacity management is handled within * virStorageBackendDiskMakeDataVol and trying to redo that logic -- 2.5.5