Re: [Qemu-devel] [PATCH v2 3/4] raw-posix: Fix try_seek_hole()'s handling of SEEK_DATA failure

[Max Reitz <mreitz@redhat.com>]

On 2014-11-13 at 16:29, Eric Blake wrote:
> On 11/13/2014 07:52 AM, Eric Blake wrote:
>> On 11/13/2014 06:03 AM, Kevin Wolf wrote:
>>> Am 13.11.2014 um 11:17 hat Markus Armbruster geschrieben:
>>>> When SEEK_HOLE tells us we're in a hole, we try SEEK_DATA to find its
>>>> end.  When that fails, we pretend the hole extends to the end of file.
>>>> Wrong.
>>> Wrong only in some cases, see below.
>>>
>>>> Except when SEEK_END fails, we screw up and claim it extends
>>>> to offset -1.  More wrong.
>>
>>>> +++ b/block/raw-posix.c
>>>> @@ -1494,8 +1494,9 @@ static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data,
>>>>       } else {
>>>>           /* On a hole.  We need another syscall to find its end.  */
>>>>           *data = lseek(s->fd, start, SEEK_DATA);
>>>> -        if (*data == -1) {
>>>> -            *data = lseek(s->fd, 0, SEEK_END);
>>>> +        if (*data < 0) {
>>>> +            /* no idea where the hole ends, give up (unlikely to happen) */
>>> Not quite unlikely. If the file ends with a sparse area, we'll get
>>> -1/ENXIO here.
>>>
>>> lseek() with SEEK_DATA starting in a hole when there is no data until
>>> EOF is actually the part that isn't documented in the man page, but
>>> ENXIO is what I'm seeing here on RHEL 7.
>> Here's the (proposed) POSIX wording:
>>
>> http://austingroupbugs.net/view.php?id=415
>>
>> And ENXIO is indeed the expected error for SEEK_DATA on a trailing hole,
>> so maybe we should special case it.
>>
> Uggh.  Historical practice on Solaris (and therefore the POSIX wording)
> says that SEEK_HOLE in a trailing hole is allowed (but not required) to
> seek to EOF instead of reporting the offset requested.  I have no clue
> why this was done, but it is VERY annoying - it means that if you
> provide an offset within a tail hole of a file, you cannot reliably tell
> if the file ends in a hole or with data, without ALSO trying SEEK_DATA.
>   For applications that are reading a file sequentially but skipping over
> holes, this behavior is fine (it short-circuits the hole/data search
> points and might shave an iteration off a lop).  But for OUR purposes,
> where we are merely trying to ascertain whether we are in a hole, we
> have an inaccurate response - since SEEK_HOLE does NOT return the offset
> we passed in, we are prone to treat the offset as belonging to data,
> which is a pessimization (you never get wrong results by treating a hole
> as data and reading it, but it is definitely slower).
>
> I think you HAVE to call lseek() twice, both with SEEK_HOLE and with
> SEEK_DATA, if you want to accurately determine whether an offset happens
> to live within a trailing hole.
>
> (By the way, I really wish Solaris had implemented a variant that
> queried, but did NOT change the file offset - maybe Linux can add that
> as an extension, and give it sane semantics of not special casing
> trailing holes...)

Are you asking for fiemap? :-P

Max