Hi,
here's a new attempt to implement a bdrv_reopen() QMP command. The
first attempt happened some months ago and the code has changed
significantly since then (including two additional patch series), but
here's the link to the previous version for reference:
https://lists.gnu.org/archive/html/qemu-block/2018-06/msg00794.html
I'm still tagging this as RFC because I expect that there will be some
more debate but I think the patches themselves are in pretty good
shape, so if there are no major concerns I think the final version
will be very similar to this one.
This is based on my "Don't pass flags to bdrv_reopen_queue()" v5
series, available on Kevin's block-next branch and originally
published here:
https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00329.html
The new command is called x-blockdev-reopen, and it's designed to be
similar to blockdev-add. It also takes BlockdevOptions as a
parameter. The "node-name" field of BlockdevOptions must be set, and
it is used to select the BlockDriverState to reopen.
In this example "hd0" is reopened with the given set of options:
{ "execute": "x-blockdev-reopen",
"arguments": {
"driver": "qcow2",
"node-name": "hd0",
"file": {
"driver": "file",
"filename": "hd0.qcow2",
"node-name": "hd0f"
},
"l2-cache-size": 524288
}
}
Changing options
================
The general idea is quite straightforward, but the devil is in the
details. Since this command tries to mimic blockdev-add, the state of
the block device after being reopened should generally be equivalent
to that of a block device added with the same set of options.
There are two important things with this:
a) Some options cannot be changed (most drivers don't allow that, in
fact).
b) If an option is missing, it should be reset to its default value
(rather than keeping its previous value).
Example: the default value of l2-cache-size is 1MB. If we set a
different value (2MB) on blockdev-add but then omit the option in
x-blockdev-reopen, then it should be reset back to 1MB. The current
bdrv_reopen() code keeps the previous value.
Trying to change an option that doesn't allow it is already being
handled by the code. The loop at the end of bdrv_reopen_prepare()
checks all options that were not handled by the block driver and sees
if any of them has been modified.
However, as I explained earlier in (b), omitting an option is supposed
to reset it to its default value, so it's also a way of changing
it. If the option cannot be changed then we should detect it and
return an error. What I'm doing in this series is making every driver
publish its list of run-time options, and which ones of them can be
modified.
Backing files
=============
This command allows reconfigurations in the node graph. Currently it
only allows changing an image's backing file, but it should be
possible to implement similar functionalities in drivers that have
other children, like blkverify or quorum.
Let's add an image with its backing file (hd1 <- hd0) like this:
{ "execute": "blockdev-add",
"arguments": {
"driver": "qcow2",
"node-name": "hd0",
"file": {
"driver": "file",
"filename": "hd0.qcow2",
"node-name": "hd0f"
},
"backing": {
"driver": "qcow2",
"node-name": "hd1",
"file": {
"driver": "file",
"filename": "hd1.qcow2",
"node-name": "hd1f"
}
}
}
}
Removing the backing file can be done by simply passing the option {
..., "backing": null } to x-blockdev-reopen.
Replacing it is not so straightforward. If we pass something like {
..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen will
assume that we're trying to change the options of the existing backing
file (and it will fail in most cases because it'll likely think that
we're trying to change the node name, among other options).
So I decided to use a reference instead: { ..., "backing": "hd2" },
where "hd2" is of an existing node that has been added previously.
Leaving out the "backing" option can be ambiguous on some cases (what
should it do? keep the current backing file? open the default one
specified in the image file?) so x-blockdev-reopen requires that this
option is present. For convenience, if the BDS doesn't have a backing
file then we allow leaving the option out.
As you can see in the patch series, I wrote a relatively large amount
of tests for many different scenarios, including some tricky ones.
Perhaps the part that I'm still not convinced about is the one about
freezing backing links to prevent them from being removed, but the
implementation was quite simple so I decided to give it a go. As
you'll see in the patches I chose to use a bool instead of a counter
because I couldn't think of a case where it would make sense to have
two users freezing the same backing link.
Thanks,
Berto
Alberto Garcia (12):
block: Allow freezing BdrvChild links
block: Freeze the backing chain for the duration of the commit job
block: Freeze the backing chain for the duration of the mirror job
block: Freeze the backing chain for the duration of the stream job
block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()
block: Handle child references in bdrv_reopen_queue()
block: Allow omitting the 'backing' option in certain cases
block: Allow changing the backing file on reopen
block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver
block: Add bdrv_reset_options_allowed()
block: Add an 'x-blockdev-reopen' QMP command
qemu-iotests: Test the x-blockdev-reopen QMP command
block.c | 299 ++++++++++++--
block/blkdebug.c | 1 +
block/commit.c | 8 +
block/crypto.c | 1 +
block/file-posix.c | 10 +
block/iscsi.c | 2 +
block/mirror.c | 8 +
block/null.c | 2 +
block/nvme.c | 1 +
block/qcow.c | 1 +
block/rbd.c | 1 +
block/replication.c | 4 +-
block/sheepdog.c | 2 +
block/stream.c | 16 +
block/vpc.c | 1 +
blockdev.c | 43 ++
include/block/block.h | 9 +-
include/block/block_int.h | 10 +
qapi/block-core.json | 42 ++
qemu-io-cmds.c | 2 +-
tests/qemu-iotests/224 | 970 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/224.out | 5 +
tests/qemu-iotests/group | 1 +
23 files changed, 1412 insertions(+), 27 deletions(-)
create mode 100644 tests/qemu-iotests/224
create mode 100644 tests/qemu-iotests/224.out
--
2.11.0