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

[Amir Goldstein]

On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter <phil@xxxxxxxxxxxxxxxx> wrote:
>
> 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.

We handled this aspect with care by placing compile time checks.

> All 7 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 file_type.h and
> add some useful conversion helpers.
>
> Patches 2-3 make use of some helpers in ufs and hfsplus
> without any on-disk implications.
>
> Patches 4-10 replace the specific implementations in ext2, exofs,
> ext4, ocfs2, f2fs, nilfs and btrfs with the common implementation.
> These patches also now include compile-time checks to ensure that
> the file system specific on-disk bits are equivalent to the common
> implementation FT_* bits. These compile-time checks are only
> included once per file system, as my reasoning is that regardless
> of their location, the build will fail/succeed anyway.
>
> In addition, where needed (for patches which no longer apply),
> I've rebased them to apply to the newest 4.19 kernel sources.
> Each patch is independent of the others, except for the
> common implementation itself which they all depend on.
>

This is still RFC, but since this is an exercise let's talk about social
engineering. First (now) you want to gather ACKs and review comments
from fs developers/maintainers.

Once all comments are addressed and assuming no objections, you have
two possible ways forward:

1. Wait for VFS maintainer to take the common code and patches ACKed
    by individual maintainers (Al practically maintains some of the small fs
    himself). Al's backlog is long, so this may be a long wait.

2. Ask a filesystem maintainer to carry the common patch along with his
   specific fs patch through his tree and let other maintainers apply the patch
   at their own will on the following development cycle.
   The ext2 tree would be a good candidate for such a move, if Jan accepts
   this change, considering that all other fs copied the buggy code from ext2.

> I would love feedback as newish kernel contributor, particularly
> from the original author Amir who suggested this task for me. I
> hope the various subsystem/fs maintainers will see fit to accept
> this work also.
>

I usually prefer to have the revisions log in the cover letter for
a 10 patch series, much less work and more appropriate when
revision log looks mostly the same on all patches, e.g.:

v2:
- Rebased against Linux 4.19 by Phillip Potter
- This version does not remove filesystem specific XXX_FT_x definitions,
  as these values are now used in compile-time checks added by
  Phillip Potter to make sure they remain the same as FT_x values
- Removed xfs patch (a variant of original patch has already been applied)
- Anything else relevant to the context of the series...

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

> Phillip Potter (10):
>   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
>   exofs: use common file type conversion
>   ext4: use common file type conversion
>   ocfs2: use common file type conversion
>   f2fs: use common file type conversion
>   nilfs2: use common file type conversion
>   btrfs: use common file type conversion
> ---
>  MAINTAINERS                        |   1 +
>  fs/btrfs/btrfs_inode.h             |   2 -
>  fs/btrfs/delayed-inode.c           |   2 +-
>  fs/btrfs/inode.c                   |  35 +++++-----
>  fs/exofs/dir.c                     |  49 +++++--------
>  fs/ext2/dir.c                      |  51 ++++++--------
>  fs/ext4/ext4.h                     |  35 +++++-----
>  fs/f2fs/dir.c                      |  43 +++++-------
>  fs/f2fs/inline.c                   |   2 +-
>  fs/hfsplus/dir.c                   |  16 +----
>  fs/nilfs2/dir.c                    |  54 +++++----------
>  fs/ocfs2/dir.c                     |  32 +++++----
>  fs/ocfs2/ocfs2_fs.h                |  13 +---
>  fs/ufs/util.h                      |  29 +-------
>  include/linux/f2fs_fs.h            |   8 ++-
>  include/linux/file_type.h          | 108 +++++++++++++++++++++++++++++
>  include/linux/fs.h                 |  17 +----
>  include/uapi/linux/btrfs_tree.h    |   2 +
>  include/uapi/linux/nilfs2_ondisk.h |   1 +
>  19 files changed, 256 insertions(+), 244 deletions(-)
>  create mode 100644 include/linux/file_type.h
>
> --
> 2.17.2
>