| Message ID | 20190917160731.10895-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backup: copy_range fixes | expand |
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 <vsementsov@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?
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 <vsementsov@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).
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 <vsementsov@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). > 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 <jsnow@redhat.com> (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.)
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 <vsementsov@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). >> > > 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 <jsnow@redhat.com> > > > (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.) >
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,
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 <vsementsov@virtuozzo.com> --- block/backup.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)