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

[Max Reitz]

On 05.08.19 11:56, Kevin Wolf wrote:
> Am 02.08.2019 um 23:19 hat Max Reitz geschrieben:
>> 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, I’d prefer for hbitmap_reset() to assert
>> that the boundaries are aligned (for 4.2), and for
>> do_sync_target_write() to be fixed (for 4.1? :-/).
>>
>> (A practical problem with this patch is that do_sync_target_write() will
>> still do the write, but it won’t change anything in the bitmap, so the
>> copy operation was effectively useless.)
>>
>> I don’t know how to fix mirror exactly, though.  I have four ideas:
>>
>> (A) Quick fix 1: do_sync_target_write() should shrink [offset, offset +
>> bytes) such that it is aligned.  This would make it skip writes that
>> don’t fill one whole chunk.
>>
>> +: Simple fix.  Could go into 4.1.
>> -: Makes copy-mode=write-blocking equal to copy-mode=background unless
>>    you set the granularity to like 512. (Still beats just being
>>    completely broken.)
> 
> write-blocking promises that the guest receives request completion only
> when the request has also been written to the target. If you completely
> skip the write, this promise is broken.
> 
> So I think you'd have to keep the write and only align the range for the
> purpose of clearing bits in the dirty bitmap. This would result in some
> duplicated I/O, which is an efficiency problem, but at least it
> shouldn't come with a correctness problem.

Hm.  I was thinking that the use case we were mostly thinking about is
people wanting their mirror job to definitely converge.  Doing that
wouldn’t guarantee that.

You’re right that I shouldn’t constrict people in what they might use
write-blocking for; maybe they mostly want to be sure the data is in the
target and don’t care too much about convergence.

In any case, what you describe is fulfilled by this patch here.  So we
may as well just take it, then.

(Unless we decide that we’d rather make write-blocking fully do what
it’s supposed to do, even at the cost of being slow, by announcing a
request_alignment, as described in (B).)

Max

signature.asc
Description: OpenPGP digital signature