diff mbox series

[PATCHv3,6/6] Documentation: Correct the description of FIEMAP_EXTENT_LAST

Message ID 279638c6939b1f6ef3ab32912cb51da1a967cf8e.1582702694.git.riteshh@linux.ibm.com
State Superseded
Headers show
Series ext4: bmap & fiemap conversion to use iomap | expand

Commit Message

Ritesh Harjani Feb. 26, 2020, 9:57 a.m. UTC
Currently FIEMAP_EXTENT_LAST is not working consistently across
different filesystem's fiemap implementations and thus this feature
may be broken. So fix the documentation about this flag to meet the
right expectations.

Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 Documentation/filesystems/fiemap.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Matthew Wilcox Feb. 26, 2020, 1:05 p.m. UTC | #1
On Wed, Feb 26, 2020 at 03:27:08PM +0530, Ritesh Harjani wrote:
> Currently FIEMAP_EXTENT_LAST is not working consistently across
> different filesystem's fiemap implementations and thus this feature
> may be broken. So fix the documentation about this flag to meet the
> right expectations.

Are you saying filesystems have both false positives and false negatives?
I can understand how a filesystem might fail to set FIEMAP_EXTENT_LAST,
but not how a filesystem might set it when there's actually another
extent beyond this one.

>  * FIEMAP_EXTENT_LAST
> -This is the last extent in the file. A mapping attempt past this
> -extent will return nothing.
> +This is generally the last extent in the file. A mapping attempt past this
> +extent may return nothing. But the user must still confirm by trying to map
> +past this extent, since different filesystems implement this differently.
Darrick J. Wong Feb. 26, 2020, 4:17 p.m. UTC | #2
On Wed, Feb 26, 2020 at 05:05:03AM -0800, Matthew Wilcox wrote:
> On Wed, Feb 26, 2020 at 03:27:08PM +0530, Ritesh Harjani wrote:
> > Currently FIEMAP_EXTENT_LAST is not working consistently across
> > different filesystem's fiemap implementations and thus this feature
> > may be broken. So fix the documentation about this flag to meet the
> > right expectations.
> 
> Are you saying filesystems have both false positives and false negatives?
> I can understand how a filesystem might fail to set FIEMAP_EXTENT_LAST,
> but not how a filesystem might set it when there's actually another
> extent beyond this one.
> 
> >  * FIEMAP_EXTENT_LAST
> > -This is the last extent in the file. A mapping attempt past this
> > -extent will return nothing.
> > +This is generally the last extent in the file. A mapping attempt past this
> > +extent may return nothing. But the user must still confirm by trying to map
> > +past this extent, since different filesystems implement this differently.

"This flag means nothing and can be set arbitrarily by the fs for the lulz."

Yuck.  I was really hoping for "This is set on the last extent record in
the dataset generated by the query parameters", particularly becaue
that's how e2fsprogs utilties interpret that flag.

--D
Jan Kara Feb. 26, 2020, 5:26 p.m. UTC | #3
On Wed 26-02-20 08:17:42, Darrick J. Wong wrote:
> On Wed, Feb 26, 2020 at 05:05:03AM -0800, Matthew Wilcox wrote:
> > On Wed, Feb 26, 2020 at 03:27:08PM +0530, Ritesh Harjani wrote:
> > > Currently FIEMAP_EXTENT_LAST is not working consistently across
> > > different filesystem's fiemap implementations and thus this feature
> > > may be broken. So fix the documentation about this flag to meet the
> > > right expectations.
> > 
> > Are you saying filesystems have both false positives and false negatives?
> > I can understand how a filesystem might fail to set FIEMAP_EXTENT_LAST,
> > but not how a filesystem might set it when there's actually another
> > extent beyond this one.
> > 
> > >  * FIEMAP_EXTENT_LAST
> > > -This is the last extent in the file. A mapping attempt past this
> > > -extent will return nothing.
> > > +This is generally the last extent in the file. A mapping attempt past this
> > > +extent may return nothing. But the user must still confirm by trying to map
> > > +past this extent, since different filesystems implement this differently.
> 
> "This flag means nothing and can be set arbitrarily by the fs for the lulz."
> 
> Yuck.  I was really hoping for "This is set on the last extent record in
> the dataset generated by the query parameters", particularly becaue
> that's how e2fsprogs utilties interpret that flag.

The problem is that in some cases, we cannot determine whether the flag
should be set without doing work equivalent to another fiemap call. And it
just seems pointless to try to provide the flag in those cases.

But I agree with Matthew that seeing the flag without the extent really
being the last one shouldn't happen (unless someone is changing the file).

								Honza
