[RFC][PATCH v3 00/10] common implementation of dirent file types

[RFC][PATCH v3 00/10] 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. 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 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-10 replace the specific implementations in ext2, exofs,
ext4, ocfs2, f2fs, nilfs 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 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.

General motivations for the patch (other than the bugfix above) are
the fact that it reduces the amount of code for most filesystems
(except ext4):
ufs: -27 lines
hfsplus: -12 lines
ext2: -9 lines
exofs: -16 lines
ext4: +1 line
ocfs2: -9 lines
f2fs: -7 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.

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 (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/Makefile                        |   3 +-
 fs/btrfs/btrfs_inode.h             |   2 -
 fs/btrfs/delayed-inode.c           |   2 +-
 fs/btrfs/inode.c                   |  32 +++++----
 fs/exofs/dir.c                     |  48 +++++--------
 fs/ext2/dir.c                      |  49 ++++++--------
 fs/ext4/ext4.h                     |  33 ++++-----
 fs/f2fs/dir.c                      |  41 +++++------
 fs/f2fs/inline.c                   |   2 +-
 fs/fs_types.c                      | 105 +++++++++++++++++++++++++++++
 fs/hfsplus/dir.c                   |  16 +----
 fs/nilfs2/dir.c                    |  52 +++++---------
 fs/ocfs2/dir.c                     |  18 +----
 fs/ocfs2/ocfs2_fs.h                |  27 ++++----
 fs/ufs/util.h                      |  29 +-------
 include/linux/f2fs_fs.h            |   8 ++-
 include/linux/fs.h                 |  17 +----
 include/linux/fs_types.h           |  73 ++++++++++++++++++++
 include/uapi/linux/btrfs_tree.h    |   2 +
 include/uapi/linux/nilfs2_ondisk.h |   1 +
 21 files changed, 314 insertions(+), 247 deletions(-)
 create mode 100644 fs/fs_types.c
 create mode 100644 include/linux/fs_types.h

--
2.17.2

[RFC][PATCH v3 10/10] btrfs: use common file type conversion

[Phillip Potter]

Deduplicate the btrfs file type conversion implementation - file systems
that use the same file types as defined by POSIX do not need to define
their own versions and can use the common helper functions decared in
fs_types.h and implemented in fs_types.c

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 fs/btrfs/btrfs_inode.h          |  2 --
 fs/btrfs/delayed-inode.c        |  2 +-
 fs/btrfs/inode.c                | 32 +++++++++++++++-----------------
 include/uapi/linux/btrfs_tree.h |  2 ++
 4 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 1343ac57b438..c7c6db6b4a35 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -196,8 +196,6 @@ struct btrfs_inode {
 	struct inode vfs_inode;
 };
 
-extern unsigned char btrfs_filetype_table[];
-
 static inline struct btrfs_inode *BTRFS_I(const struct inode *inode)
 {
 	return container_of(inode, struct btrfs_inode, vfs_inode);
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index f51b509f2d9b..c1da34e3a775 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1691,7 +1691,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx,
 		name = (char *)(di + 1);
 		name_len = btrfs_stack_dir_name_len(di);
 
-		d_type = btrfs_filetype_table[di->type];
+		d_type = fs_ftype_to_dtype(di->type);
 		btrfs_disk_key_to_cpu(&location, &di->location);
 
 		over = !dir_emit(ctx, name, name_len,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3ea5339603cf..089638719842 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -73,17 +73,6 @@ struct kmem_cache *btrfs_trans_handle_cachep;
 struct kmem_cache *btrfs_path_cachep;
 struct kmem_cache *btrfs_free_space_cachep;
 
-#define S_SHIFT 12
-static const unsigned char btrfs_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= BTRFS_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= BTRFS_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= BTRFS_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= BTRFS_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= BTRFS_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= BTRFS_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= BTRFS_FT_SYMLINK,
-};
-
 static int btrfs_setsize(struct inode *inode, struct iattr *attr);
 static int btrfs_truncate(struct inode *inode, bool skip_writeback);
 static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent);
@@ -5803,10 +5792,6 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,
 	return d_splice_alias(inode, dentry);
 }
 
