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 <arm...@redhat.com>
> ---
> 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
