[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 2/8] block: Add auto-read-only option
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v2 2/8] block: Add auto-read-only option |
Date: |
Fri, 12 Oct 2018 11:47:18 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 10/12/18 6:55 AM, Kevin Wolf wrote:
If a management application builds the block graph node by node, the
protocol layer doesn't inherit its read-only option from the format
layer any more, so it must be set explicitly.
The documentation for this option is consciously phrased in a way that
allows QEMU to switch to a better model eventually: Instead of trying
when the image is first opened, making the read-only flag dynamic and
changing it automatically whenever the first BLK_PERM_WRITE user is
attached or the last one is detached would be much more useful
behaviour.
Unfortunately, this more useful behaviour is also a lot harder to
implement, and libvirt needs a solution now before it can switch to
-blockdev, so let's start with this easier approach for now.
I agree both with the approach of getting the simpler implementation in
now (always writable, even when we don't need to write) as well as
wording the documentation to permit a future stricter approach (only
writable at the points where we need to write).
Signed-off-by: Kevin Wolf <address@hidden>
---
qapi/block-core.json | 6 ++++++
include/block/block.h | 2 ++
block.c | 21 ++++++++++++++++++++-
block/vvfat.c | 1 +
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..3a899298de 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,11 @@
# either generally or in certain configurations. In this case,
# the default value does not work and the option must be
# specified explicitly.
+# @auto-read-only: if true, QEMU may ignore the @read-only option and
+# automatically decide whether to open the image read-only or
+# read-write (and switch between the modes later), e.g.
+# depending on whether the image file is writable or whether a
+# writing user is attached to the node (default: false).
Bike-shedding: Do we really want to ignore @read-only? Here's the table
of 9 combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows
that must be preserved for back-compat:
RO Auto effect
o o *open for write, fail if not possible
f o *open for write, fail if not possible
t o *open for read, no conversion to write
o f open for write, fail if not possible
f f open for write, fail if not possible
t f open for read, no conversion to write
o t attempt write but graceful fall back to read
f t attempt write but graceful fall back to read
t t ignore RO flag, attempt write anyway
That last row is weird, why not make it an explicit error instead of
ignoring the implied difference in semantics between the two?
Or, another idea: is it worth trying to support a single tri-state
member (via an alternative between bool and enum, since the existing
code uses a JSON bool):
"read-only": false (open for write, fail if not possible)
"read-only": true (open read-only, no later switching)
"read-only": "auto" (switch as needed; or for initial implementation
attempt for write with graceful fallback to read)
omitting read-only: same as "read-only":false for back-compat
@@ -1328,6 +1338,11 @@ QemuOptsList bdrv_runtime_opts = {
.type = QEMU_OPT_BOOL,
.help = "Node is opened in read-only mode",
},
+ {
+ .name = BDRV_OPT_AUTO_READ_ONLY,
+ .type = QEMU_OPT_BOOL,
+ .help = "Node can become read-only if opening read-write fails",
+ },
If we keep your current approach, is it worth mentioning that
auto-read-only true overrides read-only true?
The code looks okay, but I'd like discussion on the bikeshed points
before giving R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[Qemu-block] [PATCH v2 4/8] nbd: Support auto-read-only option, Kevin Wolf, 2018/10/12
[Qemu-block] [PATCH v2 7/8] gluster: Support auto-read-only option, Kevin Wolf, 2018/10/12