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

[Peter Krempa]

On Wed, Sep 09, 2015 at 10:13:24 +0200, Michal Privoznik wrote:
> On 08.09.2015 18:04, Daniel P. Berrange wrote:
> > On Tue, Sep 08, 2015 at 05:59:45PM +0200, Peter Krempa wrote:
> >> 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.
> > 
> > Yep, the lock manager won't protect against you doing stupid
> > stuff to the base image, which invalidates children, but that's
> > out of scope really. We only need to consider the concurrent
> > running VM problem here.
> 
> 
> Agreed. There's no way for us to see if the layer we are looking at is
> top layer or not. On snapshot creation top layer is not modified, there
> are no reverse records. Unless we want to search each file on the system
> to see if there's 'A' when starting from 'B', it's not an argument
> against my patch.

What I wanted to point out is that this might give the users a false
sense of security in some orderings of startup of VMs while in other
cases the data will be corrupted and nothing can happen. I think we
should point out this fact in the docs very very explicitly.

> 
> > 
> > 

...

> > 
> > Yep, backing files should be marked readonly - 'shared' will allow
> > multiple VMs to write to the image at once. So you've only provided
> > mutual exclusion between an exclusive writer, vs a shared writer.
> 
> Except that both of our locking drivers threat
> VIR_LOCK_MANAGER_RESOURCE_READONLY as no-op.
> 
> But I think that since shared and exclusive writer are mutually
> exclusive, it doesn't matter that the lower layers are locked as shared
> writer. As soon as somebody wants to lock it exclusively they will fail.
> Nor virtlockd nor sanlock is actually enforcing the RO vs RW policy (if
> a file is locked for RO no writes are allowed). It's merely for tracking
> shared and exclusive locks.

In that case you should fix the documentation in the lock driver header
first so that you will then be able to justify the use of _SHARED here.

> 

Additionally this will probably end up in more work, since you will need
to go through the block job code and fix it to do the locking in the
same way you decide to do it here. The backing chain manipulation APIs
change between RW and RO locks for individual backing chain members, so
a change to shared lock will need to be used there too.

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/20150909/3a552135/attachment-0001.sig>