Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query

[Fiona Ebner]

Am 02.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
> On 02.03.23 15:34, Fiona Ebner wrote:
>> Am 02.03.23 um 11:13 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 02.03.23 13:00, Fiona Ebner wrote:
>>>> Am 01.03.23 um 17:31 schrieb Vladimir Sementsov-Ogievskiy:
>>>>> On 24.02.23 17:48, Fiona Ebner wrote:
>>>>>> This can be used by management applications starting with a job in
>>>>>> background mode to determine when the switch to active mode should
>>>>>> happen.
>>>>>>
>>>>>> Suggested-by: Vladimir Sementsov-Ogievskiy
>>>>>> <vsementsov@yandex-team.ru>
>>>>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>>>>> ---
>>>>>>     block/mirror.c       | 1 +
>>>>>>     qapi/block-core.json | 4 +++-
>>>>>>     2 files changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/mirror.c b/block/mirror.c
>>>>>> index 02b5bd8bd2..ac83309b82 100644
>>>>>> --- a/block/mirror.c
>>>>>> +++ b/block/mirror.c
>>>>>> @@ -1259,6 +1259,7 @@ static void mirror_query(BlockJob *job,
>>>>>> BlockJobInfo *info)
>>>>>>           info->u.mirror = (BlockJobInfoMirror) {
>>>>>>             .actively_synced = s->actively_synced,
>>>>>> +        .remaining_dirty = bdrv_get_dirty_count(s->dirty_bitmap),
>>>>>
>>>>> Doesn't it duplicate info->len - info->offset in meaning?
>>>>>
>>>>
>>>> Essentially yes, apart from the in-flight bytes:
>>>
>>> Is it worth reporting to user?
>>>
>>
>> You suggested that data_sent and remaining_dirty are important:
>> https://lists.nongnu.org/archive/html/qemu-devel/2023-02/msg03831.html
>>
> 
> Yes, sorry if it made you implement these fields :/ I was just thinking.

It was no big deal :)

> 
>> But I guess info->len - info->offset is just as good as part of a
>> heuristic to decide when the switch to active mode should happen.
>>
>> For us, it doesn't really matter right now, because our users didn't
>> report issues with convergence, so our plan is to just switch to active
>> mode after the job is ready. We just need actively_synced to infer when
>> the switch is complete.
>>
> 
> Hmm. But mirror can't become "actively_synced" until it not switched to
> active mode?

Yes, but that's fine. We'd wait for the job to be ready, then switch to
active mode and once actively_synced is true, we'd start migration. Then
we don't need to worry about triggering the assertion after inactivating
block devices upon switchover.

> 
>>>>>           job_progress_set_remaining(&s->common.job,
>>>>>                                      s->bytes_in_flight + cnt +
>>>>>                                      s->active_write_bytes_in_flight);
>>>>
>>>> Should I rather use that value (and rename it to e.g. data_remaining to
>>>> be more similar to data_sent from 9/9)?
>>>>
>>>> But I'd argue the same way as in 9/9: it's not transparent to users
>>>> what
>>>> offset and len mean for the mirror job, because their documentation is
>>>> for a generic block job. E.g. len is documented to be able to change in
>>>> both directions while the job runs.
>>>>
>>>
>>> Still I'm not sure that we need new status values. I.e. if you need some
>>> new ones, you should explain the case and why existing information is
>>> not enough.
>>>
>>> Especially when documentation of existing things is unclear, its better
>>> to start from improving it. And when we understand what len and offset
>>> means for mirror, it would probably be enough.
>>>
>>
>> Okay, makes sense! But I'm not sure how. Should I just add a paragraph
>> describing what the values mean for mirror in the description of @len
>> and @offset in @BlockJobInfo? Or where should this be documented?
>>
> 
> Hmm, or just in description of blockdev-mirror command. Still, I don't
> mean that you should do it.
> 
> If we want additional similar fields - then yes, we should describe why
> and what is different with existing fields, and good start for it - add
> details to documentation.
> If we don't add them - current heuristical understanding that "remaining
> ~= len - offset" is enough.
> So, I think better not add these fields now if you don't need them.
> 

Okay, sure. I'll let the people that actually need additional fields add
them :)

Best Regards,
Fiona