Re: [Qemu-devel] [PATCH 06/12] block/dirty-bitmap: add bdrv_dirty_bitmap_claim

[John Snow]

On 6/21/19 7:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2019 19:36, John Snow wrote:
>>
>>
>> On 6/20/19 12:02 PM, Max Reitz wrote:
>>> On 20.06.19 03:03, John Snow wrote:
>>>> This function can claim an hbitmap to replace its own current hbitmap.
>>>> In the case that the granularities do not match, it will use
>>>> hbitmap_merge to copy the bit data instead.
>>>
>>> I really do not like this name because to me it implies a relationship
>>> to bdrv_reclaim_dirty_bitmap().  Maybe just bdrv_dirty_bitmap_take()?
>>> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal().
>>> (Because references are taken or stolen.)
>>>
>>
>> take or steal is good. I just wanted to highlight that it truly takes
>> ownership. The double-pointer and erasing the caller's reference for
>> emphasis of this idea.
> 
> Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function
> actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap?
> 

:)

> And to be serious: it is the point where we definitely should drop 
> HBitmap:meta
> field, as it in bad relation with parent hbitmap stealing.
> 

You're right, I didn't consider how this would interact with that. Allow
me the time to re-audit how this feature works, there's clearly a lot of
problems with what I've proposed for cross-granularity merging.

>>
>>> The latter might fit in nicely with the abdication theme.  We’d just
>>> need to rename bdrv_reclaim_dirty_bitmap() to
>>> bdrv_dirty_bitmap_backstab(), and it’d be perfect.
>>>
>>
>> Don't tempt me; I do like my silly function names. You are lucky I don't
>> call
>>
>>> (On another note: bdrv_restore_dirty_bitmap() vs.
>>> bdrv_dirty_bitmap_restore() – really? :-/)
>>>
>>
>> I have done a terrible job at enforcing any kind of consistency here,
>> and it gets me all the time too. I had a big series that re-arranged and
>> re-named a ton of stuff just to make things a little more nicer, but I
>> let it bitrot because I didn't want to deal with the bike-shedding.
>>
>> I do agree I am in desperate need of a spring cleaning in here.
>>
>> One thing that does upset me quite often is that the canonical name for
>> the structure is "bdrv dirty bitmap", which makes the function names all
>> quite long.
>>
>>>> Signed-off-by: John Snow <address@hidden>
>>>> ---
>>>>   include/block/block_int.h |  1 +
>>>>   include/qemu/hbitmap.h    |  8 ++++++++
>>>>   block/dirty-bitmap.c      | 14 ++++++++++++++
>>>>   util/hbitmap.c            |  5 +++++
>>>>   4 files changed, 28 insertions(+)
>>>
>>> The implementation looks good to me.
>>>
>>> Max
>>>
>>
> 
>