Eric Blake <ebl...@redhat.com> 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 <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?
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!
