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

[Kevin Wolf <kwolf@redhat.com>]

Am 10.05.2016 um 12:29 hat Daniel P. Berrange geschrieben:
> 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.

Basically this says "we're protecting our image metadata, please protect
your actual data yourself". This isn't much better than what we have
today. We are implementing locking exactly because users are _not_
completely aware of the problems that concurrent access causes. Telling
them to be more careful doesn't solve any problem.

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

I think being strict and having options to be laxer is useful when we're
talking about corruption risks.

Kevin