10:59 a.m.
On Tue, Sep 08, 2015 at 17:17:19 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1192399
It's known fact for a while now that we should not only lock the
top level layers of backing chain but the rest of it too. And
well known too that we are not doing that. Well, up until this
commit. The reason is that while one guest can have for instance
the following backing chain: A (top) -> B -> C (bottom), the
other can be started just over B or C. But libvirt should prevent
that as it will almost certainly mangle the backing chain for the
former guest. On the other hand, we want to allow guest with the
Well, the corruption may still happen since if you run the guest that
uses image 'B' for writing, without starting the one that uses 'A' the
image 'A' will be invalidated anyways.
following backing chain: D (top) -> B -> C (bottom), because
in
both cases B and C are there just for reading. In order to
achieve that we must lock the rest of backing chain with
VIR_LOCK_MANAGER_RESOURCE_SHARED flag.
See below ...
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/locking/domain_lock.c | 53 +++++++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 20 deletions(-)
diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c
index 705b132..faf73f2 100644
--- a/src/locking/domain_lock.c
+++ b/src/locking/domain_lock.c
@@ -69,35 +69,48 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock,
static int virDomainLockManagerAddImage(virLockManagerPtr lock,
- virStorageSourcePtr src)
+ virStorageSourcePtr disk)
This function is used both in cases where you are adding the whole disk
to the lock manager but also in cases where we are adding individual
members of the backing chain that may already contain shared portions of
the backing chain (eg. when creating snapshots) which is not desired.
virDomainLockDiskAttach should recurse through the backing chain, but
virDomainLockImageAttach should not.
While this wouldn't pose a big problem for adding disks, when you are
removing a single image file from the locking, the code would unlock the
whole backing chain.
{
unsigned int diskFlags = 0;
- int type = virStorageSourceGetActualType(src);
-
- if (!src->path)
- return 0;
-
- if (!(type == VIR_STORAGE_TYPE_BLOCK ||
- type == VIR_STORAGE_TYPE_FILE ||
- type == VIR_STORAGE_TYPE_DIR))
- return 0;
+ virStorageSourcePtr src = disk;
+ int ret = -1;
+ /* The top layer of backing chain should have the correct flags set.
+ * The rest should be locked with VIR_LOCK_MANAGER_RESOURCE_SHARED
+ * for it can be shared among other domains. */
if (src->readonly)
diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY;
if (src->shared)
diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
- VIR_DEBUG("Add disk %s", src->path);
- if (virLockManagerAddResource(lock,
- VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
- src->path,
- 0,
- NULL,
- diskFlags) < 0) {
- VIR_DEBUG("Failed add disk %s", src->path);
- return -1;
+ while (src) {
+ if (!virStorageSourceIsLocalStorage(src))
+ break;
Also note that once you create a snapshot on a remote storage device on
top of the existing local backing chain, the backing chain of that image
won't get unlocked.
+
+ if (!src->path)
+ break;
+
+ VIR_DEBUG("Add disk %s", src->path);
+ if (virLockManagerAddResource(lock,
+ VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK,
+ src->path,
+ 0,
+ NULL,
+ diskFlags) < 0) {
+ VIR_DEBUG("Failed add disk %s", src->path);
+ goto cleanup;
+ }
+
+ /* Now that we've added the first disk (top layer of backing chain)
+ * into the lock manager, let's traverse the rest of the backing chain
+ * and lock each layer for RO. This should prevent accidentally
+ * starting a domain with RW disk from the middle of the chain. */
+ diskFlags = VIR_LOCK_MANAGER_RESOURCE_SHARED;
The VIR_LOCK_MANAGER_RESOURCE_SHARED flag means "shared, writable" mode
according to lock_driver.h, I think you want to lock it with
VIR_LOCK_MANAGER_RESOURCE_READONLY.
+ src = src->backingStore;
}
- return 0;
+ ret = 0;
+ cleanup:
+ return ret;
}
Peter