External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2024-12-22 13:45:38 +00:00
Code Issues Packages Projects Releases Wiki Activity

storage: Fix virStorageBackendDiskDeleteVol for device mapper

Browse Source 
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.
This commit is contained in:
John Ferlan 2016-04-26 08:53:57 -04:00
parent e7bde8d319
commit 8cdff0b93f
3 changed files with 70 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
configure.ac
View File

@ -1958,19 +1958,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])
@ -1989,12 +1982,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
@ -2006,8 +1999,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"])

2
src/storage/parthelper.c
View File

@ -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

99
src/storage/storage_backend_disk.c
View File

@ -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