| Message ID | 279638c6939b1f6ef3ab32912cb51da1a967cf8e.1582702694.git.riteshh@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4: bmap & fiemap conversion to use iomap | expand |
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.
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
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
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.
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 --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
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(-)