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

[Max Reitz <mreitz@redhat.com>]

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