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

[Max Reitz]

On 01.08.19 14:02, Vladimir Sementsov-Ogievskiy wrote:
> 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..

You’re right.  It’s totally broken.  Nice.

The following gets me a nice content mismatch:

$ ./qemu-img create -f qcow2 src.qcow2 2M
$ ./qemu-io -c 'write -P 42 0 2M' src.qcow2
$ cp src.qcow2 ref.qcow2

{"execute":"qmp_capabilities"}
{"return": {}}
{"execute":"blockdev-add","arguments":
 {"node-name":"src","driver":"qcow2",
  "file":{"driver":"file","filename":"src.qcow2"}}}
{"return": {}}
{"execute":"drive-backup","arguments":
 {"device":"src","job-id":"backup","target":"tgt.qcow2",
  "format":"qcow2","sync":"full","speed":1048576}}
{"timestamp": {"seconds": 1564661742, "microseconds": 268384},
 "event": "JOB_STATUS_CHANGE",
  "data": {"status": "created", "id": "backup"}}
{"timestamp": {"seconds": 1564661742, "microseconds": 268436},
 "event": "JOB_STATUS_CHANGE",
  "data": {"status": "running", "id": "backup"}}
{"return": {}}
{"execute":"human-monitor-command",
 "arguments":{"command-line":
              "qemu-io src \"write -P 23 1114112 65536\""}}
{"return": ""}
{"execute":"human-monitor-command",
 "arguments":{"command-line":
              "qemu-io src \"write -P 66 1048576 1M\""}}
{"return": ""}
{"timestamp": {"seconds": 1564661744, "microseconds": 278362}.
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "waiting", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278534},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "pending", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278778},
 "event": "BLOCK_JOB_COMPLETED",
 "data": {"device": "backup", "len": 2097152, "offset": 2162688,
          "speed": 1048576, "type": "backup"}}
{"execute":"quit"}
{"timestamp": {"seconds": 1564661744, "microseconds": 278884},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "concluded", "id": "backup"}}
{"timestamp": {"seconds": 1564661744, "microseconds": 278946},
 "event": "JOB_STATUS_CHANGE",
 "data": {"status": "null", "id": "backup"}}
{"return": {}}

$ ./qemu-img compare src.qcow2
Content mismatch at offset 1114112!


Aww maaan.  Setting copy_range to false “fixes” it.  I guess this’ll
need to be fixed for 4.1. :-/

> 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?

Who knows, but it’s probably the best approximation we have.

> 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.

Perhaps after fixing backup. :-/

Max

signature.asc
Description: OpenPGP digital signature