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 >>>>>> <vsement...@yandex-team.ru> >>>>>> Signed-off-by: Fiona Ebner <f.eb...@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