Re: [PATCH v7 07/33] block/block-copy: introduce block_copy_set_copy_opts()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

10.08.2021 17:55, Hanna Reitz wrote:
> On 04.08.21 11:37, Vladimir Sementsov-Ogievskiy wrote:
>> We'll need a possibility to set compress and use_copy_range options
>> after initialization of the state. So make corresponding part of
>> block_copy_state_new() separate and public.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block-copy.h |  2 ++
>>   block/block-copy.c         | 66 +++++++++++++++++++++-----------------
>>   2 files changed, 39 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/block/block-copy.h b/include/block/block-copy.h
>> index 734389d32a..f0ba7bc828 100644
>> --- a/include/block/block-copy.h
>> +++ b/include/block/block-copy.h
>> @@ -28,6 +28,8 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>                                        int64_t cluster_size, bool use_copy_range,
>>                                        bool compress, Error **errp);
>> +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
>> +                              bool compress);
>>   void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm);
>>   void block_copy_state_free(BlockCopyState *s);
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 58b4345a5a..84c29fb233 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -315,21 +315,11 @@ static uint32_t block_copy_max_transfer(BdrvChild *source, BdrvChild *target)
>>                                        target->bs->bl.max_transfer));
>>   }
>> -BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>> -                                     int64_t cluster_size, bool use_copy_range,
>> -                                     bool compress, Error **errp)
>> +/* Function should be called prior any actual copy request */
> 
> Given this is something the caller should know, I’d prefer putting this into the block-copy.h header.
> 
> However, I realize we have a wild mix of this in qemu and often do put such information into the C source file, so...
> 
>> +void block_copy_set_copy_opts(BlockCopyState *s, bool use_copy_range,
>> +                              bool compress)
>>   {
>> -    BlockCopyState *s;
>> -    BdrvDirtyBitmap *copy_bitmap;
>>       bool is_fleecing;
>> -
>> -    copy_bitmap = bdrv_create_dirty_bitmap(source->bs, cluster_size, NULL,
>> -                                           errp);
>> -    if (!copy_bitmap) {
>> -        return NULL;
>> -    }
>> -    bdrv_disable_dirty_bitmap(copy_bitmap);
>> -
>>       /*
>>        * If source is in backing chain of target assume that target is going to be
>>        * used for "image fleecing", i.e. it should represent a kind of snapshot of
>> @@ -344,24 +334,12 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
>>        * For more information see commit f8d59dfb40bb and test
>>        * tests/qemu-iotests/222
>>        */
>> -    is_fleecing = bdrv_chain_contains(target->bs, source->bs);
>> +    is_fleecing = bdrv_chain_contains(s->target->bs, s->source->bs);
>> -    s = g_new(BlockCopyState, 1);
>> -    *s = (BlockCopyState) {
>> -        .source = source,
>> -        .target = target,
>> -        .copy_bitmap = copy_bitmap,
>> -        .cluster_size = cluster_size,
>> -        .len = bdrv_dirty_bitmap_size(copy_bitmap),
>> -        .write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) |
>> -            (compress ? BDRV_REQ_WRITE_COMPRESSED : 0),
>> -        .mem = shres_create(BLOCK_COPY_MAX_MEM),
>> -        .max_transfer = QEMU_ALIGN_DOWN(
>> -                                    block_copy_max_transfer(source, target),
>> -                                    cluster_size),
>> -    };
>> +    s->write_flags = (is_fleecing ? BDRV_REQ_SERIALISING : 0) |
>> +        (compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
> 
> Shouldn’t we keep the is_fleecing check in block_copy_state_new()? We must perform it at least once, but there is no benefit in repeating it on every block_copy_set_copy_opts() call, right?
> 

I think, it may help if user calls block_copy_set_copy_opts() after graph change


-- 
Best regards,
Vladimir