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? > > 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 <js...@redhat.com> > -- Best regards, Vladimir