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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5836 bytes --]

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 <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]