18.09.2019 22:57, John Snow wrote: > > > On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote: >> Of course, QEMU_ALIGN_UP is a typo, it should be QEMU_ALIGN_DOWN, as we >> are trying to find aligned size which satisfy both source and target. >> Also, don't ignore too small max_transfer. In this case seems safer to >> disable copy_range. >> >> Fixes: 9ded4a0114968e >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> >> --- >> block/backup.c | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index 763f0d7ff6..d8fdbfadfe 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -741,12 +741,16 @@ BlockJob *backup_job_create(const char *job_id, >> BlockDriverState *bs, >> job->cluster_size = cluster_size; >> job->copy_bitmap = copy_bitmap; >> copy_bitmap = NULL; >> - job->use_copy_range = !compress; /* compression isn't supported for it >> */ >> job->copy_range_size = >> MIN_NON_ZERO(blk_get_max_transfer(job->common.blk), >> blk_get_max_transfer(job->target)); >> - job->copy_range_size = MAX(job->cluster_size, >> - QEMU_ALIGN_UP(job->copy_range_size, >> - job->cluster_size)); >> + job->copy_range_size = QEMU_ALIGN_DOWN(job->copy_range_size, >> + job->cluster_size); >> + /* >> + * Compression is not supported for copy_range. Also, we don't want to >> + * handle small max_transfer for copy_range (which currently don't >> + * handle max_transfer at all). >> + */ >> + job->use_copy_range = !compress && job->copy_range_size > 0; >> /* Required permissions are already taken with target's blk_new() */ >> block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL, >> > > I'm clear on the alignment fix, I'm not clear on the comment about > max_transfer and how it relates to copy_range_size being non-zero. > > "small max transfer" -- what happens when it's zero? we're apparently OK with > a single cluster, but when it's zero, what happens?
if it zero it means that source or target requires max_transfer less than cluster_size. It seems not valid to call copy_range in this case. Still it's OK to use normal read/write, as they handle max_transfer internally in a loop (copy_range doesn't do it). -- Best regards, Vladimir
