[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Fiona Ebner
Subject: Re: [PATCH 8/9] mirror: return the remaining dirty bytes upon query
Date: Thu, 2 Mar 2023 13:34:34 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0

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:

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?

reply via email to

[Prev in Thread] Current Thread [Next in Thread]