| Message ID | 20230224144825.466375-9-f.ebner@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: allow switching from background to active mode | expand |
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? > }; > } > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 07e0f30492..91594eace4 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1308,10 +1308,12 @@ > # @actively-synced: Whether the source is actively synced to the target, i.e. > # same data and new writes are done synchronously to both. > # > +# @remaining-dirty: How much of the source is dirty relative to the target. > +# > # Since 8.0 > ## > { 'struct': 'BlockJobInfoMirror', > - 'data': { 'actively-synced': 'bool' } } > + 'data': { 'actively-synced': 'bool', 'remaining-dirty': 'int64' } } > > ## > # @BlockJobInfo:
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: > 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. Best Regards, Fiona
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? >> 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.
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?
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.
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
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), }; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 07e0f30492..91594eace4 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1308,10 +1308,12 @@ # @actively-synced: Whether the source is actively synced to the target, i.e. # same data and new writes are done synchronously to both. # +# @remaining-dirty: How much of the source is dirty relative to the target. +# # Since 8.0 ## { 'struct': 'BlockJobInfoMirror', - 'data': { 'actively-synced': 'bool' } } + 'data': { 'actively-synced': 'bool', 'remaining-dirty': 'int64' } } ## # @BlockJobInfo:
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(-)