[PATCHv5,6/6] Documentation: Correct the description of FIEMAP_EXTENT_LAST
diff mbox series

Message ID 5a00e8d4283d6849e0b8f408c8365b31fbc1d153.1582880246.git.riteshh@linux.ibm.com
State New
Headers show
Series
  • ext4: bmap & fiemap conversion to use iomap
Related show

Commit Message

Ritesh Harjani Feb. 28, 2020, 9:26 a.m. UTC
Currently FIEMAP_EXTENT_LAST is not working consistently across
different filesystem's fiemap implementations. So add more information
about how else this flag could set in other implementation.

Also in general, user should not completely rely on this flag as
such since it could return false value for e.g.
when there is a delalloc extent which might get converted during
writeback, immediately after the fiemap calls return.

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

Comments

Darrick J. Wong Feb. 28, 2020, 3:20 p.m. UTC | #1
On Fri, Feb 28, 2020 at 02:56:59PM +0530, Ritesh Harjani wrote:
> Currently FIEMAP_EXTENT_LAST is not working consistently across
> different filesystem's fiemap implementations. So add more information
> about how else this flag could set in other implementation.
> 
> Also in general, user should not completely rely on this flag as
> such since it could return false value for e.g.
> when there is a delalloc extent which might get converted during
> writeback, immediately after the fiemap calls return.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>

Looks reasonable,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  Documentation/filesystems/fiemap.txt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
> index f6d9c99103a4..fedfa9b9dde5 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. In some implementations this flag is also set on
> +the last dataset queried by the user (via fiemap->fm_length).
>  
>  * FIEMAP_EXTENT_UNKNOWN
>  The location of this extent is currently unknown. This may indicate
> -- 
> 2.21.0
>
Matthew Wilcox Feb. 28, 2020, 3:36 p.m. UTC | #2
On Fri, Feb 28, 2020 at 02:56:59PM +0530, Ritesh Harjani wrote:
> Currently FIEMAP_EXTENT_LAST is not working consistently across
> different filesystem's fiemap implementations. So add more information
> about how else this flag could set in other implementation.
> 
> Also in general, user should not completely rely on this flag as
> such since it could return false value for e.g.
> when there is a delalloc extent which might get converted during
> writeback, immediately after the fiemap calls return.
> 
> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>  Documentation/filesystems/fiemap.txt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
> index f6d9c99103a4..fedfa9b9dde5 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.

This sentence still seems like it should be true.  If the filesystem knows
there are more extents to come, it will definitely not set the LAST flag.

> @@ -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.

I'm not sure I'd highlight 'may be' here.

> @@ -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. In some implementations this flag is also set on
> +the last dataset queried by the user (via fiemap->fm_length).

The word 'dataset' is used nowhere else in this document.  How about

"Some filesystems set this flag to indicate this extent is the last one in
the range queried by the user"
Ritesh Harjani March 2, 2020, 8:10 a.m. UTC | #3
On 2/28/20 9:06 PM, Matthew Wilcox wrote:
> On Fri, Feb 28, 2020 at 02:56:59PM +0530, Ritesh Harjani wrote:
>> Currently FIEMAP_EXTENT_LAST is not working consistently across
>> different filesystem's fiemap implementations. So add more information
>> about how else this flag could set in other implementation.
>>
>> Also in general, user should not completely rely on this flag as
>> such since it could return false value for e.g.
>> when there is a delalloc extent which might get converted during
>> writeback, immediately after the fiemap calls return.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
>> ---
>>   Documentation/filesystems/fiemap.txt | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
>> index f6d9c99103a4..fedfa9b9dde5 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.
> 
> This sentence still seems like it should be true.  If the filesystem knows
> there are more extents to come, it will definitely not set the LAST flag.
> 

sure.

>> @@ -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.
> 
> I'm not sure I'd highlight 'may be' here.

Sure.

> 
>> @@ -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. In some implementations this flag is also set on
>> +the last dataset queried by the user (via fiemap->fm_length).
> 
> The word 'dataset' is used nowhere else in this document.  How about
> 
> "Some filesystems set this flag to indicate this extent is the last one in
> the range queried by the user"

Sure.

Thanks for the review.
Will make the suggested changes and send a v6.

-ritesh
Theodore Y. Ts'o March 14, 2020, 3:48 a.m. UTC | #4
On Mon, Mar 02, 2020 at 01:40:06PM +0530, Ritesh Harjani wrote:
> 
> Thanks for the review.
> Will make the suggested changes and send a v6.

I didn't see a v6, so I revised this patch to read:

commit 499800830ae5d44ae29f69c98ab9893f0425cb51
Author: Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Fri Feb 28 14:56:59 2020 +0530

    Documentation: correct the description of FIEMAP_EXTENT_LAST
    
    Currently FIEMAP_EXTENT_LAST is not working consistently across
    different filesystem's fiemap implementations. So add more information
    about how else this flag could set in other implementation.
    
    Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
    Link: https://lore.kernel.org/r/5a00e8d4283d6849e0b8f408c8365b31fbc1d153.1582880246.git.riteshh@linux.ibm.com
    Signed-off-by: Theodore Ts'o <tytso@mit.edu>

diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
index f6d9c99103a4..ac87e6fda842 100644
--- a/Documentation/filesystems/fiemap.txt
+++ b/Documentation/filesystems/fiemap.txt
@@ -115,8 +115,10 @@ 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. Some implementations set this flag to
+indicate this extent is the last one in the range queried by the user
+(via fiemap->fm_length).
 
 * FIEMAP_EXTENT_UNKNOWN
 The location of this extent is currently unknown. This may indicate

     	      	      	     		  	   - Ted

Patch
diff mbox series

diff --git a/Documentation/filesystems/fiemap.txt b/Documentation/filesystems/fiemap.txt
index f6d9c99103a4..fedfa9b9dde5 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. In some implementations this flag is also set on
+the last dataset queried by the user (via fiemap->fm_length).
 
 * FIEMAP_EXTENT_UNKNOWN
 The location of this extent is currently unknown. This may indicate