[RFC][PATCH v5 00/09] common implementation of dirent file types

[RFC][PATCH v5 00/09] common implementation of dirent file types

[Phillip Potter]

This cleanup series is a respin of Amir Goldstein's work, created
in late 2016. It removes several instances of duplicated code. Most
of the duplication dates back to git pre-historic era.

The controversial aspect of this cleanup is that it uses common
code to handle file system specific on-disk format bits. The previous
series handled this aspect by placing compile-time checks to catch
cases where the fs specific definitions deviated from the common
ones. Linus Torvalds mentioned that he was not happy with the
concept of two sets of definitions having to stay in sync, and the
code that goes along with that. It was agreed that if the values
must stay the same forever, it is better to keep the common
definitions and remove the filesystem specific ones. This discussion
can be followed at:
https://marc.info/?l=linux-fsdevel&m=154758043730079&w=2

All 6 file systems use a single byte to store dirent file type
on-disk and all of them use the same conversion routines from
i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants.

Patch 1 places a common implementation in fs_types.c with some useful
conversion helpers. The corresponding header fs_types.h contains
related macros and definitions, as well as helper declarations.

Patches 2-3 make use of the helpers in ufs and hfsplus
without any on-disk implications.

Patches 4-9 replace the specific implementations in ext2, ocfs2,
f2fs, ext4, nilfs2 and btrfs with the common implementation.
The ext2 implementation appears to have been copied as a basis of
the others, and in particular, for code that tries to access the
ext2_type_by_mode array there is a bug that this patch series
fixes. Essentially, it is defined with size S_IFMT >> S_SHIFT,
so 15. This means that it is possible with a malformed inode to
get an index of 15, as the array is always accessed with:
ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];

These patches mostly no longer include compile-time checks to ensure
the filesystem specific on-disk bits are equivalent to the common
implementation FT_* bits, and instead op to remove the filesystem
specific definitions entirely where possible, as a result of the
referenced discussion above.

With the ext4 patch, the EXT4_FT_* definitions are instead defined
to be FT_*, to give less code churn with the same result (no need
to modify fs/ext4/namei.c). Also, the nilfs2 and btrfs filesystems
keep their filesystem specific definitions in the include/uapi/linux
directory, so these cannot be changed trivially without breaking
userspace. For this reason, the compile time checks remain in these
two filesystems.

In addition, I've checked that the patches apply and build with the
newest 5.0-rc2 kernel sources, and that they work. Each patch is
independent of the others, except for the common implementation
itself which they all depend on.

General motivations for the patch (other than the bugfix above) are
the fact that it reduces the amount of code for the covered filesystems:
ufs: -27 lines
hfsplus: -12 lines
ext2: -39 lines
ocfs2: -38 lines
f2fs: -38 lines
ext4: -13 lines
nilsfs2: -17 lines
btrfs: -2 lines

It also makes it easier to add hypothetical future filesystems which
wish to reuse these POSIX-type definitions and on-disk formats, as they
can simply now use the common implementation.

I welcome feedback, and thank those who have already provided it.
Hopefully, the relevant maintainers will see fit to merge this work.

v5:
- Added extra comment wording in the common implementation patch to
  make it clear that the FT_* definitions must never change, as they
  are now being depended on by multiple filesystems
- Removed compile-time checks from ext2, ext4, ocfs2 and f2fs patches
- Modified ext2, ocfs2 and f2fs patches to remove the filesystem
  specific **_FT_* definitions completely
- Modified ext4 patch to define EXT4_FT_* as FT_*. This results in
  less code churn and no need to modify fs/ext4/namei.c
- Reapplied and tested against 5.0-rc2
- Removed some Acked-by and Reviewed-by tags due to refactoring and
  the subsequent desire for rechecking by maintainers.

v4:
- Removed exofs patch from series, as there is ongoing discussion on
  whether or not exofs should be removed from the kernel tree - happy
  to add it back should exofs end up staying, particularly as Boaz
  acked my patch.
