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

[Vladimir Sementsov-Ogievskiy]

01.08.2019 22:06, Max Reitz wrote:
> 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.

OK, I'll try.

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

Hmm, understand, OK.


-- 
Best regards,
Vladimir