| Message ID | cover.1547739122.git.berto@igalia.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add a 'x-blockdev-reopen' QMP command | expand |
On 1/17/19 9:33 AM, Alberto Garcia wrote: > > 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). Could we use QAPI alternate types where we could state that: "option":"new" - set the option "option":null - reset the option to its default omit option - leave the option unchanged basically, making each of the options an Alternate with JSON null, so that a request to reset to the default becomes an explicit action? > > 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. Hmm - that makes my proposal of "option":null as an explicit request to the default a bit trickier, if we are already using null for a different meaning for backing files. > > 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 >
On Thu 17 Jan 2019 04:50:20 PM CET, Eric Blake wrote: >> Removing the backing file can be done by simply passing the option { >> ..., "backing": null } to x-blockdev-reopen. >> > Hmm - that makes my proposal of "option":null as an explicit request > to the default a bit trickier, if we are already using null for a > different meaning for backing files. At the moment 'backing' seems to be the only option that allows a null value (BlockdevRefOrNull in particular). I suppose there could be more in the future? Also, having 'null' meaning 'reset to default' would make this API differ from blockdev-add, which I'm trying not to do. Berto
17.01.2019 18:50, Eric Blake wrote: > On 1/17/19 9:33 AM, Alberto Garcia wrote: > >> >> 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). > > Could we use QAPI alternate types where we could state that: > > "option":"new" - set the option > "option":null - reset the option to its default > omit option - leave the option unchanged > > basically, making each of the options an Alternate with JSON null, so > that a request to reset to the default becomes an explicit action? +1 Sorry, I missed the previous discussion. What is the reason to reset option to default if it missed? It looks more common to not touch things which are not asked to. Also, should we consider an option which may be changed during life-cycle of a device not by user request to change it? If yes, it should be safer to not reset it. > >> >> 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. > > Hmm - that makes my proposal of "option":null as an explicit request to > the default a bit trickier, if we are already using null for a different > meaning for backing files. And here: if we are going to reset to default for not listed options, than just not listing "backing" should remove it (as default is null, obviously), and we don't need this special "null" parameter. Moreover, backing is an example of an option I mentioned, it definitely may change after block-stream for example, so resetting to default is unsafe, and user will have to carefully set backing on reopen (not tha backing, that was used with first blockdev-add, but backing which we have after block-stream) > >> >> 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 >> > >
On Tue 22 Jan 2019 09:15:25 AM CET, Vladimir Sementsov-Ogievskiy wrote: >>> 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). >> >> Could we use QAPI alternate types where we could state that: >> >> "option":"new" - set the option >> "option":null - reset the option to its default >> omit option - leave the option unchanged >> >> basically, making each of the options an Alternate with JSON null, so >> that a request to reset to the default becomes an explicit action? > > +1 > > Sorry, I missed the previous discussion. What is the reason to reset > option to default if it missed? It looks more common to not touch > things which are not asked to. The idea is to have a command that works like blockdev-add. If you pass a set of options to x-blockdev-reopen then the end result should be similar to what you would get if you had used blockdev-add. > Also, should we consider an option which may be changed during > life-cycle of a device not by user request to change it? If yes, it > should be safer to not reset it. Do you have any example in mind? > And here: if we are going to reset to default for not listed options, > than just not listing "backing" should remove it (as default is null, > obviously), and we don't need this special "null" parameter. This is a bit trickier: the default for 'backing' is not to omit the backing file, but to open the one that is specified in the image metadata. $ qemu-img create -f qcow2 base.qcow2 1M $ qemu-img create -f qcow2 -b base.qcow2 active.qcow2 If you open active.qcow2 with blockdev-add then base.qcow2 is opened as its backing file, and the only way to prevent that is to specify a different backing file or null if you don't want to open any. For x-blockdev-reopen we want to keep the same behavior as much as possible, but instead of opening base.qcow2 if 'backing' is missing we force the user to specify that option. > Moreover, backing is an example of an option I mentioned, it > definitely may change after block-stream for example, so resetting to > default is unsafe, and user will have to carefully set backing on > reopen (not tha backing, that was used with first blockdev-add, but > backing which we have after block-stream) Exactly. Since opening the default backing file is not something that we want to do during a reopen operation we don't allow leaving that option out. Berto
ping On Thu 17 Jan 2019 04:33:51 PM CET, Alberto Garcia wrote: > Hi, > > here's a patch series to implement a QMP command for bdrv_reopen(). > This is not too different from the previous iteration (RFC v2, see > changes below), but I'm not tagging it as RFC any longer. > > 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.
Hi, here's a patch series to implement a QMP command for bdrv_reopen(). This is not too different from the previous iteration (RFC v2, see changes below), but I'm not tagging it as RFC any longer. 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 v1: - Patch 10: forbid setting a new backing file with a different AioContext. - Patch 13 (new): Remove unused parameter from bdrv_reopen_multiple. - Patch 14: Acquire the AioContext before calling bdrv_reopen_multiple(). - Patch 15: More test cases. - Patches 3, 8, 9, 11, 12: scripts/checkpatch.pl is more picky now with the format of multi-line comments, so correct them. RFCv2: https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00901.html Alberto Garcia (13): 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: Remove the AioContext parameter from bdrv_reopen_multiple() block: Add an 'x-blockdev-reopen' QMP command qemu-iotests: Test the x-blockdev-reopen QMP command block.c | 333 +++++++++++++-- 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 | 7 +- block/sheepdog.c | 2 + block/stream.c | 16 + block/vpc.c | 1 + blockdev.c | 47 +++ include/block/block.h | 11 +- include/block/block_int.h | 12 + qapi/block-core.json | 42 ++ qemu-io-cmds.c | 4 +- tests/qemu-iotests/224 | 1001 ++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/224.out | 5 + tests/qemu-iotests/group | 1 + 23 files changed, 1484 insertions(+), 33 deletions(-) create mode 100644 tests/qemu-iotests/224 create mode 100644 tests/qemu-iotests/224.out