Re: x-blockdev-reopen & block-dirty-bitmaps

[John Snow]

On 2/17/20 4:52 AM, Kevin Wolf wrote:
> Am 14.02.2020 um 21:32 hat John Snow geschrieben:
>> On 2/14/20 3:19 PM, Kevin Wolf wrote:

>>> I think I've asked this before, but I don't remember the answer...
>>>
>>> What would be the problem with letting the enable/disable command
>>> temporarily reopen the backing file read-write, like the commit job [1]
>>> does?
>>>
>>
>> I guess no problem? I wouldn't want it to do this automatically, but
>> perhaps we could add a "force=True" bool where it tries to do just this.
>>
>> (And once reopen works properly we could deprecate this workaround again.)
> 
> I'm not sure if I would view this only as a workaround, but if you like
> it better with force=true, I won't object either.
> 

It feels like the wrong place to manage node permissions. I could be
wrong, but I felt like bitmap commands should try to stay focused on
managing bitmaps instead of graph management.

At the very least, I feel like we ought to require 'force' so that
management clients like libvirt can be aware that this will (attempt to)
change the graph in those cases.

Thanks for the suggestion!

>>> [1] I mean, I know that this code is wrong strictly speaking because we
>>>     really should be counting read-write users [2] rather than blindly
>>>     making the node read-only at the end of the operation - but somehow
>>>     it seems to work in practice for commit jobs.
>>>
>>> [2] Counting really means just looking at parent BdrvChild links that
>>>     have WRITE permissions. I guess doing it right would mean getting
>>>     rid of BlockDriverState.read_only (which is redundant) and using
>>>     only permissions.
>>
>> OK, sounds good. I'll make a mockup that tries to accurately detect
>> read-only-ness and reverts changes only if it made any to begin with.
> 
> Fixing it for commit would be appreciated, though as I said it seems to
> be a theoretical case mostly because we never got bug reports for it.
> 
> For bitmaps it's even more theoretical because we hold the BQL between
> the switch to read-write and the switch back. So I don't think we can
> actually run into this case for the bitmap enable/disable operation.
> 

Yes, only a theoretical problem -- but personally I find it confusing to
have competing APIs that don't always agree, so it's just in my nature
to try and "use the one that's more correct."

Thanks again.
--js