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

[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.


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

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

--
Best regards,
Vladimir