| Message ID | 20190917160731.10895-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | backup: copy_range fixes | expand |
On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote: > We shouldn't try to copy bytes beyond EOF. Fix it. > > Fixes: 9ded4a0114968e > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > block/backup.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/backup.c b/block/backup.c > index d8fdbfadfe..89f7f89200 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, > > assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size)); > assert(QEMU_IS_ALIGNED(start, job->cluster_size)); > - nbytes = MIN(job->copy_range_size, end - start); > + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start); I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves. If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned? We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well? Then ... > nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size); Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely? > bdrv_reset_dirty_bitmap(job->copy_bitmap, start, > job->cluster_size * nr_clusters); >
18.09.2019 23:14, John Snow wrote: > > > On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote: >> We shouldn't try to copy bytes beyond EOF. Fix it. >> >> Fixes: 9ded4a0114968e >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> --- >> block/backup.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index d8fdbfadfe..89f7f89200 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, >> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size)); >> assert(QEMU_IS_ALIGNED(start, job->cluster_size)); >> - nbytes = MIN(job->copy_range_size, end - start); >> + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start); > > I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves. last cluster may exceed EOF. And backup_do_cow (who calls backup_cow_with_offload) rounds all to clusters. It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing. > > If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned? Actually yes, as we are resetting dirty bitmap. > > We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well? Yes assertion may be added. > > Then ... > >> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size); > > Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely? nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers. nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF. Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes. Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix. > >> bdrv_reset_dirty_bitmap(job->copy_bitmap, start, >> job->cluster_size * nr_clusters); >> >
On 9/19/19 3:02 AM, Vladimir Sementsov-Ogievskiy wrote: > 18.09.2019 23:14, John Snow wrote: >> >> >> On 9/17/19 12:07 PM, Vladimir Sementsov-Ogievskiy wrote: >>> We shouldn't try to copy bytes beyond EOF. Fix it. >>> >>> Fixes: 9ded4a0114968e >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> --- >>> block/backup.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/block/backup.c b/block/backup.c >>> index d8fdbfadfe..89f7f89200 100644 >>> --- a/block/backup.c >>> +++ b/block/backup.c >>> @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, >>> assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size)); >>> assert(QEMU_IS_ALIGNED(start, job->cluster_size)); >>> - nbytes = MIN(job->copy_range_size, end - start); >>> + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start); >> >> I'm a little confused. I think the patch as written is correct, but I don't know what problem it solves. > > last cluster may exceed EOF. And backup_do_cow (who calls backup_cow_with_offload) rounds all to clusters. > It's not bad, but we need to crop nbytes before calling actual io functions. backup_cow_with_bounce_buffer does the same thing. > >> >> If we're going to allow the caller to pass in an end that's beyond EOF, does that mean we are *requiring* the caller to pass in a value that's aligned? > > Actually yes, as we are resetting dirty bitmap. > >> >> We should probably assert what kind of a value we're accepted here, right? We do for start, but should we for 'end' as well? > > Yes assertion may be added. > >> >> Then ... >> >>> nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size); >> >> Don't we just round this right back up immediately anyway? Does that mean we have callers that are passing in an 'end' that's more than 1 job-cluster beyond EOF? That seems like something that should be fixed in the caller, surely? > > nr_clusters are used to set/reset dirty bitmap. It's OK. Still, for last cluster we can drop it and use nbytes directly. No there should not be such callers. > nbytes is used to call blk_co_copy_range, and must be cropped to not exceed EOF. > Ah, right, right ... I *was* confused. We don't use nr_clusters for the IO itself, just the bitmap. So we effectively re-calculate aligned and unaligned values for use in different places. > Also, backup_cow_with_bounce_buffer behave in similar way: it crops nbytes. > > Of course, there is a place for good refactoring, but I think not in this patch, it's a small bug fix. > >> >>> bdrv_reset_dirty_bitmap(job->copy_bitmap, start, >>> job->cluster_size * nr_clusters); >>> >> > > We should make the interface here a little more clear I think, but what you wrote is correct. Reviewed-by: John Snow <jsnow@redhat.com>
diff --git a/block/backup.c b/block/backup.c index d8fdbfadfe..89f7f89200 100644 --- a/block/backup.c +++ b/block/backup.c @@ -161,7 +161,7 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job, assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size)); assert(QEMU_IS_ALIGNED(start, job->cluster_size)); - nbytes = MIN(job->copy_range_size, end - start); + nbytes = MIN(job->copy_range_size, MIN(end, job->len) - start); nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size); bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size * nr_clusters);