Re: [Qemu-devel] [PATCH 13/31] file-posix: Switch to .bdrv_co_block_status()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

On 04/17/2017 08:33 PM, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based.  Update the file protocol driver accordingly.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/file-posix.c | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)

This patch makes qemu-iotests 109 fail, due to an assertion in 6/31 that
the nested call to bdrv_block_status() [thanks to BDRV_BLOCK_RAW] is
still sector-aligned.  The culprit:

> +static int64_t coroutine_fn raw_co_block_status(BlockDriverState *bs,
> +                                                int64_t offset,
> +                                                int64_t bytes, int64_t *pnum,
> +                                                BlockDriverState **file)
>  {
> -    off_t start, data = 0, hole = 0;
> +    off_t data = 0, hole = 0;
>      int64_t total_size;
>      int ret;
> 
> @@ -1856,39 +1856,38 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>          return ret;
>      }
> 
> -    start = sector_num * BDRV_SECTOR_SIZE;
>      total_size = bdrv_getlength(bs);

total_size is always sector-aligned (bdrv_getlength() purposefully
widens the input file size, 560 bytes in the case of an empty qcow image
in test 109, out to a sector boundary)...

>      if (total_size < 0) {
>          return total_size;
> -    } else if (start >= total_size) {
> +    } else if (offset >= total_size) {
>          *pnum = 0;
>          return 0;
> -    } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
> -        nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
> +    } else if (offset + bytes > total_size) {
> +        bytes = total_size - offset;
>      }
> 
> -    ret = find_allocation(bs, start, &data, &hole);
> +    ret = find_allocation(bs, offset, &data, &hole);

...while find_allocation still obeys byte-granularity limits of the
underlying file system, and therefore reports a hole starting at offset
560 (the only time a hole starts at a non-sector boundary); pre-patch
that didn't matter (because we rounded before returning sectors through
*pnum), but post-patch, returning 560 instead of 1024 triggers problems.

I'm still debating whether the best fix is to tweak the file-posix
limits to just round up to sector boundaries at EOF, or to make the
block layer itself special-case EOF in case other protocols also have
byte-granularity support.  Any advice you have while reviewing is
appreciated, but it does mean I'll need a v2 of the series one way or
another.

And I didn't spot it during my earlier testing because I was skipping
109 as failing prior to my patches even started testing it - but now
that Jeff and Fam have both posted patches for two separate problems in
test 109 (the need to munge "len", and failure to do proper cleanup), I
no longer have that excuse.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]