Re: [Qemu-block] [RFC] Intermediate block mirroring

[Alberto Garcia]

On Mon 11 Jun 2018 02:20:06 PM CEST, Kevin Wolf wrote:
> Am 01.06.2018 um 12:51 hat Alberto Garcia geschrieben:
>> On Thu 03 May 2018 02:22:41 PM CEST, Kevin Wolf wrote:
>> >> > Were the (more or less) exact requirements of QMP blockdev-reopen
>> >> > discussed? How is it different from qemu-io's "reopen" command?
>> >> > What are the options that you can and can not change?
>> >> 
>> >> I can't quite remember, I'm afraid.  I think it was supposed to be
>> >> pretty much qemu-io's reopen (so just bdrv_reopen()).  I suppose you
>> >> cannot change the driver (obviously) or probably the node name, because
>> >> either would result in the node being replaced by a completely new one.
>> >> 
>> >> Other than that, it probably depends on what the block driver supports,
>> >> but ideally you should be able to change everything.
>> >
>> > Honestly the design of bdrv_reopen() is quite broken because of the
>> > way it tries to maintain old options if they aren't specified, and
>> > guesses what you might mean when you add flags to the mix. The exact
>> > semantics are quite complicated and I'd rather avoid them in a stable
>> > API.
>> >
>> > A clean QMP command would probably apply the same defaults as
>> > blockdev-add, so you just get to specify the full options again.
>> 
>> I have a prototype of this working and almost ready to be published, but
>> there's a tricky thing with this part:
>> 
>> If we want blockdev-reopen to apply the defaults for all options except
>> from the ones expliclity specified by the user, then it means that we
>> need to check not just the options that are present, but also the ones
>> that are omitted.
>> 
>> For example:
>> 
>>    { "execute": "blockdev-add",
>>      "arguments": { "driver": "null-aio",
>>                     "node-name": "root",
>>                     "size": 1024 }
>> 
>> This adds a null-aio block device with the "size" option set to 1024
>> (the default is 1 << 30).
>> 
>> null_reopen_prepare() allows reopening that block device, but it does
>> not allow changing any of its options. Attempting to change the value of
>> "size" is detected by the loop that checks unhandled options at the end
>> of bdrv_reopen_prepare() and returns "Cannot change the option 'size'".
>> 
>> So far, so good. We have this generic check for all options that works
>> with all drivers, so as long as we only specify options that we know
>> that can be changed, everything is fine.
>> 
>> However if we want blockdev-reopen to apply the default values for all
>> omitted options, then omitting "size" would be equivalent to setting it
>> to its default value (1 << 30). And if "size" cannot be changed then
>> QEMU should complain unless we explicitly set "size" to 1024 again on
>> reopen.
>> 
>> This complicates things a bit, because we would go from "the options
>> that can't be changed are the ones that are not handled by each driver's
>> _prepare() function" to "options that are absent can also produce an
>> error".
>
> To be honest, I think this is fine. If the user can specify the size
> once (in blockdev-add), they can do it again (in blockdev-reopen).
>
> We just need to make sure that we don't break existing bdrv_reopen*()
> calls that come from places other than the monitor.

Good. I have the first revision of the series almost ready, expect it
this week.

Berto