Re: [Qemu-block] [PATCH v8 00/36] block: Image locking series

[Max Reitz]

On 30.09.2016 14:09, Fam Zheng wrote:
> Hi all,
> 
> I wanted to post something before the long holiday as promised, but I couldn't
> refine or test enough due to limited time.  Please take this as an RFC and do 
> a
> high level review. Thanks.

I did that, looked at some patches in more and at some in less detail.
All patches I didn't reply to looked roughly fine to me (although that
doesn't have to mean much, as I really didn't take a close look at many
patches :-)).

> 
> v8: Move user interface option from block device to qdev. [Kevin]
>     Add "exclusive" back. [Kevin]
> 
>     Note: limited by the qdev interface, until blk_lock_image() is called, the
>     images are not locked by block layer at open, because before device tells
>     us what to do, block layer cannot figure out the correct lock mode any
>     more.
> 
>     TODO 1: Lock explicitly in utils (qemu-img, qemu-io, etc) after opening
>     image.
> 
>     TODO 2: The image locking test case 153 patch is not updated thus broken
>     because of the moved option.
> 
>     TODO 3: Are the open flags unnecessary because we already have
>     ImageLockMode? If so, how to converge them?

Well, yes, what do the flags do now? :-)

BDRV_O_NO_LOCK is still used, but it could probably be replaced by a
call to blk_lock_image(). BDRV_O_SHARED_LOCK and BDRV_O_EXCLUSIVE_LOCK
are no longer used at all, though.

<parenthesis>

I personally still don't like making locking a qdev property very much
because it doesn't make sense to me*. But I remember Kevin had his
reasons (even though I can no longer remember them) for asking for it,
and I don't have any besides "It doesn't make sense to me". After having
though a bit about it (= having written three paragraphs and deleted
them again), I guess I can make some sense of it, though it seems to be
a rather esoteric use case still; it appears to me that a guest could
use it to signal that it's fine with some block device being shared;
then we could use a shared lock or none at all or I don't know.
Otherwise, we should get an exclusive lock for write access and a shared
lock for read access.

(Although, fun question (that's the reason for the "I don't know"): What
if guest A supports such a volatile block device, but guest B doesn't?
Then imagine we run A with write access and B with read-only access. A
will claim no lock or a shared lock, while B will probably claim a
shared lock, too, expecting writing users to try to grab an exclusive
lock. So suddenly both can open the image file, but B isn't actually
prepared for that. Fun!)

So as far as I understand it, those qdev properties should eventually be
changeable by the guest. And if that is so, the user probably shouldn't
touch them at all because the guest OS really knows whether it can cope
with volatile block devices or not, and it will tell qemu if that is so.

That may be the reason why the qdev property doesn't make sense to me at
the first glance: It does make sense for the guest OS to set this
property at that level, but it doesn't make sense for the user to do so.
Or maybe it does, but the user is really asking for things breaking then
("YES, I AM SURE THAT MY GUEST WILL BE COMPLETELY FINE WITH FLOPPY DISKS
JUST CHANGING RANDOMLY" -- but I guess we've all been at that point
ourselves...).

So after having convinced myself twice now, having the qdev property is
probably correct.

</parenthesis>

But that's where the flags come in again: The guest may be fine with a
shared lock or none at all. But what about the block layer? Say you're
using a qcow2 image; that will not like sharing at all, independently of
what the guest things. So the block layer needs to have internal
mechanisms to elevate the locks proposed by the guest devices to e.g.
exclusive ones. I don't think that is something addressed by this series
in this version. Maybe you'll still need the flags for that.


Final thing: Say you have a user who's crazy. Maybe they want to debug
something. Anyway, they have some qcow2 image attached to some guest
device, and they want to access that image simultaneously from some
other process; as I said, crazy, but there may be legitimate uses for
that so we should allow it.

Now first that user of course has to set the device's lock_mode to
shared or none, thus promising qemu that the guest will be fine with
volatile data. But of course qcow2 will override that lock mode, because
qcow2 doesn't care about the guest: It primarily cares about its own
metadata integrity. So at this point the user will need some way to
override qcow2's choice, too. Therefore, I think we also need a per-node
flag to override the locking mode.

qcow2 would probably make this flag default to exclusive for its
underlying node, and the user would then have to override that flag for
exactly that underlying node.

Therefore I think we still need a block layer property for the lock
mode. While I now agree that we do need a qdev property, I think that
alone won't be sufficient.

Max

signature.asc
Description: OpenPGP digital signature