Message ID | 20171004014347.25099-3-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Series | block: Avoid copy-on-read assertions | expand |
On Tue, Oct 03, 2017 at 08:43:44PM -0500, Eric Blake wrote: > Handle a 0-length block status request up front, with a uniform > return value claiming the area is not allocated. > > Most callers don't pass a length of 0 to bdrv_get_block_status() > and friends; but it definitely happens with a 0-length read when > copy-on-read is enabled. While we could audit all callers to > ensure that they never make a 0-length request, and then assert > that fact, it was just as easy to fix things to always report > success (as long as the callers are careful to not go into an > infinite loop). However, we had inconsistent behavior on whether > the status is reported as allocated or defers to the backing > layer, depending on what callbacks the driver implements, and > possibly wasting quite a few CPU cycles to get to that answer. > Consistently reporting unallocated up front doesn't really hurt > anything, and makes it easier both for callers (0-length requests > now have well-defined behavior) and for drivers (drivers don't > have to deal with 0-length requests). > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: new patch > --- > block/io.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/io.c b/block/io.c > index e0f904583f..1f5baac41d 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1773,9 +1773,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > return total_sectors; > } > > - if (sector_num >= total_sectors) { > + if (sector_num >= total_sectors || !nb_sectors) { > *pnum = 0; > - return BDRV_BLOCK_EOF; > + return sector_num >= total_sectors ? BDRV_BLOCK_EOF : 0; > } Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> If you respin, please consider using a separate if statement to make the code clearer: if (!nb_sectors) { *pnum = 0; return 0; } if (sector_num >= total_sectors) { *pnum = 0; return BDRV_BLOCK_EOF; }
On 10/05/2017 09:35 AM, Stefan Hajnoczi wrote: > On Tue, Oct 03, 2017 at 08:43:44PM -0500, Eric Blake wrote: >> Handle a 0-length block status request up front, with a uniform >> return value claiming the area is not allocated. >> >> Most callers don't pass a length of 0 to bdrv_get_block_status() >> and friends; but it definitely happens with a 0-length read when >> copy-on-read is enabled. While we could audit all callers to >> ensure that they never make a 0-length request, and then assert >> that fact, it was just as easy to fix things to always report >> success (as long as the callers are careful to not go into an >> infinite loop). However, we had inconsistent behavior on whether >> the status is reported as allocated or defers to the backing >> layer, depending on what callbacks the driver implements, and >> possibly wasting quite a few CPU cycles to get to that answer. >> Consistently reporting unallocated up front doesn't really hurt >> anything, and makes it easier both for callers (0-length requests >> now have well-defined behavior) and for drivers (drivers don't >> have to deal with 0-length requests). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > If you respin, please consider using a separate if statement to make the > code clearer: > > if (!nb_sectors) { > *pnum = 0; > return 0; > } > if (sector_num >= total_sectors) { > *pnum = 0; > return BDRV_BLOCK_EOF; > } In fact, I think I would prefer to prioritize BDRV_BLOCK_EOF as the first condition, rather than the second, by swapping those two statements (that is, reading beyond EOF should set the EOF bit, regardless of whether nb_sectors was non-zero).
diff --git a/block/io.c b/block/io.c index e0f904583f..1f5baac41d 100644 --- a/block/io.c +++ b/block/io.c @@ -1773,9 +1773,9 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, return total_sectors; } - if (sector_num >= total_sectors) { + if (sector_num >= total_sectors || !nb_sectors) { *pnum = 0; - return BDRV_BLOCK_EOF; + return sector_num >= total_sectors ? BDRV_BLOCK_EOF : 0; } n = total_sectors - sector_num;
Handle a 0-length block status request up front, with a uniform return value claiming the area is not allocated. Most callers don't pass a length of 0 to bdrv_get_block_status() and friends; but it definitely happens with a 0-length read when copy-on-read is enabled. While we could audit all callers to ensure that they never make a 0-length request, and then assert that fact, it was just as easy to fix things to always report success (as long as the callers are careful to not go into an infinite loop). However, we had inconsistent behavior on whether the status is reported as allocated or defers to the backing layer, depending on what callbacks the driver implements, and possibly wasting quite a few CPU cycles to get to that answer. Consistently reporting unallocated up front doesn't really hurt anything, and makes it easier both for callers (0-length requests now have well-defined behavior) and for drivers (drivers don't have to deal with 0-length requests). Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: new patch --- block/io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)