Re: [PATCH v7] generic: initial fiemap range query test

From: Nikolay Borisov <nborisov@suse.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>,
	Dave Chinner <david@fromorbit.com>
Cc: Eric Sandeen <sandeen@sandeen.net>,
	eguan@redhat.com, linux-xfs@vger.kernel.org,
	fstests@vger.kernel.org
Subject: Re: [PATCH v7] generic: initial fiemap range query test
Date: Thu, 7 Dec 2017 08:55:36 +0200	[thread overview]
Message-ID: <ea697081-b253-e0c2-633c-9a58d6bf1ee1@suse.com> (raw)
In-Reply-To: <20171206224627.GO19219@magnolia>

On  7.12.2017 00:46, Darrick J. Wong wrote:
> On Thu, Dec 07, 2017 at 09:16:03AM +1100, Dave Chinner wrote:
>> On Wed, Dec 06, 2017 at 04:01:58PM -0600, Eric Sandeen wrote:
>>> On 12/6/17 3:57 PM, Dave Chinner wrote:
>>>
>>>> There's a *simple answer* to this problem: fix the new command's
>>>> output.
>>>>
>>>> That is: the user asked for a specific range, so the command itself
>>>> should trim the map returned by the kernel to only display the exact
>>>> range the user asked for.  Then it doesn't matter if the underlying
>>>> filesystem trims the extents or not, because the we're going to do
>>>> that anyway in userspace.
>>>
>>> I have a different opinion:
>>>
>>> xfs_io is a debugging tool; the fiemap command sends an ioctl to the kernel.
>>>
>>> Ranged fiemap queries are a real thing; you put numbers into the kernel,
>>> and you get numbers out of the kernel.
>>>
>>> IMNSO, xfs_io should present to the user /what the kernel returned/,
>>> and not re-interpret it to fit some other notion of correctness if we
>>> don't like what the kernel told us.
>>
>> I hardly think "trimming to the range the user asked for" is
>> "re-interpreting what the kernel told us". It's limiting output
>> range to exactly what the user asked for - the output is still
>> correct regardless of how it's filtered to match what the user asked
>> for....
> 
> ...except that xfs_io is a tool we use to debug the garbage that FIEMAP
> sprays back at userspace, so it absolutely should not be hiding things
> like this from the test suite:
> 
> $ xfs_io -c "fiemap -va 0k 65k" moofile
> moo:
>  EXT: FILE-OFFSET               BLOCK-RANGE            TOTAL FLAGS
>    0: [0..18446744073709551615]: 1585456720..1585456719     0 0x301
>    1: [0..129]:                 hole                     130

Isn't there something similar in xfs_bmapi_read:

  /* Reading past eof, act as though there's a hole up to end. */
                if (eof)

                        got.br_startoff = end;

> 
> (Yay ext4.  WTF does the extent have length 0?)
> 
> So.... historically XFS trimmed the FIEMAP results.  Fix the test to
> reflect the XFS behavior, get ext4 (and btrfs) to fix their weird
> implementations, and leave xfs_io alone.
> 
> (Don't mind me shifting positions. :P)
> 
> --D
> 
>>> If you want to have some user-friendlier behavior where xfs_io layers
>>> behaviors on top of what the kernel provides, then add a "-t" argument for trim,
>>> but hiding ioctl inconsistencies by filtering them through xfs_io sounds
>>> like the wrong approach to me.
>>
>> Just filter the last output in the test, then, so it looks like
>>
>> 2: [128..XXX] data
>>
>> There is absolutely no excuse for creating multiple tests to support
>> a small difference in trailing extent range output from different
>> filesystem.
>>
>> Cheers,
>>
>> Dave.
>> -- 
>> Dave Chinner
>> david@fromorbit.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 