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

[Vladimir Sementsov-Ogievskiy]

04.10.2019 17:48, Max Reitz wrote:
> On 04.10.19 15:22, Vladimir Sementsov-Ogievskiy wrote:
>> 04.10.2019 15:59, Max Reitz wrote:
>>> On 03.10.19 11:34, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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
>>>
>>> Well, the thing is that I’d find it weird if write-blocking wasn’t
>>> blocking in all cases.  And in my opinion, it makes more sense for
>>> active mirror if all writes actively contributed to convergence.
>>>
>>
>> Why? What is the benefit in it?
>> What is "all writes actively contributed to convergence" for user?
> 
> One thing I wonder about is whether it’s really guaranteed that the
> background job will run under full I/O load, and how often it runs.
> 
> I fear that with your model, the background job might starve and the
> mirror may take a very long time.

Hmmm. I think mirror job is in same context as guest writes, and goes
through same IO api.. Why it will starve? (I understand that my words
are not an evidence...).

>  It won’t diverge, but it also won’t
> really converge.

But same will be with current behavior: guest is not guaranteed to write
to all parts of disk. And in most scenarios it doesn't. So, if mirror job
starve because of huge guest IO load, we will not converge anyway.

So, background process is necessary thing for converge anyway.

> 
> The advantage of letting all writes block is that even under full I/O
> load, the mirror job will progress at a steady pace.
> 
>> I think for user there may be the following criteria:
>>
>> 1. guaranteed converge, with any guest write load.
>> Both current and my proposed variants are OK.
>>
>> 2. Less impact on guest.
>> Obviously my proposed variant is better
>>
>> 3. Total time of mirroring
>> Seems, current may be a bit better, but I don't think that unaligned
>> tails gives significant impact.
>>
>> ===
>>
>> So, assume I want [1]+[2]. And possibly
>> 2.2: Even less impact on guest: ignore not only unaligned tails if they are
>> already dirty, but full synchronous mirror operation if area is already 
>> dirty.
>>
>> How should I call this? Should it be separate mode, or option for 
>> write-blocking?
> 
> I don’t know whether it makes sense to add a separate mode or a separate
> option just for this difference.  I don’t think anyone would choose the
> non-default option.

At least Virtuozzo will choose :)

> 
> But I do think there’s quite a bit of difference in how the job behaves
> still...  I don’t know. :-/
> 
> Max
> 


-- 
Best regards,
Vladimir