Re: [Qemu-block] [PATCH for-2.7 v2 00/17] block: Lock images when opening

[Daniel P. Berrange]

On Fri, Apr 15, 2016 at 11:27:50AM +0800, Fam Zheng wrote:
> v2: Lock byte 1 in the image itself, no lock file. [Daniel]
>     Fix migration (image are not locked in bdrv_open_common if
>     BDRV_O_INACTIVE). [Denis]
>     Simplify test case fixes because of the above.
>     Add lock for RBD.
>     Add "-L" option in "qemu-img" and "qemu-nbd" too. [Denis]
>     Add test case for image locking.
> 
> Too many troubles have been caused by two processes writing to the same image
> unexpectedly. This series introduces automatical image locking into QEMU to
> avoid such tragedy. With this, the user won't be able to open the image from
> two processes (e.g. using qemu-img when the image is attached to the guest).
> 
> Underneath is the fcntl syscall that locks the local file, similar to what is
> already used in libvirt virtlockd. The difference is that libvirt locks byte 
> 0,
> we lock byte 1.  Read only openings are mapped to shared locks.

I'm afraid the way you are using fcntl is not actually safe, because
fcntl does not correctly deal with concurrent usage within the same
VM. I have tested your series with the following sequence of steps.

Consider I have a pair of disk images:

 $ qemu-img create master.img 10G
 Formatting 'master.img', fmt=raw size=10737418240
 $ qemu-img create -f qcow2 -obacking_file=master.img overlay.img
 Formatting 'overlay.img', fmt=qcow2 size=10737418240 backing_file='master.img' 
encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16


Now I launch QEMU pointing it to overlay.img:

 $ qemu-system-x86_64 -drive file=/home/berrange/VirtualMachines/overlay.img  
-monitor stdio
 QEMU 2.5.91 monitor - type 'help' for more information

Looking at the locks held, everything is correct:

 $lslocks  | grep qemu
 qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 
/home/berrange/VirtualMachines/overlay.img
 qemu-system-x86 28723  POSIX    10G READ  0          1          1 
/home/berrange/VirtualMachines/master.img


Now see what happens when I hotplug a second disk image pointing
to master.img in write mode - this should be denied since writing
to master.img will invalidate the backing store used by the currently
open overlay.img:

(qemu) drive_add 0:1:1 file=/home/berrange/VirtualMachines/master.img,if=none
WARNING: Image format was not specified for 
'/home/berrange/VirtualMachines/master.img' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write 
operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
OK

It was mistakenly allowed. We should not allow writing to a disk
image that is used as backing store for an overlay. Now look at
the locks held:

$ lslocks  | grep qemu
qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 
/home/berrange/VirtualMachines/overlay.img
qemu-system-x86 28723  POSIX    10G WRITE 0          1          1 
/home/berrange/VirtualMachines/master.img

The read lock on master.img has simply been "upgraded" to a write
lock. This is totally incorrect behaviour from POV of protecting
the disk images.

Now lets unplug the drive we just added:

(qemu) drive_del none0


And look at the locks held again:

$ lslocks  | grep qemu
qemu-system-x86 28723  POSIX 192.5K WRITE 0          1          1 
/home/berrange/VirtualMachines/overlay.img

So we've now lost the lock on msater.img, despite the fact that
it is still open for reading as a backing store of overlay.img.


This is all caused by the problems I've mentioned in previous
discussions whereby  locks are scoped to the process, not the
file handle. eg opening a second file handle wil happily upgrde
the lock held by the first file handle from read to write.
Closing the second file handle will happily release the lock,
even though the first file handle is still open.


If you want todo locks inside of QEMU, you really can't rely
on delegating handling to each individual block driver instance.
You need to have a process global registry of which files you have
already locked, and check against that registry before even
attempting to open any file that might correspond to a disk image.
This registry needs to canonicalize the file path too, to allow
for possibility of QEMU being given differen paths to the same
file.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|