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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

02.10.2019 18:52, Max Reitz wrote:
> On 02.10.19 17:06, Vladimir Sementsov-Ogievskiy wrote:
>> 02.10.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>> 02.10.2019 17:57, 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.
>>>>
>>>> But that’s not what active mirror is for.  The point of active mirror is
>>>> that it must converge because every guest write will contribute towards
>>>> that goal.
>>>>
>>>> If you skip active mirroring for unaligned guest writes, they will not
>>>> contribute towards converging, but in fact lead to the opposite.
>>>>
>>>
>>> The will not contribute only if region is already dirty. Actually, after
>>> first iteration of mirroring (copying the whole disk), all following writes
>>> will contribute, so the whole process must converge. It is a bit similar with
>>> running one mirror loop in normal mode, and then enable write-blocking.
>>>
>>
>>
>> In other words, we don't need "all guest writes contribute" to converge,
>> "all guest writes don't create new dirty bits" is enough, as we have parallel
>> mirror iteration which contiguously handles dirty bits.
> 
> Hm, in a sense.  But it does mean that guest writes will not contribute
> to convergence.
> 
> And that’s against the current definition of write-blocking, which does
> state that “when data is written to the source, write it (synchronously)
> to the target as well”.
> 

Hmm, understand. But IMHO our proposed behavior is better in general.
Do you think it's a problem to change spec now?
If yes, I'll resend with an option


-- 
Best regards,
Vladimir