Re: [PATCH 3/9] mirror: implement mirror_change method

2023-03-02 Thread Fiona Ebner
Am 01.03.23 um 16:18 schrieb Vladimir Sementsov-Ogievskiy:
> On 24.02.23 17:48, Fiona Ebner wrote:
>> which allows switching the @copy-mode from 'background' to
>> 'write-blocking'.
>>
>> Once the job is in active mode, no new writes need to be registered in
>> the dirty bitmap, because they are synchronously written to the
>> target. But since the method is called from the monitor and IO might
>> be happening in an iothread at the same time, a drained section is
>> needed.
>>
>> This is useful for management applications, so they can start out in
>> background mode to avoid limiting guest write speed and switch to
>> active mode when certain criteria are fullfilled. Currently, the
>> amount of information that can be used for those criteria is a bit
>> limited, so the plan is to extend quering of block jobs to return more
>> information relevant for mirror.
>>
>> Signed-off-by: Fiona Ebner 
>> ---
>>
>> Sorry, I still haven't fully grasped the drained logic. Is my
>> rationale for the drained section correct? There also are some yield
>> points in block/io.c, for example when the driver implements
>> bdrv_aio_pwritev (file-win32 and null), and the bitmap is only updated
>> after that. Is that another reason it's required?
>>
> 
> I think your logic is correct. But draining is quite expensive.
> 
> Could we avoid it? For example, just count separately
> in_flight_non_active writes (the writes that are started with copy_mode
> = BACKGROUND), and disable dirty bitmap when this counter becomes 0.
> 
> Or better idea: move the dirty bitmap to the filter, and make it always
> disabled. Then, we can set it by hand in bdrv_mirror_top_do_write() only
> when copy_to_target = false.
> 
Sounds good to me! I'll try to go for the second approach in the next
version.

Best Regards,
Fiona




Re: [PATCH 3/9] mirror: implement mirror_change method

2023-03-01 Thread Vladimir Sementsov-Ogievskiy

On 24.02.23 17:48, Fiona Ebner wrote:

which allows switching the @copy-mode from 'background' to
'write-blocking'.

Once the job is in active mode, no new writes need to be registered in
the dirty bitmap, because they are synchronously written to the
target. But since the method is called from the monitor and IO might
be happening in an iothread at the same time, a drained section is
needed.

This is useful for management applications, so they can start out in
background mode to avoid limiting guest write speed and switch to
active mode when certain criteria are fullfilled. Currently, the
amount of information that can be used for those criteria is a bit
limited, so the plan is to extend quering of block jobs to return more
information relevant for mirror.

Signed-off-by: Fiona Ebner 
---

Sorry, I still haven't fully grasped the drained logic. Is my
rationale for the drained section correct? There also are some yield
points in block/io.c, for example when the driver implements
bdrv_aio_pwritev (file-win32 and null), and the bitmap is only updated
after that. Is that another reason it's required?



I think your logic is correct. But draining is quite expensive.

Could we avoid it? For example, just count separately in_flight_non_active 
writes (the writes that are started with copy_mode = BACKGROUND), and disable 
dirty bitmap when this counter becomes 0.

Or better idea: move the dirty bitmap to the filter, and make it always 
disabled. Then, we can set it by hand in bdrv_mirror_top_do_write() only when 
copy_to_target = false.


--
Best regards,
Vladimir