Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
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 --- 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 > > > (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
Re: [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
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 >>> --- >>> 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." Just a suggestion, though, so: Reviewed-by: John Snow (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.)
Re: [Qemu-block] [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
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 >> --- >> 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
Re: [Qemu-block] [Qemu-devel] [PATCH v12 1/2] block/backup: fix max_transfer handling for copy_range
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 --- 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?
