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. Max
signature.asc
Description: OpenPGP digital signature