Re: [Qemu-devel] [PATCH 3/3] block/backup: refactor write_flags

[Max Reitz]

On 31.07.19 18:01, Vladimir Sementsov-Ogievskiy wrote:
> 30.07.2019 21:28, John Snow wrote:
>>
>>
>> On 7/30/19 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> write flags are constant, let's store it in BackupBlockJob instead of
>>> recalculating. It also makes two boolean fields to be unused, so,
>>> drop them.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>> ---
>>>   block/backup.c | 24 ++++++++++++------------
>>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index c5f941101a..4651649e9d 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -47,7 +47,6 @@ typedef struct BackupBlockJob {
>>>       uint64_t len;
>>>       uint64_t bytes_read;
>>>       int64_t cluster_size;
>>> -    bool compress;
>>>       NotifierWithReturn before_write;
>>>       QLIST_HEAD(, CowRequest) inflight_reqs;
>>>   
>>> @@ -55,7 +54,7 @@ typedef struct BackupBlockJob {
>>>       bool use_copy_range;
>>>       int64_t copy_range_size;
>>>   
>>> -    bool serialize_target_writes;
>>> +    BdrvRequestFlags write_flags;
>>>   } BackupBlockJob;
>>>   
>>>   static const BlockJobDriver backup_job_driver;
>>> @@ -110,10 +109,6 @@ static int coroutine_fn 
>>> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>>       BlockBackend *blk = job->common.blk;
>>>       int nbytes;
>>>       int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>>> -    int write_flags =
>>> -            (job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0) |
>>> -            (job->compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
>>> -
>>>   
>>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>>       hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
>>> @@ -132,7 +127,7 @@ static int coroutine_fn 
>>> backup_cow_with_bounce_buffer(BackupBlockJob *job,
>>>       }
>>>   
>>>       ret = blk_co_pwrite(job->target, start, nbytes, *bounce_buffer,
>>> -                        write_flags);
>>> +                        job->write_flags);
>>>       if (ret < 0) {
>>>           trace_backup_do_cow_write_fail(job, start, ret);
>>>           if (error_is_read) {
>>> @@ -160,7 +155,6 @@ static int coroutine_fn 
>>> backup_cow_with_offload(BackupBlockJob *job,
>>>       BlockBackend *blk = job->common.blk;
>>>       int nbytes;
>>>       int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
>>> -    int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING 
>>> : 0;
>>>   
>>>       assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
>>>       assert(QEMU_IS_ALIGNED(start, job->cluster_size));
>>> @@ -168,7 +162,7 @@ static int coroutine_fn 
>>> backup_cow_with_offload(BackupBlockJob *job,
>>>       nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
>>>       hbitmap_reset(job->copy_bitmap, start, job->cluster_size * 
>>> nr_clusters);
>>>       ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
>>> -                            read_flags, write_flags);
>>> +                            read_flags, job->write_flags);
>>>       if (ret < 0) {
>>>           trace_backup_do_cow_copy_range_fail(job, start, ret);
>>>           hbitmap_set(job->copy_bitmap, start, job->cluster_size * 
>>> nr_clusters);
>>> @@ -638,10 +632,16 @@ BlockJob *backup_job_create(const char *job_id, 
>>> BlockDriverState *bs,
>>>       job->sync_mode = sync_mode;
>>>       job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
>>>                          sync_bitmap : NULL;
>>> -    job->compress = compress;
>>>   
>>> -    /* Detect image-fleecing (and similar) schemes */
>>> -    job->serialize_target_writes = bdrv_chain_contains(target, bs);
>>> +    /*
>>> +     * Set write flags:
>>> +     *  1. Detect image-fleecing (and similar) schemes
>>> +     *  2. Handle compression
>>> +     */
>>> +    job->write_flags =
>>> +            (bdrv_chain_contains(target, bs) ? BDRV_REQ_SERIALISING : 0) |
>>> +            (compress ? BDRV_REQ_WRITE_COMPRESSED : 0);
>>> +
>>>       job->cluster_size = cluster_size;
>>>       job->copy_bitmap = copy_bitmap;
>>>       copy_bitmap = NULL;
>>>
>>
>> What happens if you did pass BDRV_REQ_WRITE_COMPRESSED to
>> blk_co_copy_range? Is that rejected somewhere in the stack?
> 
> 
> Hmm, I'm afraid that it will be silently ignored.
> 
> And I have one question related to copy offload too.
> 
> Do we really need to handle max_transfer in backup code for copy offload?
> Is max_transfer related to it really?
> 
> Anyway, bl.max_transfer should be handled in generic copy_range code in 
> block/io.c
> (if it should at all), I'm going to fix it, but may be, I can just drop this 
> limitation
> from backup?

On a quick glance, it doesn’t look like a limitation to me but actually
like the opposite.  backup_cow_with_bounce_buffer() only copies up to
cluster_size, whereas backup_cow_with_offload() will copy up to the
maximum transfer size permitted by both source and target for copy
offloading.

Max

signature.asc
Description: OpenPGP digital signature