[libvirt] [PATCH 2/2] virDomainLockManagerAddImage: Recursively lock backing chain

[Peter Krempa]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150908/c16d8467/attachment-0001.sig>