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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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?

Yes.  Fixing...

>> +    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?

I got confused.

Here's how block.h explains the allocation flags:

 * DATA ZERO OFFSET_VALID
 *  t    t        t       sectors read as zero, bs->file is zero at offset
 *  t    f        t       sectors read as valid from bs->file at offset
 *  f    t        t       sectors preallocated, read as zero, bs->file not
 *                        necessarily zero at offset
 *  f    f        t       sectors preallocated but read from backing_hd,
 *                        bs->file contains garbage at offset

Should a hole in a bdrv_file bs have status DATA | ZERO (first row) or
just ZERO (third row)?

First row:

* "sectors read as zero": certainly true in a hole.

* "bs->file is zero at offset": not sure what that's supposed to mean.
  bs->file is null.

Third row:

* "sectors preallocated": not sure what that's supposed to mean.
  Probably preallocation != off.  If that's what it means, then it's
  false in a hole.

* "read as zero": certainly true in a hole.

* "bs->file not necessarily zero at offset": not sure what that's
  supposed to mean.  bs->file is null.

Now you're probably confused, too.

Anyway, I shouldn't make such a change in a cleanup patch!  v2 will
stick to the old flags.

Thanks!