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

[Fiona Ebner]

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

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.

>>>          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?