Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP

[Eric Blake]

On 11/12/2014 01:27 PM, Markus Armbruster wrote:
> Commit 5500316 (May 2012) implemented raw_co_is_allocated() as
> follows:
> 

> Thus, the FIEMAP code executes rarely.  Makes it a nice hidey-hole for
> bugs.  Worse, bugs hiding there can theoretically bite even on a host
> that has SEEK_HOLE/SEEK_DATA.
> 
> I don't want to worry about this crap, not even theoretically.  Get
> rid of it, then clean up the mess, including spotty error checking.

Sounds reasonable to me.  It's rather a big patch (both nuking a bad
interface and rewriting the use of the good interface) that might have
been better as two commits, but I can live with it.

> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  block/raw-posix.c | 128 
> ++++++++++++++++++++----------------------------------
>  1 file changed, 47 insertions(+), 81 deletions(-)
> 

> +/*
> + * Find allocation range in @bs around offset @start.
> + * If @start is in a hole, store @start in @hole and the end of the
> + * hole in @data.
> + * If @start is in a data, store @start to @data, and the end of the
> + * data to @hole.
> + * If we can't find out, pretend there are no holes.
> + */
> +static void find_allocation(BlockDriverState *bs, off_t start,
> +                            off_t *data, off_t *hole)

Sounds like a good contract interface.

> +    /* in hole, end not yet known */
> +    offs = lseek(s->fd, start, SEEK_DATA);
> +    if (offs < 0) {
> +        /* no idea where the hole ends, give up (unlikely to happen) */
> +        goto dunno;
> +    }
> +    assert(offs >= start);
> +    *hole = start;
> +    *data = offs;

This assertion feels like an off-by-one.  The same offset cannot be both
a hole and data (except in some racy situation where some other process
is writing data to that offset in between our two lseek calls, but
that's already in no-man's land because no one else should be writing
the file while qemu has it open).  Is it worth using 'assert(offs >
start)' instead?

> +    ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
> +    find_allocation(bs, start, &data, &hole);
> +    if (data == start) {
>          /* On a data extent, compute sectors to the end of the extent.  */
>          *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE);
> -        return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start;
>      } else {
>          /* On a hole, compute sectors to the beginning of the next extent.  
> */
> +        assert(hole == start);
>          *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE);
> -        return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start;
> +        ret |= BDRV_BLOCK_ZERO;
>      }
> +    return ret;

The old code omits BDRV_BLOCK_DATA on a hole.  Why are you adding it
here, and why are you not mentioning it in the commit message?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature