Re: [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset

[John Snow]

On 8/6/19 8:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.08.2019 0:19, Max Reitz wrote:
>> On 02.08.19 20:58, Vladimir Sementsov-Ogievskiy wrote:
>>> hbitmap_reset is broken: it rounds up the requested region. It leads to
>>> the following bug, which is shown by fixed test:
>>>
>>> assume granularity = 2
>>> set(0, 3) # count becomes 4
>>> reset(0, 1) # count becomes 2
>>>
>>> But user of the interface assume that virtual bit 1 should be still
>>> dirty, so hbitmap should report count to be 4!
>>>
>>> In other words, because of granularity, when we set one "virtual" bit,
>>> yes, we make all "virtual" bits in same chunk to be dirty. But this
>>> should not be so for reset.
>>>
>>> Fix this, aligning bound correctly.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>
>>> Hi all!
>>>
>>> Hmm, is it a bug or feature? :)
>>> I don't have a test for mirror yet, but I think that sync mirror may be 
>>> broken
>>> because of this, as do_sync_target_write() seems to be using unaligned 
>>> reset.
>>
>> Crap.
>>
>>
>> Yes, you’re right.  This would fix it, and it wouldn’t fix it in the
>> worst way.
>>
>> But I don’t know whether this patch is the best way forward still.  I
>> think call hbitmap_reset() with unaligned boundaries generally calls for
>> trouble, as John has laid out.  If mirror’s do_sync_target_write() is
>> the only offender right now, 
> 
> Another thing is migration/block. Should we care of it, is it supported at 
> all?
> 

Downstream products always have time and room to get additional fixes; I
think this is supported from an upstream POV so we should investigate this.

I assume migration/block has the same problem that it fully clears
unaligned blocks?

(I can look shortly but can't just yet.)

--js