On 31.10.2016 16:38, Fam Zheng wrote:
> This is v9 of the image locking series. I redid the whole series, adopting the
> "two locks" approach from Kevin and Max.
> Depends on "[Qemu-devel] [PATCH] raw-posix: Rename 'raw_s' to 'rs'" in Max's
> block branch.
> Fam Zheng (14):
>   osdep: Add qemu_lock_fd and qemu_unlock_fd
>   block: Define BDRV_O_SHARE_RW
>   qemu-io: Set "share-rw" flag together with read-only
>   qemu-img: Set "share-rw" flag in read-only commands
>   block: Set "share-rw" flag in drive-backup when sync=none
>   block: Set "share-rw" flag for incoming migration
>   iotests: 055: Don't attach the drive to vm for drive-backup
>   iotests: 030: Read-only open image for getting map
>   iotests: 087: Don't attch test image twice
>   iotests: 085: Avoid image locking conflict
>   iotests: 091: Quit QEMU before checking image
>   tests: Use null-co:// instead of /dev/null as the dummy image
>   raw-posix: Implement image locking
>   tests: Add test-image-lock

One issue I have with the series in the current state is that it does
not involve the format layer. For raw images, it's fine to be shared if
BDRV_O_SHARE_RW comes from the block backend, but for qcow2 images it's
not. Therefore, most drivers in the format layer should force
BDRV_O_SHARE_RW to be off for the protocol layer.

In fact, BDRV_O_SHARE_RW does not mean that an image has to allow
sharing, but it means that the block backend (i.e. the user of the block
device) is fine with sharing. Therefore, it's actually very fine to set
this flag for the drive-backup target because we can easily justify not
to care about concurrent writes there.

However, we have to have a way to override locking, and you have four
patches in your series that look like they're trying to do this:

We don't really have to override locking in patches 3 and 4, it's enough
to give a hint to the block layer that sharing is fine (i.e. to set
BDRV_O_SHARE_RW). If something in the block layer overrides this
decision, we can just let the user override it again (by setting

Patch 5 can easily justify setting BDRV_O_SHARE_RW (as said above), but
it cannot justify setting disable-lock, and there is no user to do so.
If the target image is e.g. in qcow2 format, we should not just
force-share the image but we should fix drive-backup.

Patch 6 on the other hand is very justified in force-sharing the image.
Unfortunately, it doesn't really do that. Setting BDRV_O_SHARE_RW should
not do that, as I said above. It should ideally set disable-lock. But
then again, patch 13 does all kinds of funny force-sharing based on
whether BDRV_O_INACTIVE is set, so maybe this could be the general
model: Would it work to drop patch 6 and just make raw-posix always keep
the image unlocked if BDRV_O_INACTIVE is set?

(The issue with setting disable-lock internally is furthermore that it's
raw-posix-specific (and I think that is justified) -- you cannot just
set it blindly and hope it gets to the right BDS eventually.)


Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to