20.09.2019 4:13, John Snow wrote: > > > On 9/19/19 2:50 AM, Vladimir Sementsov-Ogievskiy wrote: >> 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 <[email protected]> >>>> --- >>>> 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). >> > > oh, I'm ... sorry, I just didn't quite understand the comment. > > You're just making sure copy_range after all of our checks is non-zero, > plain and simple. If max_transfer was *smaller than a job cluster*, we > might end up with a copy_range size that's zero, which is obviously... > not useful. > > So, I might phrase "Also, we don't want to..." as: > > "copy_range does not respect max_transfer, so we factor that in here. If > it's smaller than the job->cluster_size, we are unable to use copy_range."
We actually able to: just using a loop and calling copy_range several times. May be just: copy_range does not respect max_transfer, so we factor that in here. If it's smaller than the job->cluster_size, we do not use copy_range. > > Just a suggestion, though, so: > > Reviewed-by: John Snow <[email protected]> > > > (SHOULD copy_range respect max_transfer? I guess it would be quite > different -- it would only count things it had to fall back and actually > *transfer*, right? I suppose that because it can have that fallback we > need to accommodate it here in backup.c, hence this workaround clamp.) > -- Best regards, Vladimir
