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@xxxxxxxxxx> > --- > 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list