[libvirt] [PATCH 3/3] storage: Fix virStorageBackendDiskDeleteVol for device mapper
John Ferlan
jferlan at redhat.com
Tue May 10 13:26:42 UTC 2016
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 at redhat.com>
---
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
More information about the libvir-list
mailing list