Re: [Qemu-devel] [PATCH v3 1/9] block: add .bdrv_need_rw_file_child_during_reopen_rw handler

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2306 bytes --]

On 01.08.19 16:02, Vladimir Sementsov-Ogievskiy wrote:
> 31.07.2019 15:09, Max Reitz wrote:

[...]

>> So -- without having tried, of course -- I think a better design would
>> be to look for bs->file->bs in the ReopenQueue, recursively all of its
>> children, and move all of those entries into a new queue, and then
>> invoke bdrv_reopen_multiple() on that one first.
> 
> Why all children recursively? Shouldn't we instead only follow defined
> dependencies?
> Or it just seems bad to put not full subtree into bdrv_reopen multiple() ?

For example, making a node RW without opening its children RW seems
wrong.  Whenever we reopen a node, we should reopen all of its children,
if they are in the queue.

>> The question then becomes how to roll back those changes...  I don’t
>> know whether just having bdrv_reopen() partially succeed is so bad.
>> Otherwise, we’d need a function to translate an existing node's state
>> into a BdrvReopenQueueEntry so we can thus return to the old state.
> 
> And this rollback may fail too and we are still in partial success state.
> 
> But if we roll-back simple ro->rw reopen it's safe enough: in worst case
> file will be rw, but marked ro, so Qemu may have more access rights than
> needed but will not use them, this is why I was concentrating around
> only ro->rw case..

In practice, this is always so.  The “children need to be reopened
before parent” case is always a sign of more permissions being taken;
whereas “children need to be reopened after parent” is a sign of
permissions being released.

What we want to fix now is the former “reopen children before parent”
case.  Because this is always a sign of taking more permissions, a
partial success/failure state means we always have taken more
permissions than we need to.

> So, what about go similar way to this patch, i.e. only reopen ro->rw children
> which we need to be rw, not touching other flags, but check, that in reopen
> queue we have this child, and it is going to be reopened RW, and if not,
> return error immediately?

If the RO -> RW change for the child is accompanied by other options
being changed, the user may find it vital to change these flags along
with the RO/RW access.  We shouldn’t ignore them.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]