| Message ID | 20190805114923.23701-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mirror: Only mirror granularity-aligned chunks | expand |
On 05.08.19 13:49, Max Reitz wrote: > In write-blocking mode, all writes to the top node directly go to the > target. We must only mirror chunks of data that are aligned to the > job's granularity, because that is how the dirty bitmap works. > Therefore, the request alignment for writes must be the job's > granularity (in write-blocking mode). > > Unfortunately, this forces all reads and writes to have the same > granularity (we only need this alignment for writes to the target, not > the source), but that is something to be fixed another time. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > This is an alternative to Vladimir's "util/hbitmap: fix unaligned reset" > patch. I don't mind much either way, both of pros and cons. Comparing I don’t know why I can’t write (especially lately). s/of/have/, of course. Max > this patch to Vladimir's: > > + Makes copy-mode=write-blocking really work (unless I'm mistaken) > - Lowers performance with copy-mode=write-blocking unnecessarily > --- > block/mirror.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 8cb75fb409..3f9c5a178a 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1481,6 +1481,15 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, > *nshared = BLK_PERM_ALL; > } > > +static void bdrv_mirror_top_refresh_limits(BlockDriverState *bs, Error **errp) > +{ > + MirrorBDSOpaque *s = bs->opaque; > + > + if (s && s->job && s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) { > + bs->bl.request_alignment = s->job->granularity; > + } > +} > + > /* Dummy node that provides consistent read to its users without requiring it > * from its backing file and that allows writes on the backing file chain. */ > static BlockDriver bdrv_mirror_top = { > @@ -1493,6 +1502,7 @@ static BlockDriver bdrv_mirror_top = { > .bdrv_co_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, > .bdrv_child_perm = bdrv_mirror_top_child_perm, > + .bdrv_refresh_limits = bdrv_mirror_top_refresh_limits, > }; > > static BlockJob *mirror_start_job( > @@ -1678,6 +1688,8 @@ static BlockJob *mirror_start_job( > > QTAILQ_INIT(&s->ops_in_flight); > > + bdrv_refresh_limits(mirror_top_bs, &error_abort); > + > trace_mirror_start(bs, s, opaque); > job_start(&s->common.job); > >
05.08.2019 14:49, Max Reitz wrote: > In write-blocking mode, all writes to the top node directly go to the > target. We must only mirror chunks of data that are aligned to the > job's granularity, because that is how the dirty bitmap works. > Therefore, the request alignment for writes must be the job's > granularity (in write-blocking mode). > > Unfortunately, this forces all reads and writes to have the same > granularity (we only need this alignment for writes to the target, not > the source), but that is something to be fixed another time. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > This is an alternative to Vladimir's "util/hbitmap: fix unaligned reset" > patch. I don't mind much either way, both of pros and cons. Comparing > this patch to Vladimir's: > > + Makes copy-mode=write-blocking really work (unless I'm mistaken) > - Lowers performance with copy-mode=write-blocking unnecessarily > --- > block/mirror.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/block/mirror.c b/block/mirror.c > index 8cb75fb409..3f9c5a178a 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -1481,6 +1481,15 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, > *nshared = BLK_PERM_ALL; > } > > +static void bdrv_mirror_top_refresh_limits(BlockDriverState *bs, Error **errp) > +{ > + MirrorBDSOpaque *s = bs->opaque; > + > + if (s && s->job && s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) { > + bs->bl.request_alignment = s->job->granularity; > + } > +} > + > /* Dummy node that provides consistent read to its users without requiring it > * from its backing file and that allows writes on the backing file chain. */ > static BlockDriver bdrv_mirror_top = { > @@ -1493,6 +1502,7 @@ static BlockDriver bdrv_mirror_top = { > .bdrv_co_block_status = bdrv_co_block_status_from_backing, > .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, > .bdrv_child_perm = bdrv_mirror_top_child_perm, > + .bdrv_refresh_limits = bdrv_mirror_top_refresh_limits, > }; > > static BlockJob *mirror_start_job( > @@ -1678,6 +1688,8 @@ static BlockJob *mirror_start_job( > > QTAILQ_INIT(&s->ops_in_flight); > > + bdrv_refresh_limits(mirror_top_bs, &error_abort); > + > trace_mirror_start(bs, s, opaque); > job_start(&s->common.job); > > Am I right that the fact that no guest request will skip this limit is guaranteed by aio_context_acquire/release around blockdev_mirror_common? Not sure how much it lowers performance, but it should work.. Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 05.08.19 15:00, Vladimir Sementsov-Ogievskiy wrote: > 05.08.2019 14:49, Max Reitz wrote: >> In write-blocking mode, all writes to the top node directly go to the >> target. We must only mirror chunks of data that are aligned to the >> job's granularity, because that is how the dirty bitmap works. >> Therefore, the request alignment for writes must be the job's >> granularity (in write-blocking mode). >> >> Unfortunately, this forces all reads and writes to have the same >> granularity (we only need this alignment for writes to the target, not >> the source), but that is something to be fixed another time. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> This is an alternative to Vladimir's "util/hbitmap: fix unaligned reset" >> patch. I don't mind much either way, both of pros and cons. Comparing >> this patch to Vladimir's: >> >> + Makes copy-mode=write-blocking really work (unless I'm mistaken) >> - Lowers performance with copy-mode=write-blocking unnecessarily >> --- >> block/mirror.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index 8cb75fb409..3f9c5a178a 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -1481,6 +1481,15 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, >> *nshared = BLK_PERM_ALL; >> } >> >> +static void bdrv_mirror_top_refresh_limits(BlockDriverState *bs, Error **errp) >> +{ >> + MirrorBDSOpaque *s = bs->opaque; >> + >> + if (s && s->job && s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) { >> + bs->bl.request_alignment = s->job->granularity; >> + } >> +} >> + >> /* Dummy node that provides consistent read to its users without requiring it >> * from its backing file and that allows writes on the backing file chain. */ >> static BlockDriver bdrv_mirror_top = { >> @@ -1493,6 +1502,7 @@ static BlockDriver bdrv_mirror_top = { >> .bdrv_co_block_status = bdrv_co_block_status_from_backing, >> .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, >> .bdrv_child_perm = bdrv_mirror_top_child_perm, >> + .bdrv_refresh_limits = bdrv_mirror_top_refresh_limits, >> }; >> >> static BlockJob *mirror_start_job( >> @@ -1678,6 +1688,8 @@ static BlockJob *mirror_start_job( >> >> QTAILQ_INIT(&s->ops_in_flight); >> >> + bdrv_refresh_limits(mirror_top_bs, &error_abort); >> + >> trace_mirror_start(bs, s, opaque); >> job_start(&s->common.job); >> >> > > Am I right that the fact that no guest request will skip this limit is guaranteed by > aio_context_acquire/release around blockdev_mirror_common? Hm. As long as we don’t drain or release the context. Unfortunately, block_job_add_bdrv() does release the context (if the job runs in a foreign context). The reason we need this call is because the automatic calls invoke the function when either bs->opaque or bs->opaque->job are still NULL, or when bs->opaque->job->copy_mode is not yet set. So I could just call it when copy_mode has been set (which is still before the dirty bitmap is created; and if we have I/O before that point, we have bigger problems than this.) Max > Not sure how much it lowers performance, but it should work.. > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
diff --git a/block/mirror.c b/block/mirror.c index 8cb75fb409..3f9c5a178a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1481,6 +1481,15 @@ static void bdrv_mirror_top_child_perm(BlockDriverState *bs, BdrvChild *c, *nshared = BLK_PERM_ALL; } +static void bdrv_mirror_top_refresh_limits(BlockDriverState *bs, Error **errp) +{ + MirrorBDSOpaque *s = bs->opaque; + + if (s && s->job && s->job->copy_mode == MIRROR_COPY_MODE_WRITE_BLOCKING) { + bs->bl.request_alignment = s->job->granularity; + } +} + /* Dummy node that provides consistent read to its users without requiring it * from its backing file and that allows writes on the backing file chain. */ static BlockDriver bdrv_mirror_top = { @@ -1493,6 +1502,7 @@ static BlockDriver bdrv_mirror_top = { .bdrv_co_block_status = bdrv_co_block_status_from_backing, .bdrv_refresh_filename = bdrv_mirror_top_refresh_filename, .bdrv_child_perm = bdrv_mirror_top_child_perm, + .bdrv_refresh_limits = bdrv_mirror_top_refresh_limits, }; static BlockJob *mirror_start_job( @@ -1678,6 +1688,8 @@ static BlockJob *mirror_start_job( QTAILQ_INIT(&s->ops_in_flight); + bdrv_refresh_limits(mirror_top_bs, &error_abort); + trace_mirror_start(bs, s, opaque); job_start(&s->common.job);
In write-blocking mode, all writes to the top node directly go to the target. We must only mirror chunks of data that are aligned to the job's granularity, because that is how the dirty bitmap works. Therefore, the request alignment for writes must be the job's granularity (in write-blocking mode). Unfortunately, this forces all reads and writes to have the same granularity (we only need this alignment for writes to the target, not the source), but that is something to be fixed another time. Signed-off-by: Max Reitz <mreitz@redhat.com> --- This is an alternative to Vladimir's "util/hbitmap: fix unaligned reset" patch. I don't mind much either way, both of pros and cons. Comparing this patch to Vladimir's: + Makes copy-mode=write-blocking really work (unless I'm mistaken) - Lowers performance with copy-mode=write-blocking unnecessarily --- block/mirror.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)