- Reapplied and tested against 4.20-rc3
- Moved compile-time checks in btrfs patch to more consistent location
- Incorporated Acked-by and Reviewed-by tags received so far

v3:
- Moved compile-time checks to better locations, and shortened comments,
  as well as formatting to 80 columns
- Renamed header to fs_types.h, and placed comment regarding DT_* constants
  relation to POSIX and glibc dirent.h
- Changed "should" to "must" in comments mentioning the need for fs specific
  values to match the common values in fs_types.h
- Split functions and lookup tables to separate C file (fs/fs_types.c)
- Added kernel-doc comments for all three utility functions, and removed
  therefore unnecessary lines from comment in fs_types.h
- Tweaked commit messages slightly to explain that file systems using
  POSIX file types now don't need to define their own conversion routines
- Renamed fs_dtype function to fs_ftype_to_dtype for consistency with other
  helper functions
- Tweaked fs_ftype_to_dtype to take unsigned int argument, to prevent
  out of bounds access of memory
- Added additional text to comment in fs_types.h explaining the definitions
  must never change
- Changed DT_* types in fs_types.h to explicit constants as they can't
  change anyway
- 80 line violation in the ext2 patch - left as is due to the fact the code
  would look worse if fixed, other patches tweaked slightly to keep within
  80 line limit (by moving last parameter in various if statements)

v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove filesystem specific XXX_FT_* definitions,
  as these values are now used in compile-time checks added by Phillip
  Potter to make sure they remain the same as the generic FT_* values
- Removed xfs patch (a variant of original patch has already been applied)
- Added SPDX tag to new header file and included it in MAINTAINERS

v1:
- Initial implementation by Amir Goldstein:
  https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2

Phillip Potter (9):
  fs: common implementation of file type conversions
  ufs: use fs_umode_to_dtype() helper
  hfsplus: use fs_umode_to_dtype() helper
  ext2: use common file type conversion
  ocfs2: use common file type conversion
  f2fs: use common file type conversion
  ext4: use common file type conversion
  nilfs2: use common file type conversion
  btrfs: use common file type conversion

 MAINTAINERS                        |   1 +
 fs/Makefile                        |   3 +-
 fs/btrfs/btrfs_inode.h             |   2 -
 fs/btrfs/delayed-inode.c           |   2 +-
 fs/btrfs/inode.c                   |  32 +++++----
 fs/ext2/dir.c                      |  35 ++--------
 fs/ext2/ext2.h                     |  16 -----
 fs/ext4/ext4.h                     |  37 ++++------
 fs/f2fs/dir.c                      |  27 +-------
 fs/f2fs/inline.c                   |   2 +-
 fs/fs_types.c                      | 105 +++++++++++++++++++++++++++++
 fs/hfsplus/dir.c                   |  16 +----
 fs/nilfs2/dir.c                    |  52 +++++---------
 fs/ocfs2/dir.c                     |  20 ++----
 fs/ocfs2/ocfs2_fs.h                |  28 +-------
 fs/ufs/util.h                      |  29 +-------
 include/linux/f2fs_fs.h            |  15 -----
 include/linux/fs.h                 |  17 +----
 include/linux/fs_types.h           |  75 +++++++++++++++++++++
 include/uapi/linux/btrfs_tree.h    |   2 +
 include/uapi/linux/nilfs2_ondisk.h |   1 +
 21 files changed, 249 insertions(+), 268 deletions(-)
 create mode 100644 fs/fs_types.c
 create mode 100644 include/linux/fs_types.h

--
2.19.1

Re: [RFC][PATCH v5 00/09] common implementation of dirent file types

[Matthew Wilcox]

