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

[Vladimir Sementsov-Ogievskiy]

01.08.2019 14:37, Max Reitz wrote:
> On 01.08.19 13:32, Vladimir Sementsov-Ogievskiy wrote:
>> 01.08.2019 14:28, Max Reitz wrote:
>>> 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.
>>>
>>
>> I mean, why not to just copy the whole chunk comes in notifier and don't 
>> care about
>> max_transfer? Backup loop copies cluster by cluster anyway, so only notifier 
>> may copy
>> larger chunk.
> 
> One thing that comes to mind is the hbitmap_get() check in
> backup_do_cow().  You don’t want to copy everything just because the
> first cluster needs to be copied.
> 

Hmm, but seems that we do exactly this, and this is wrong. But this is separate 
thing..


About copy_range, I just don't sure that max_transfer is a true restriction for 
copy_range.
For example, for file_posix max_transfer comes from some specific ioctl or from 
sysfs.. Is
it appropriate as limitation for copy_file_range?

Also, Max, could you please take a look at "[PATCH v3] blockjob: drain all job 
nodes in block_job_drain"
thread? Maybe, what John questions is obvious for you.

-- 
Best regards,
Vladimir