Re: [Qemu-block] [Qemu-devel] [PATCH v4 00/27] block: Lock images when opening

[Daniel P. Berrange]

On Tue, May 10, 2016 at 12:07:06PM +0200, Kevin Wolf wrote:
> Am 10.05.2016 um 11:43 hat Daniel P. Berrange geschrieben:
> > On Tue, May 10, 2016 at 11:35:14AM +0200, Kevin Wolf wrote:
> > > Am 10.05.2016 um 11:23 hat Daniel P. Berrange geschrieben:
> > > > On Tue, May 10, 2016 at 11:14:22AM +0200, Kevin Wolf wrote:
> > > > > Am 10.05.2016 um 10:50 hat Daniel P. Berrange geschrieben:
> > > > > > On Tue, May 10, 2016 at 09:43:04AM +0100, Richard W.M. Jones wrote:
> > > > > > > On Tue, May 10, 2016 at 09:14:26AM +0100, Richard W.M. Jones 
> > > > > > > wrote:
> > > > > > > > However I didn't test the write-shareable case (the libvirt
> > > > > > > > <shareable/> flag which should map to a shared lock -- is that 
> > > > > > > > right Dan?).
> > > > > > > 
> > > > > > > To Dan (mainly): I think setting the <shareable/> flag in libvirt 
> > > > > > > only
> > > > > > > sets cache=unsafe on the qemu drive (it may have other effects for
> > > > > > > virtlockd).  Should there be an extra qemu drive flag to 
> > > > > > > communicate
> > > > > > > that the drive is write-shareable?
> > > > > > 
> > > > > > Sure, if QEMU had a way to indicate that the disk was used in a
> > > > > > write-sharable mode, libvirt would use it.
> > > > > > 
> > > > > > I agree with your general point that we should do no locking for
> > > > > > read-only access, F_RDLCK for shared-write and F_WRLCK for
> > > > > > exclusive-write access. I think I made that point similarly against
> > > > > > an earlier version of this patchset
> > > > > 
> > > > > Why should we do no locking for read-only access by default? If an 
> > > > > image
> > > > > is written to, read-only accesses are potentially inconsistent and we
> > > > > should protect users against it.
> > > > > 
> > > > > The only argument I can see in the old versions of this series is
> > > > > "libguestfs does it and usually it gets away with it". For me, that's
> > > > > not an argument for generally allowing this, but at most for 
> > > > > providing a
> > > > > switch to bypass the locking.
> > > > > 
> > > > > Because let's be clear about this: If someone lost data because they
> > > > > took an inconsistent backup this way and comes to us qemu developers,
> > > > > all we can tell them is "sorry, what you did is not supported". And
> > > > > that's a pretty strong sign that locking should prevent it by default.
> > > > 
> > > > We have 3 usage scenarios - readonly-share, writable-shared and
> > > > writable-exclusive, and only 2 lock types to play with. This series
> > > > does no locking at all in the writable-shared case, so we still have
> > > > the problem you describe in that someone opening the image for readonly
> > > > access will succeeed even when it is used writable-shared by another
> > > > process.
> > > > 
> > > > So we have to pick a trade-off here. IMHO it is more important to
> > > > ensure we have locks in both the write-shared & write-exclusive case,
> > > > as both of those can definitely result in corrupted images if not
> > > > careful, where as read-only access merely results in your reading
> > > > bad data, no risk of corrupting the original image.
> > > 
> > > I agree that we want locking for the writable-shared case. That doesn't
> > > mean no locking for read-only, though. I think read-only and writeable-
> > > shared are the same thing as far as locking is concerned.
> > 
> > It doesn't make much sense to say that it is unsafe to use read-only
> > in combination with write-exclusive, but then allow read-only with
> > write-shared. In both cases you have the same scenario that the
> > read-only app may get inconsistent data when reading.
> 
> Doesn't write-shared usually indicate that the contents can actually be
> consistently read/written to, for example because a cluster filesystem
> is used? If you couldn't, you would use write-exclusive instead.
> 
> In contrast, write-exclusive indicates that correct concurrent access
> isn't possible, for example because you're using a "normal" filesystem
> that might additionally keep things in a writeback cache in the guest.
> You wouldn't use write-shared there.
> 
> > > This is the matrix of the allowed combinations (without a manual lock
> > > override that enables libguestfs to use unsupported cases), as I see it:
> > > 
> > >                     wr-excl     wr-shared   read-only
> > > write-exclusive     no          no          no
> > > write-shared        no          yes         yes
> > > read-only           no          yes         yes
> > > 
> > > Do you disagree with any of the entries?
> > 
> > I would have 'yes' in all the read-only cells.
> 
> I'm surprised how low the standards seem to be when we're talking about
> data integrity. If occasionally losing data is okay, the qemu block
> layer could be quite a bit simpler.

To my mind there's two separate issues at play here. Protection against
two processes writing to the same image in a way that causes unrecoverable
corruption. Protection against reading bad data from an otherwise fine
disk image.

Both write-exclusive & write-shared can obviously cause unrecoverable
corruption, so we need locking for both of those scenarios.

Whether you lock or not, read-only access can never cause corruption of
a disk image, so IMHO locking is irrelevant there. Whether it is safe
to have a reader concurrently access disk contents with a writer (whether
shared or exclusive) is dependant on what type of format the data in
the image is stored in. That data format is upto the guest OS and
invisible to QEMU - it may or may not be valid to have readers accessing
a disk that is in shared-write mode, we have no way of knowing. As such
IMHO it doesn't make sense to claim we provide useful protection between
a reader and writers - there will always be cases where QEMU is either
too strict, or too lax, thus giving a false sense of safety or forcing
apps to just unconditionally turn off locking in read-only mode defeating
the point.

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 :|