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

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

[Max Reitz]

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2567 bytes --]

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);
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3453 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]