-unsigned char btrfs_filetype_table[] = {
-	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
-};
-
 /*
  * All this infrastructure exists because dir_emit can fault, and we are holding
  * the tree lock when doing readdir.  For now just allocate a buffer and copy
@@ -5879,6 +5864,19 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 	bool put = false;
 	struct btrfs_key location;
 
+	/*
+	 * compile-time asserts that generic FT_x types still match
+	 * BTRFS_FT_x types
+	 */
+	BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN);
+	BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE);
+	BUILD_BUG_ON(BTRFS_FT_DIR != FT_DIR);
+	BUILD_BUG_ON(BTRFS_FT_CHRDEV != FT_CHRDEV);
+	BUILD_BUG_ON(BTRFS_FT_BLKDEV != FT_BLKDEV);
+	BUILD_BUG_ON(BTRFS_FT_FIFO != FT_FIFO);
+	BUILD_BUG_ON(BTRFS_FT_SOCK != FT_SOCK);
+	BUILD_BUG_ON(BTRFS_FT_SYMLINK != FT_SYMLINK);
+
 	if (!dir_emit_dots(file, ctx))
 		return 0;
 
@@ -5945,7 +5943,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		name_ptr = (char *)(entry + 1);
 		read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
 				   name_len);
-		put_unaligned(btrfs_filetype_table[btrfs_dir_type(leaf, di)],
+		put_unaligned(fs_ftype_to_dtype(btrfs_dir_type(leaf, di)),
 				&entry->type);
 		btrfs_dir_item_key_to_cpu(leaf, di, &location);
 		put_unaligned(location.objectid, &entry->ino);
@@ -6350,7 +6348,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 
 static inline u8 btrfs_inode_type(struct inode *inode)
 {
-	return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT];
+	return fs_umode_to_ftype(inode->i_mode);
 }
 
 /*
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index aff1356c2bb8..a4f5fb56a45b 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -307,6 +307,8 @@
  *
  * Used by:
  * struct btrfs_dir_item.type
+ *
+ * Values 0..7 must match common file type values in fs_types.h.
  */
 #define BTRFS_FT_UNKNOWN	0
 #define BTRFS_FT_REG_FILE	1
-- 
2.17.2

Re: [RFC][PATCH v3 10/10] btrfs: use common file type conversion

[David Sterba]

On Sat, Oct 27, 2018 at 01:53:48AM +0100, Phillip Potter wrote:
> Deduplicate the btrfs file type conversion implementation - file systems
> that use the same file types as defined by POSIX do not need to define
> their own versions and can use the common helper functions decared in
> fs_types.h and implemented in fs_types.c
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>

Acked-by: David Sterba <dsterba@suse.com>

Re: [RFC][PATCH v3 10/10] btrfs: use common file type conversion

[Phillip Potter]

On Tue, Oct 30, 2018 at 08:03:17PM +0100, David Sterba wrote:
> On Sat, Oct 27, 2018 at 01:53:48AM +0100, Phillip Potter wrote:
> > Deduplicate the btrfs file type conversion implementation - file systems
> > that use the same file types as defined by POSIX do not need to define
> > their own versions and can use the common helper functions decared in
> > fs_types.h and implemented in fs_types.c
> > 
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> 
> Acked-by: David Sterba <dsterba@suse.com>

Dear David,

Thanks for this - are you happy for me to move the compile-time tests as per
Amir's suggestion?

Regards,
Phil

Re: [RFC][PATCH v3 10/10] btrfs: use common file type conversion

[David Sterba]

On Wed, Oct 31, 2018 at 09:22:31AM +0000, Phillip Potter wrote:
> Thanks for this - are you happy for me to move the compile-time tests as per
> Amir's suggestion?

Yes that's ok.