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 disable-lock). 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.) Max
signature.asc
Description: OpenPGP digital signature