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

[Max Reitz]

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.  It won’t diverge, but it also won’t
really converge.

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.

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

Max

signature.asc
Description: OpenPGP digital signature