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

[Max Reitz <mreitz@redhat.com>]

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