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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-13 at 13:00, Kevin Wolf wrote:
> Am 13.11.2014 um 12:45 hat Max Reitz geschrieben:
>> On 2014-11-13 at 12:40, Kevin Wolf wrote:
>>> Am 13.11.2014 um 00:25 hat Eric Blake geschrieben:
>>>> On 11/12/2014 01:27 PM, Markus Armbruster wrote:
>>>>> +    /* 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?
>>> As soon as you say "except", it's wrong to assert this at all. We can't
>>> guarantee that the condition is true and it's not a programming error
>>> in qemu if it's false. Sounds to me as if it should be a normal error
>>> check rather than an assertion.
>>>
>>> Also, what happens after EOF? I haven't read the patch yet, maybe it
>>> handles the situation already earlier, but if it doesn't, won't we get
>>> offset == start then?
>> raw_co_get_block_status() already bails out if start is at or beyond EOF.
> Okay, so that's basically the same "except" as above.
>
> Except that the window for the race is much larger because the
> raw_co_get_block_status() check uses the cached value, so any file size
> change in the background after qemu has opened the image would trigger
> an assertion failure.

Well, iotest 102 tests exactly that.

Max