On Mon, Jan 21, 2019 at 12:54:25AM +0000, Phillip Potter wrote:
> These patches mostly no longer include compile-time checks to ensure
> the filesystem specific on-disk bits are equivalent to the common
> implementation FT_* bits, and instead op to remove the filesystem
> specific definitions entirely where possible, as a result of the
> referenced discussion above.
> 
> With the ext4 patch, the EXT4_FT_* definitions are instead defined
> to be FT_*, to give less code churn with the same result (no need
> to modify fs/ext4/namei.c). Also, the nilfs2 and btrfs filesystems
> keep their filesystem specific definitions in the include/uapi/linux
> directory, so these cannot be changed trivially without breaking
> userspace. For this reason, the compile time checks remain in these
> two filesystems.

Just because something is exposed through the uapi directory today
doesn't mean userspace actually uses it.  For example,
https://codesearch.debian.net/search?q=BTRFS_FT_DIR

The only code which uses the filetype defines is going to be code which
actually looks at a raw filesystem image.  All three examples of userspace
code in Debian have their own definitions instead of using the one which
the kernel provides.

Re: [RFC][PATCH v5 00/09] common implementation of dirent file types

[Amir Goldstein]

On Mon, Jan 21, 2019 at 11:35 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Jan 21, 2019 at 12:54:25AM +0000, Phillip Potter wrote:
> > These patches mostly no longer include compile-time checks to ensure
> > the filesystem specific on-disk bits are equivalent to the common
> > implementation FT_* bits, and instead op to remove the filesystem
> > specific definitions entirely where possible, as a result of the
> > referenced discussion above.
> >
> > With the ext4 patch, the EXT4_FT_* definitions are instead defined
> > to be FT_*, to give less code churn with the same result (no need
> > to modify fs/ext4/namei.c). Also, the nilfs2 and btrfs filesystems
> > keep their filesystem specific definitions in the include/uapi/linux
> > directory, so these cannot be changed trivially without breaking
> > userspace. For this reason, the compile time checks remain in these
> > two filesystems.
>
> Just because something is exposed through the uapi directory today
> doesn't mean userspace actually uses it.  For example,
> https://codesearch.debian.net/search?q=BTRFS_FT_DIR
>
> The only code which uses the filetype defines is going to be code which
> actually looks at a raw filesystem image.  All three examples of userspace
> code in Debian have their own definitions instead of using the one which
> the kernel provides.

That's a good point, but I think we should let btrfs developers choose
where and how to change the definitions.
Probably the ext4 example (patch 7) would be the best to follow, as btrfs
also uses some non common constants (BTRFS_FT_MAX,
BTRFS_FT_XATTR) that do not map to the common DT_* values.

Thanks,
Amir.

Re: [RFC][PATCH v5 00/09] common implementation of dirent file types

[Phillip Potter]

On Mon, Jan 21, 2019 at 01:35:10AM -0800, Matthew Wilcox wrote:
> On Mon, Jan 21, 2019 at 12:54:25AM +0000, Phillip Potter wrote:
> > These patches mostly no longer include compile-time checks to ensure
> > the filesystem specific on-disk bits are equivalent to the common
> > implementation FT_* bits, and instead op to remove the filesystem
> > specific definitions entirely where possible, as a result of the
> > referenced discussion above.
> > 
> > With the ext4 patch, the EXT4_FT_* definitions are instead defined
> > to be FT_*, to give less code churn with the same result (no need
> > to modify fs/ext4/namei.c). Also, the nilfs2 and btrfs filesystems
> > keep their filesystem specific definitions in the include/uapi/linux
> > directory, so these cannot be changed trivially without breaking
> > userspace. For this reason, the compile time checks remain in these
> > two filesystems.
> 
> Just because something is exposed through the uapi directory today
> doesn't mean userspace actually uses it.  For example,
> https://codesearch.debian.net/search?q=BTRFS_FT_DIR
> 
> The only code which uses the filetype defines is going to be code which
> actually looks at a raw filesystem image.  All three examples of userspace
> code in Debian have their own definitions instead of using the one which
> the kernel provides.

Thank you for this suggestion. I will wait and then possibly refactor the
last two patches of the series.

Regards,
Phil