From: Peter Krempa <pkrempa@redhat.com> The current design of the lock drivers holds the locks on the disk images only while the VM is running. There's no concept of 'read' locks. Block jobs can be used also when the VM is paused which is when the automatic locks are released. Allowing block jobs would either corrupt the data for anyone else holding the lock or the job could read garbage if another VM is using it in write mode. As of such there's no way libvirt can declare that block jobs are compatible with what we want to declare in regards to locking. Add a statement to the lock driver config that this is incompatible with blockjobs and forbid blockjobs when disk locks are in use. While the locking backend could be improved, there isn't enough justification to spend that amount of work required especially since now qemu has image locking which includes read locks. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/locking/lockd.conf | 4 ++++ src/locking/sanlock.conf | 4 ++++ src/qemu/qemu_backup.c | 2 +- src/qemu/qemu_block.c | 2 +- src/qemu/qemu_checkpoint.c | 2 +- src/qemu/qemu_domain.c | 9 ++++++++- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_snapshot.c | 25 +++++++++++++++---------- 9 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/locking/lockd.conf b/src/locking/lockd.conf index ed7703ee06..a83be8b599 100644 --- a/src/locking/lockd.conf +++ b/src/locking/lockd.conf @@ -4,6 +4,10 @@ # application wishes to instead manually manage leases in # the guest XML, then this parameter can be disabled # +# Beware that block jobs (external disk snapshots, pull, +# commit, backup) do not work when automatic disk leases +# are enabled. +# #auto_disk_leases = 0 # diff --git a/src/locking/sanlock.conf b/src/locking/sanlock.conf index 3c356bef9c..743921f6c3 100644 --- a/src/locking/sanlock.conf +++ b/src/locking/sanlock.conf @@ -10,6 +10,10 @@ # # Uncomment this to enable automatic lease creation. # +# Beware that block jobs (external disk snapshots, pull, +# commit, backup) do not work when automatic disk leases +# are enabled. +# # NB: the 'host_id' parameter must be set if enabling this # #auto_disk_leases = 1 diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index a0544c83dc..45f8544957 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -287,7 +287,7 @@ qemuBackupDiskPrepareDataOne(virDomainObj *vm, return -1; } - if (!qemuDomainDiskBlockJobIsSupported(dd->domdisk)) + if (!qemuDomainDiskBlockJobIsSupported(priv->driver, dd->domdisk)) return -1; if (dd->store->format == VIR_STORAGE_FILE_NONE) { diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 324c2635b4..ce957c2e05 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -3686,7 +3686,7 @@ qemuBlockCommit(virDomainObj *vm, if (virDomainObjCheckActive(vm) < 0) return NULL; - if (!qemuDomainDiskBlockJobIsSupported(disk)) + if (!qemuDomainDiskBlockJobIsSupported(driver, disk)) return NULL; if (virStorageSourceIsEmpty(disk->src)) { diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index f063b5a5c0..705086d88e 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -371,7 +371,7 @@ qemuCheckpointPrepare(virQEMUDriver *driver, return -1; } - if (!qemuDomainDiskBlockJobIsSupported(vm->def->disks[i])) + if (!qemuDomainDiskBlockJobIsSupported(priv->driver, vm->def->disks[i])) return -1; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2f1d220966..3751deb85d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10513,7 +10513,8 @@ qemuDomainDiskHasLatencyHistogram(virDomainDiskDef *disk) * Note that this does not verify whether other block jobs are running etc. */ bool -qemuDomainDiskBlockJobIsSupported(virDomainDiskDef *disk) +qemuDomainDiskBlockJobIsSupported(virQEMUDriver *driver, + virDomainDiskDef *disk) { if (qemuDiskBusIsSD(disk->bus)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -10536,6 +10537,12 @@ qemuDomainDiskBlockJobIsSupported(virDomainDiskDef *disk) return false; } + if (virLockManagerPluginAutoDiskLeases(driver->lockManager)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("block jobs are not supported when automatic disk locking is activated via sanlock/lockd")); + return false; + } + return true; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 24e62dd2e7..da1914979c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1079,7 +1079,8 @@ bool qemuDomainDiskHasLatencyHistogram(virDomainDiskDef *disk); bool -qemuDomainDiskBlockJobIsSupported(virDomainDiskDef *disk); +qemuDomainDiskBlockJobIsSupported(virQEMUDriver *driver, + virDomainDiskDef *disk); int qemuDomainDefNumaCPUsRectify(virDomainDef *def, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 544955ecf9..d271853515 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14015,7 +14015,7 @@ qemuDomainBlockPullCommon(virDomainObj *vm, if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; - if (!qemuDomainDiskBlockJobIsSupported(disk)) + if (!qemuDomainDiskBlockJobIsSupported(priv->driver, disk)) goto endjob; if (base && @@ -14543,7 +14543,7 @@ qemuDomainBlockCopyCommon(virDomainObj *vm, if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; - if (!qemuDomainDiskBlockJobIsSupported(disk)) + if (!qemuDomainDiskBlockJobIsSupported(priv->driver, disk)) goto endjob; if (virStorageSourceIsFD(mirror)) { diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 07548ca62e..988ecb965a 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -726,7 +726,8 @@ qemuSnapshotPrepareDiskExternalInactive(virDomainSnapshotDiskDef *snapdisk, static int -qemuSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDef *snapdisk, +qemuSnapshotPrepareDiskExternalActive(virQEMUDriver *driver, + virDomainSnapshotDiskDef *snapdisk, virDomainDiskDef *domdisk) { virStorageType actualType = virStorageSourceGetActualType(snapdisk->src); @@ -740,7 +741,7 @@ qemuSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDef *snapdisk, return -1; } - if (!qemuDomainDiskBlockJobIsSupported(domdisk)) + if (!qemuDomainDiskBlockJobIsSupported(driver, domdisk)) return -1; switch (actualType) { @@ -771,7 +772,8 @@ qemuSnapshotPrepareDiskExternalActive(virDomainSnapshotDiskDef *snapdisk, static int -qemuSnapshotPrepareDiskExternal(virDomainDiskDef *disk, +qemuSnapshotPrepareDiskExternal(virQEMUDriver *driver, + virDomainDiskDef *disk, virDomainSnapshotDiskDef *snapdisk, bool active, bool reuse) @@ -805,7 +807,7 @@ qemuSnapshotPrepareDiskExternal(virDomainDiskDef *disk, if (qemuSnapshotPrepareDiskExternalInactive(snapdisk, disk) < 0) return -1; } else { - if (qemuSnapshotPrepareDiskExternalActive(snapdisk, disk) < 0) + if (qemuSnapshotPrepareDiskExternalActive(driver, snapdisk, disk) < 0) return -1; } @@ -937,6 +939,7 @@ qemuSnapshotPrepare(virDomainObj *vm, unsigned int *flags) { size_t i; + qemuDomainObjPrivate *priv = vm->privateData; bool active = virDomainObjIsActive(vm); bool reuse = (*flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; bool found_internal = false; @@ -977,7 +980,8 @@ qemuSnapshotPrepare(virDomainObj *vm, break; case VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL: - if (qemuSnapshotPrepareDiskExternal(dom_disk, disk, active, reuse) < 0) + if (qemuSnapshotPrepareDiskExternal(priv->driver, dom_disk, disk, + active, reuse) < 0) return -1; external++; @@ -2445,6 +2449,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, virDomainDef *inactiveConfig, qemuSnapshotRevertMemoryData *memdata) { + qemuDomainObjPrivate *priv = vm->privateData; size_t i; bool active = virDomainObjIsActive(vm); virDomainDef *domdef = NULL; @@ -2477,20 +2482,20 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, if (snapdisk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) continue; - if (qemuSnapshotPrepareDiskExternal(domdisk, snapdisk, active, false) < 0) + if (qemuSnapshotPrepareDiskExternal(priv->driver, domdisk, snapdisk, + active, false) < 0) return -1; } if (memdata && snapdef->memorysnapshotfile) { - virQEMUDriver *driver = ((qemuDomainObjPrivate *) vm->privateData)->driver; g_autoptr(virDomainDef) savedef = NULL; memdata->path = snapdef->memorysnapshotfile; - if (qemuSaveImageGetMetadata(driver, NULL, memdata->path, NULL, NULL, + if (qemuSaveImageGetMetadata(priv->driver, NULL, memdata->path, NULL, NULL, &savedef, &memdata->data) < 0) return -1; - memdata->fd = qemuSaveImageOpen(driver, memdata->path, + memdata->fd = qemuSaveImageOpen(priv->driver, memdata->path, false, &memdata->wrapperFd, false); if (memdata->fd < 0) return -1; @@ -2500,7 +2505,7 @@ qemuSnapshotRevertExternalPrepare(virDomainObj *vm, if (qemuSaveImageFDSkipHeader(memdata->fd) < 0) return -1; - if (!virDomainDefCheckABIStability(savedef, domdef, driver->xmlopt)) + if (!virDomainDefCheckABIStability(savedef, domdef, priv->driver->xmlopt)) return -1; } -- 2.54.0