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

[Vladimir Sementsov-Ogievskiy]

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?


> 
> I had just assumed it wouldn't work quite right because of the nature of
> copy offloading, but I don't actually know what it does do.
> 
> This seems like a pretty minor cleanup, but why not. I like dropping
> extra fields when I can:
> 
> Reviewed-by: John Snow <address@hidden>
> 


-- 
Best regards,
Vladimir