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

[Qemu-devel] [PATCH v2] 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.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
v2: Fix the placement of our bdrv_refresh_limits() call [Vladimir]
---
 block/mirror.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 8cb75fb409..7f0ff01beb 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(
@@ -1637,6 +1647,21 @@ static BlockJob *mirror_start_job(
         s->should_complete = true;
     }
 
+    /*
+     * Must be called before we start tracking writes, but after
+     *
+     *     ((MirrorBlockJob *)
+     *         ((MirrorBDSOpaque *)
+     *             mirror_top_bs->opaque
+     *         )->job
+     *     )->copy_mode
+     *
+     * has the correct value.
+     * (We start tracking writes as of the following
+     * bdrv_create_dirty_bitmap() call.)
+     */
+    bdrv_refresh_limits(mirror_top_bs, &error_abort);
+
     s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
         goto fail;
-- 
2.21.0

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

[Vladimir Sementsov-Ogievskiy]

05.08.2019 17:55, 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.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> v2: Fix the placement of our bdrv_refresh_limits() call [Vladimir]
> ---
>   block/mirror.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 8cb75fb409..7f0ff01beb 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(
> @@ -1637,6 +1647,21 @@ static BlockJob *mirror_start_job(
>           s->should_complete = true;
>       }
>   
> +    /*
> +     * Must be called before we start tracking writes, but after
> +     *
> +     *     ((MirrorBlockJob *)
> +     *         ((MirrorBDSOpaque *)
> +     *             mirror_top_bs->opaque
> +     *         )->job
> +     *     )->copy_mode
> +     *
> +     * has the correct value.
> +     * (We start tracking writes as of the following
> +     * bdrv_create_dirty_bitmap() call.)
> +     */
> +    bdrv_refresh_limits(mirror_top_bs, &error_abort);

Also, I'm not sure about error_abort() here. why not use local_err and return NULL
if failed?

> +
>       s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>       if (!s->dirty_bitmap) {
>           goto fail;
> 


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 05.08.19 17:09, Vladimir Sementsov-Ogievskiy wrote:
> 05.08.2019 17:55, 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.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> v2: Fix the placement of our bdrv_refresh_limits() call [Vladimir]
>> ---
>>   block/mirror.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 8cb75fb409..7f0ff01beb 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(
>> @@ -1637,6 +1647,21 @@ static BlockJob *mirror_start_job(
>>           s->should_complete = true;
>>       }
>>   
>> +    /*
>> +     * Must be called before we start tracking writes, but after
>> +     *
>> +     *     ((MirrorBlockJob *)
>> +     *         ((MirrorBDSOpaque *)
>> +     *             mirror_top_bs->opaque
>> +     *         )->job
>> +     *     )->copy_mode
>> +     *
>> +     * has the correct value.
>> +     * (We start tracking writes as of the following
>> +     * bdrv_create_dirty_bitmap() call.)
>> +     */
>> +    bdrv_refresh_limits(mirror_top_bs, &error_abort);
> 
> Also, I'm not sure about error_abort() here. why not use local_err and return NULL
> if failed?

Why would it fail?

Max

>> +
>>       s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>>       if (!s->dirty_bitmap) {
>>           goto fail;
>>
> 
> 



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

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

[Max Reitz]

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

On 05.08.19 17:14, Max Reitz wrote:
> On 05.08.19 17:09, Vladimir Sementsov-Ogievskiy wrote:
>> 05.08.2019 17:55, 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.
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>> v2: Fix the placement of our bdrv_refresh_limits() call [Vladimir]
>>> ---
>>>   block/mirror.c | 25 +++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 8cb75fb409..7f0ff01beb 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(
>>> @@ -1637,6 +1647,21 @@ static BlockJob *mirror_start_job(
>>>           s->should_complete = true;
>>>       }
>>>   
>>> +    /*
>>> +     * Must be called before we start tracking writes, but after
>>> +     *
>>> +     *     ((MirrorBlockJob *)
>>> +     *         ((MirrorBDSOpaque *)
>>> +     *             mirror_top_bs->opaque
>>> +     *         )->job
>>> +     *     )->copy_mode
>>> +     *
>>> +     * has the correct value.
>>> +     * (We start tracking writes as of the following
>>> +     * bdrv_create_dirty_bitmap() call.)
>>> +     */
>>> +    bdrv_refresh_limits(mirror_top_bs, &error_abort);
>>
>> Also, I'm not sure about error_abort() here. why not use local_err and return NULL
>> if failed?
> 
> Why would it fail?

Ah.  That’s why.  Because the children can fail.

That is so stupid.  Like, really.

Max


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