[Qemu-devel] [PATCH] mirror: Only mirror granularity-aligned chunks

Max Reitz posted 1 patch 4 years, 7 months ago
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test s390x passed
Test checkpatch passed
Test asan passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190805114923.23701-1-mreitz@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>, John Snow <jsnow@redhat.com>
There is a newer version of this series
block/mirror.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
[Qemu-devel] [PATCH] mirror: Only mirror granularity-aligned chunks
Posted by Max Reitz 4 years, 7 months ago
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);
 
-- 
2.21.0


Re: [Qemu-devel] [PATCH] mirror: Only mirror granularity-aligned chunks
Posted by Max Reitz 4 years, 7 months ago
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);
>  
> 


Re: [Qemu-devel] [PATCH] mirror: Only mirror granularity-aligned chunks
Posted by Vladimir Sementsov-Ogievskiy 4 years, 7 months ago
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>

-- 
Best regards,
Vladimir
Re: [Qemu-devel] [PATCH] mirror: Only mirror granularity-aligned chunks
Posted by Max Reitz 4 years, 7 months ago
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>
>