Re: [Qemu-devel] [PATCH 00/13] Add a 'x-blockdev-reopen' QMP command

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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
>>
> 
> 


-- 
Best regards,
Vladimir