Re: [PATCH 3/4] block/mirror: support unaligned write in active mirror

[Max Reitz]

On 11.10.19 10:33, Vladimir Sementsov-Ogievskiy wrote:
> 04.10.2019 19:31, Max Reitz wrote:
>> On 12.09.19 17:13, Vladimir Sementsov-Ogievskiy wrote:
>>> Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
>>> region in the dirty bitmap, which means that we may not copy some bytes
>>> and assume them copied, which actually leads to producing corrupted
>>> target.
>>>
>>> So 9adc1cb49af8d forced dirty bitmap granularity to be
>>> request_alignment for mirror-top filter, so we are not working with
>>> unaligned requests. However forcing large alignment obviously decreases
>>> performance of unaligned requests.
>>>
>>> This commit provides another solution for the problem: if unaligned
>>> padding is already dirty, we can safely ignore it, as
>>> 1. It's dirty, it will be copied by mirror_iteration anyway
>>> 2. It's dirty, so skipping it now we don't increase dirtiness of the
>>>     bitmap and therefore don't damage "synchronicity" of the
>>>     write-blocking mirror.
>>>
>>> If unaligned padding is not dirty, we just write it, no reason to touch
>>> dirty bitmap if we succeed (on failure we'll set the whole region
>>> ofcourse, but we loss "synchronicity" on failure anyway).
>>>
>>> Note: we need to disable dirty_bitmap, otherwise we will not be able to
>>> see in do_sync_target_write bitmap state before current operation. We
>>> may of course check dirty bitmap before the operation in
>>> bdrv_mirror_top_do_write and remember it, but we don't need active
>>> dirty bitmap for write-blocking mirror anyway.
>>>
>>> New code-path is unused until the following commit reverts
>>> 9adc1cb49af8d.
>>>
>>> Suggested-by: Denis V. Lunev <address@hidden>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/mirror.c | 39 ++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index d176bf5920..d192f6a96b 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -1204,6 +1204,39 @@ do_sync_target_write(MirrorBlockJob *job, 
>>> MirrorMethod method,
>>>                        QEMUIOVector *qiov, int flags)
>>>   {
>>>       int ret;
>>> +    size_t qiov_offset = 0;
>>> +
>>> +    if (!QEMU_IS_ALIGNED(offset, job->granularity) &&
>>> +        bdrv_dirty_bitmap_get(job->dirty_bitmap, offset)) {
>>> +            /*
>>> +             * Dirty unaligned padding
>>> +             * 1. It's already dirty, no damage to "actively_synced" if we 
>>> just
>>> +             *    skip unaligned part.
>>> +             * 2. If we copy it, we can't reset corresponding bit in
>>> +             *    dirty_bitmap as there may be some "dirty" bytes still not
>>> +             *    copied.
>>> +             * So, just ignore it.
>>> +             */
>>> +            qiov_offset = QEMU_ALIGN_UP(offset, job->granularity) - offset;
>>> +            if (bytes <= qiov_offset) {
>>> +                /* nothing to do after shrink */
>>> +                return;
>>> +            }
>>> +            offset += qiov_offset;
>>> +            bytes -= qiov_offset;
>>> +    }
>>> +
>>> +    if (!QEMU_IS_ALIGNED(offset + bytes, job->granularity) &&
>>> +        bdrv_dirty_bitmap_get(job->dirty_bitmap, offset + bytes - 1))
>>> +    {
>>> +        uint64_t tail = (offset + bytes) % job->granularity;
>>> +
>>> +        if (bytes <= tail) {
>>> +            /* nothing to do after shrink */
>>> +            return;
>>> +        }
>>> +        bytes -= tail;
>>> +    }
>>>   
>>>       bdrv_reset_dirty_bitmap(job->dirty_bitmap, offset, bytes);
>>>   
>>
>> The bdrv_set_dirty_bitmap() in the error case below needs to use the
>> original offset/bytes, I suppose.
> 
> No, because we shrink tail only if it is already dirty. And we've locked the
> region for in-flight operation, so nobody can clear the bitmap in a meantime.

True.  But wouldn’t it be simpler to understand to just use the original
offsets?

> But still, here is something to do:
> 
> for not-shrinked tails, if any, we should:
> 1. align down for reset
> 2. align up for set on failure

Well, the align up is done automatically, and I think that’s pretty
self-explanatory.

Max

>>
>> Apart from that, looks good to me.
>>
>> Max
>>
> 
>

signature.asc
Description: OpenPGP digital signature