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

[Vladimir Sementsov-Ogievskiy]

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.

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


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


-- 
Best regards,
Vladimir