[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv_get_block_status() |
Date: |
Thu, 5 Oct 2017 10:35:09 -0400 |
User-agent: |
Mutt/1.9.0 (2017-09-02) |
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 <address@hidden>
>
> ---
> 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 <address@hidden>
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;
}
- [Qemu-devel] [PATCH v2 0/5] block: Avoid copy-on-read assertions, Eric Blake, 2017/10/03
- [Qemu-devel] [PATCH v2 1/5] qemu-io: Add -C for opening with copy-on-read, Eric Blake, 2017/10/03
- [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv_get_block_status(), Eric Blake, 2017/10/03
- Re: [Qemu-devel] [PATCH v2 2/5] block: Uniform handling of 0-length bdrv_get_block_status(),
Stefan Hajnoczi <=
- [Qemu-devel] [PATCH v2 3/5] block: Add blkdebug hook for copy-on-read, Eric Blake, 2017/10/03
- [Qemu-devel] [PATCH v2 4/5] block: Perform copy-on-read in loop, Eric Blake, 2017/10/03
- [Qemu-devel] [PATCH v2 5/5] iotests: Add test 197 for covering copy-on-read, Eric Blake, 2017/10/03
- Re: [Qemu-devel] [PATCH v2 0/5] block: Avoid copy-on-read assertions, no-reply, 2017/10/04