Dave Chinner Feb. 27, 2020, 3:24 a.m. UTC | #4
On Wed, Feb 26, 2020 at 06:26:53PM +0100, Jan Kara wrote:
> On Wed 26-02-20 08:17:42, Darrick J. Wong wrote:
> > On Wed, Feb 26, 2020 at 05:05:03AM -0800, Matthew Wilcox wrote:
> > > On Wed, Feb 26, 2020 at 03:27:08PM +0530, Ritesh Harjani wrote:
> > > > Currently FIEMAP_EXTENT_LAST is not working consistently across
> > > > different filesystem's fiemap implementations and thus this feature
> > > > may be broken. So fix the documentation about this flag to meet the
> > > > right expectations.
> > > 
> > > Are you saying filesystems have both false positives and false negatives?
> > > I can understand how a filesystem might fail to set FIEMAP_EXTENT_LAST,
> > > but not how a filesystem might set it when there's actually another
> > > extent beyond this one.
> > > 
> > > >  * FIEMAP_EXTENT_LAST
> > > > -This is the last extent in the file. A mapping attempt past this
> > > > -extent will return nothing.
> > > > +This is generally the last extent in the file. A mapping attempt past this
> > > > +extent may return nothing. But the user must still confirm by trying to map
> > > > +past this extent, since different filesystems implement this differently.
> > 
> > "This flag means nothing and can be set arbitrarily by the fs for the lulz."
> > 
> > Yuck.  I was really hoping for "This is set on the last extent record in
> > the dataset generated by the query parameters", particularly becaue
> > that's how e2fsprogs utilties interpret that flag.
> 
> The problem is that in some cases, we cannot determine whether the flag
> should be set without doing work equivalent to another fiemap call. And it
> just seems pointless to try to provide the flag in those cases.
> 
> But I agree with Matthew that seeing the flag without the extent really
> being the last one shouldn't happen (unless someone is changing the file).

Which, of course, can happen. And it can even be the kernel (e.g.
data writeback converting delalloc extents).

IOWs, the information returned to userspace is out of date before it
even gets to userspace, so userspace can't rely on the LAST flag to
be correct in any situation. That's just an extension of the fact
that userspace cannot rely on the extent map being correct and
accurate in any situation....

Cheers,

Dave.
Ritesh Harjani Feb. 27, 2020, 5:17 a.m. UTC | #5
On 2/26/20 9:47 PM, Darrick J. Wong wrote:
> On Wed, Feb 26, 2020 at 05:05:03AM -0800, Matthew Wilcox wrote:
>> On Wed, Feb 26, 2020 at 03:27:08PM +0530, Ritesh Harjani wrote:
>>> Currently FIEMAP_EXTENT_LAST is not working consistently across
>>> different filesystem's fiemap implementations and thus this feature
>>> may be broken. So fix the documentation about this flag to meet the
>>> right expectations.
>>
>> Are you saying filesystems have both false positives and false negatives?
>> I can understand how a filesystem might fail to set FIEMAP_EXTENT_LAST,
>> but not how a filesystem might set it when there's actually another
>> extent beyond this one.
>>
>>>   * FIEMAP_EXTENT_LAST
>>> -This is the last extent in the file. A mapping attempt past this
>>> -extent will return nothing.
>>> +This is generally the last extent in the file. A mapping attempt past this
>>> +extent may return nothing. But the user must still confirm by trying to map
>>> +past this extent, since different filesystems implement this differently.
> 
> "This flag means nothing and can be set arbitrarily by the fs for the lulz."
> 

:) Got it. Will add more information to it.


> Yuck.  I was really hoping for "This is set on the last extent record in
> the dataset generated by the query parameters", particularly becaue
> that's how e2fsprogs utilties interpret that flag.

-extent may return nothing. But the user must still confirm by trying to map
-past this extent, since different filesystems implement this differently.
+extent may return nothing. In some implementations this flag is also set on
+the last dataset queried by the user (via fiemap->fm_length).


Let me know if above looks good.

-ritesh
diff mbox series

Patch

diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
index f6d9c99103a4..7c5d22511df7 100644
--- a/Documentation/filesystems/fiemap.txt
+++ b/Documentation/filesystems/fiemap.txt
@@ -71,8 +71,7 @@  allocated is less than would be required to map the requested range,
 the maximum number of extents that can be mapped in the fm_extent[]
 array will be returned and fm_mapped_extents will be equal to
 fm_extent_count. In that case, the last extent in the array will not
-complete the requested range and will not have the FIEMAP_EXTENT_LAST
-flag set (see the next section on extent flags).
+complete the requested range.
 
 Each extent is described by a single fiemap_extent structure as
 returned in fm_extents.
@@ -96,7 +95,7 @@  block size of the file system.  With the exception of extents flagged as
 FIEMAP_EXTENT_MERGED, adjacent extents will not be merged.
 
 The fe_flags field contains flags which describe the extent returned.
-A special flag, FIEMAP_EXTENT_LAST is always set on the last extent in
+A special flag, FIEMAP_EXTENT_LAST *may be* set on the last extent in
 the file so that the process making fiemap calls can determine when no
 more extents are available, without having to call the ioctl again.
 
@@ -115,8 +114,9 @@  data. Note that the opposite is not true - it would be valid for
 FIEMAP_EXTENT_NOT_ALIGNED to appear alone.
 
 * FIEMAP_EXTENT_LAST
-This is the last extent in the file. A mapping attempt past this
-extent will return nothing.
+This is generally the last extent in the file. A mapping attempt past this
+extent may return nothing. But the user must still confirm by trying to map
+past this extent, since different filesystems implement this differently.
 
 * FIEMAP_EXTENT_UNKNOWN
 The location of this extent is currently unknown. This may indicate