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