Am 07.05.2021 um 09:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
17.03.2021 20:15, Alberto Garcia wrote:
When the x-blockdev-reopen was added it allowed reconfiguring the
graph by replacing backing files, but changing the 'file' option was
forbidden. Because of this restriction some operations are not
possible, notably inserting and removing block filters.
I now started to work on making backup-top filter public..
And I think, we'll need separate commands to insert/remove filters
anyway.. As blockdev-reopen has the following problems:
1. It can't append filter above top node, connected to block-device.
(but bdrv_append() can do it)
We once had some patches that made the 'drive' qdev property runtime
writable. What happened to them?
2. It can't simultaneously create new node and append it. This is
important for backup-top filter, which unshares write even when has no
writing parent. Now bdrv_append() works in a smart way for it: it
first do both graph modification (add child to filter, and replace
original node by filter) and then update graph permissions. So, we'll
need a command which in one roll create filter node and inserts it
where needed.
What backup-top could do, however, is enabling restrictions only if it
has a parent (no matter whether that parent requires writing or not).
3. blockdev-reopen requires to specify all options (otherwise, they
will be changed to default). So it requires passing a lot of
information. But we don't need to touch any option of original bs
parent to insert a filter between parent and bs. In other words, we
don't want to reopen something. We want to insert filter.
Yeah, but this was a decision we made consciously because otherwise we'd
have a hard time telling which options should be updated and which
shouldn't, and we would need separate QAPI types for open and reopen.
If we now say that this is a reason for avoiding blockdev-reopen even
though changing some option is the goal, that would be inconsistent.
===
Hmm, another mentioned use of blockdev-reopen was reopening some RO
node to RW, to modify bitmaps.. And here again, blockdev-reopen is not
very convenient:
1. Again, it requires to specify all options (at least, that was not
default on node open).. And this only to change one property:
read-only. Seems overcomplicated.
2. Bitmaps modifications are usually done in transactions. Adding a
clean transaction support for small command that reopens only to RW,
and back to RO on transaction finalization seems simpler, than for
entire generic blockdev-reopen.
===
Hmm, interesting. x-blockdev-reopen says that not specified options
are reset to default. x-blockdev-reopen works through
bdrv_reopen_multiple, so I think bdrv_reopen_mutliple should reset
options to default as well. Now look at bdrv_reopen_set_read_only():
it specifies only one option: "read-only". This means that all other
options will be reset to default. But for sure, callers of
bdrv_reopen_set_read_only() want only to change read-only status of
node, not all other options. Do we have a bug here?
The difference between these cases is the keep_old_opts parameter to
bdrv_reopen_queue(). It is false for x-blockdev-reopen, but true in
bdrv_reopen_set_read_only().