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

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

[Amir Goldstein]

This cleanup series removes 8 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.
All 8 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 hoists a common implementation to file_type.h and
add some useful conversion helpers.

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

Patches 4-3 replace the specific implementation in ext2 and exofs
with the common implementation.  In this case, the fs specific
constants (EXT2_FT_*) are defined in local include file and never
used, so the constants have been removed.

Patches 5-6 replace the specific implementation in ext4 and ocfs2
with the common implementation.  In this case, the fs specific
constants (EXT4_FT_*) are defined in local include file that look
like it is a version of an include file used in a library.
The constants remained in the include file and were defined to
refer to the common constants (EXT4_FT_DIR => FT_DIR).

Patches 7-8 replace the specific implementation in f2fs and nilfs2
with the common implementation.  In this case, the fs specific
constants (F2FS_FT_*) are defined in a global include file.
The constants remained in the include file a comment was added
to keep them in sync with the common constants.

Patches 9-10 replace the specific implementation in btrfs and xfs
with the common implementation.  In this case, the fs specific
constants (BTRFS_FT_*) are defined in an include file that is
also used by a library.  Also, both btrfs and xfs use the 4th bit
of the file type on-disk.  The constants remained in the include
file and a comment was added that constants 0..7 should be kept
in sync with the common constants.

I posted xfstest generic/396 to sanity test d_type in readdir.
Tested these patches on ext2, ext4, xfs.
In any case, all the individual fs patches are completely independent
of each other and all depend only on patch 1, so each fs maintainer
can test and apply the relevant patch to his tree after the
common implementation is merged.

Obviously, I am looking for feedback from fs maintainers if the
approach chosen for their fs is the right approach and feedback
on the cleanup in general.

Amir Goldstein (11):
  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
  xfs: use common file type conversion

 fs/btrfs/btrfs_inode.h             |   2 -
 fs/btrfs/delayed-inode.c           |   2 +-
 fs/btrfs/inode.c                   |  19 +------
 fs/exofs/common.h                  |  12 -----
 fs/exofs/dir.c                     |  34 +-----------
 fs/ext2/dir.c                      |  35 +++---------
 fs/ext2/ext2.h                     |  16 ------
 fs/ext4/ext4.h                     |  38 +++++--------
 fs/f2fs/dir.c                      |  27 +---------
 fs/f2fs/inline.c                   |   2 +-
 fs/hfsplus/dir.c                   |  16 +-----
 fs/nilfs2/dir.c                    |  38 ++-----------
 fs/ocfs2/dir.c                     |  18 ++-----
 fs/ocfs2/ocfs2_fs.h                |  32 ++++-------
 fs/ufs/util.h                      |  29 +---------
 fs/xfs/libxfs/xfs_da_format.h      |   5 +-
 fs/xfs/libxfs/xfs_dir2.c           |  17 ------
 fs/xfs/libxfs/xfs_dir2.h           |   6 ---
 fs/xfs/xfs_iops.c                  |   2 +-
 include/linux/f2fs_fs.h            |   8 +--
 include/linux/file_type.h          | 107 +++++++++++++++++++++++++++++++++++++
 include/linux/fs.h                 |  17 +-----
 include/uapi/linux/btrfs_tree.h    |   2 +
 include/uapi/linux/nilfs2_ondisk.h |   1 +
 24 files changed, 167 insertions(+), 318 deletions(-)
 create mode 100644 include/linux/file_type.h

-- 
2.7.4

[RFC][PATCH 01/11] fs: common implementation of file type conversions

[Amir Goldstein]

Many file systems use a copy&paste implementation
of dirent to on-disk file type conversions.

Create a common implementation to be used by file systems
with some useful conversion helpers to reduce open coded
file type conversions in file system code.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/file_type.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h        |  17 +-------
 2 files changed, 108 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/file_type.h

diff --git a/include/linux/file_type.h b/include/linux/file_type.h
new file mode 100644
index 0000000..03ee1a1
--- /dev/null
+++ b/include/linux/file_type.h
@@ -0,0 +1,107 @@
+#ifndef _LINUX_FILE_TYPE_H
+#define _LINUX_FILE_TYPE_H
+
+/*
+ * This is a common implementation of dirent to fs on-disk
+ * file type conversion.  Although the fs on-disk bits are
+ * specific to every file system, in practice, many file systems
+ * use the exact same on-disk format to describe the lower 3
+ * file type bits that represent the 7 POSIX file types.
+ * All those file systems can use this generic code for the
+ * conversions:
+ *  i_mode -> fs on-disk file type (ftype)
+ *  fs on-disk file type (ftype) -> dirent file type (dtype)
+ *  i_mode -> dirent file type (dtype)
+ */
+
+/*
+ * struct dirent file types
+ * exposed to user via getdents(2), readdir(3)
+ *
+ * These match bits 12..15 of stat.st_mode
+ * (ie "(i_mode >> 12) & 15").
+ */
+#define S_DT_SHIFT	12
+#define S_DT(mode)	(((mode) & S_IFMT) >> S_DT_SHIFT)
+#define DT_MASK		(S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN	0
+#define DT_FIFO		S_DT(S_IFIFO) /* 1 */
+#define DT_CHR		S_DT(S_IFCHR) /* 2 */
+#define DT_DIR		S_DT(S_IFDIR) /* 4 */
+#define DT_BLK		S_DT(S_IFBLK) /* 6 */
+#define DT_REG		S_DT(S_IFREG) /* 8 */
+#define DT_LNK		S_DT(S_IFLNK) /* 10 */
+#define DT_SOCK		S_DT(S_IFSOCK) /* 12 */
+#define DT_WHT		14
+
+#define DT_MAX		(DT_MASK + 1) /* 16 */
+
+/*
+ * fs on-disk file types.
+ * Only the low 3 bits are used for the POSIX file types.
+ * Other bits are reserved for fs private use.
+ *
+ * Note that no fs currently stores the whiteout type on-disk,
+ * so whiteout dirents are exposed to user as DT_CHR.
+ */
+#define FT_UNKNOWN	0
+#define FT_REG_FILE	1
+#define FT_DIR		2
+#define FT_CHRDEV	3
+#define FT_BLKDEV	4
+#define FT_FIFO		5
+#define FT_SOCK		6
+#define FT_SYMLINK	7
+
+#define FT_MAX		8
+
+/*
+ * fs on-disk file type to dirent file type conversion
+ */
+static unsigned char fs_dtype_by_ftype[] = {
+	DT_UNKNOWN,
+	DT_REG,
+	DT_DIR,
+	DT_CHR,
+	DT_BLK,
+	DT_FIFO,
+	DT_SOCK,
+	DT_LNK
+};
+
+static inline unsigned char fs_dtype(int filetype)
+{
+	if (filetype >= FT_MAX)
+		return DT_UNKNOWN;
+
+	return fs_dtype_by_ftype[filetype];
+}
+
+/*
+ * dirent file type to fs on-disk file type conversion
+ * Values not initialized explicitly are FT_UNKNOWN (0).
+ */
+static unsigned char fs_ftype_by_dtype[DT_MAX] = {
+	[DT_REG]	= FT_REG_FILE,
+	[DT_DIR]	= FT_DIR,
+	[DT_LNK]	= FT_SYMLINK,
+	[DT_CHR]	= FT_CHRDEV,
+	[DT_BLK]	= FT_BLKDEV,
+	[DT_FIFO]	= FT_FIFO,
+	[DT_SOCK]	= FT_SOCK,
+};
+
+/* st_mode to fs on-disk file type conversion */
+static inline unsigned char fs_umode_to_ftype(umode_t mode)
+{
+	return fs_ftype_by_dtype[S_DT(mode)];
+}
+
+/* st_mode to dirent file type conversion */
+static inline unsigned char fs_umode_to_dtype(umode_t mode)
+{
+	return fs_dtype(fs_umode_to_ftype(mode));
+}
+
+#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e6e4146..8f1580d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -31,6 +31,7 @@
 #include <linux/workqueue.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/delayed_call.h>
+#include <linux/file_type.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -1582,22 +1583,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
 
 /*
- * File types
- *
- * NOTE! These match bits 12..15 of stat.st_mode
- * (ie "(i_mode >> 12) & 15").
- */
-#define DT_UNKNOWN	0
-#define DT_FIFO		1
-#define DT_CHR		2
-#define DT_DIR		4
-#define DT_BLK		6
-#define DT_REG		8
-#define DT_LNK		10
-#define DT_SOCK		12
-#define DT_WHT		14
-
-/*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.
  * This allows the kernel to read directories into kernel space or
-- 
2.7.4

[RFC][PATCH 02/11] ufs: use fs_umode_to_dtype() helper

[Amir Goldstein]

replace switch statement with common lookup table implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ufs/util.h | 29 +----------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index b7fbf53..2a853e0 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -157,34 +157,7 @@ ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
 	if ((UFS_SB(sb)->s_flags & UFS_DE_MASK) != UFS_DE_44BSD)
 		return;
 
-	/*
-	 * TODO turn this into a table lookup
-	 */
-	switch (mode & S_IFMT) {
-	case S_IFSOCK:
-		de->d_u.d_44.d_type = DT_SOCK;
-		break;
-	case S_IFLNK:
-		de->d_u.d_44.d_type = DT_LNK;
-		break;
-	case S_IFREG:
-		de->d_u.d_44.d_type = DT_REG;
-		break;
-	case S_IFBLK:
-		de->d_u.d_44.d_type = DT_BLK;
-		break;
-	case S_IFDIR:
-		de->d_u.d_44.d_type = DT_DIR;
-		break;
-	case S_IFCHR:
-		de->d_u.d_44.d_type = DT_CHR;
-		break;
-	case S_IFIFO:
-		de->d_u.d_44.d_type = DT_FIFO;
-		break;
-	default:
-		de->d_u.d_44.d_type = DT_UNKNOWN;
-	}
+	de->d_u.d_44.d_type = fs_umode_to_dtype(mode);
 }
 
 static inline u32
-- 
2.7.4

[RFC][PATCH 03/11] hfsplus: use fs_umode_to_dtype() helper

[Amir Goldstein]

replace if/else statements with common lookup table implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/hfsplus/dir.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 31d5e3f..edf60c7 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -224,7 +224,6 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 				break;
 		} else if (type == HFSPLUS_FILE) {
 			u16 mode;
-			unsigned type = DT_UNKNOWN;
 
 			if (fd.entrylength < sizeof(struct hfsplus_cat_file)) {
 				pr_err("small file entry\n");
@@ -233,21 +232,10 @@ static int hfsplus_readdir(struct file *file, struct dir_context *ctx)
 			}
 
 			mode = be16_to_cpu(entry.file.permissions.mode);
-			if (S_ISREG(mode))
-				type = DT_REG;
-			else if (S_ISLNK(mode))
-				type = DT_LNK;
-			else if (S_ISFIFO(mode))
-				type = DT_FIFO;
-			else if (S_ISCHR(mode))
-				type = DT_CHR;
-			else if (S_ISBLK(mode))
-				type = DT_BLK;
-			else if (S_ISSOCK(mode))
-				type = DT_SOCK;
 
 			if (!dir_emit(ctx, strbuf, len,
-				      be32_to_cpu(entry.file.id), type))
+				      be32_to_cpu(entry.file.id),
+				      fs_umode_to_dtype(mode)))
 				break;
 		} else {
 			pr_err("bad catalog entry type\n");
-- 
2.7.4

[RFC][PATCH 04/11] ext2: use common file type conversion

[Amir Goldstein]

deduplicate the ext2 file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ext2/dir.c  | 35 ++++++-----------------------------
 fs/ext2/ext2.h | 16 ----------------
 2 files changed, 6 insertions(+), 45 deletions(-)

diff --git a/fs/ext2/dir.c b/fs/ext2/dir.c
index d9650c9..efb32c9 100644
--- a/fs/ext2/dir.c
+++ b/fs/ext2/dir.c
@@ -250,33 +250,10 @@ ext2_validate_entry(char *base, unsigned offset, unsigned mask)
 	return (char *)p - base;
 }
 
-static unsigned char ext2_filetype_table[EXT2_FT_MAX] = {
-	[EXT2_FT_UNKNOWN]	= DT_UNKNOWN,
-	[EXT2_FT_REG_FILE]	= DT_REG,
-	[EXT2_FT_DIR]		= DT_DIR,
-	[EXT2_FT_CHRDEV]	= DT_CHR,
-	[EXT2_FT_BLKDEV]	= DT_BLK,
-	[EXT2_FT_FIFO]		= DT_FIFO,
-	[EXT2_FT_SOCK]		= DT_SOCK,
-	[EXT2_FT_SYMLINK]	= DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= EXT2_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= EXT2_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= EXT2_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= EXT2_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= EXT2_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= EXT2_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= EXT2_FT_SYMLINK,
-};
-
 static inline void ext2_set_de_type(ext2_dirent *de, struct inode *inode)
 {
-	umode_t mode = inode->i_mode;
 	if (EXT2_HAS_INCOMPAT_FEATURE(inode->i_sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
-		de->file_type = ext2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+		de->file_type = fs_umode_to_ftype(inode->i_mode);
 	else
 		de->file_type = 0;
 }
@@ -291,14 +268,14 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
 	unsigned long n = pos >> PAGE_SHIFT;
 	unsigned long npages = dir_pages(inode);
 	unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
-	unsigned char *types = NULL;
 	int need_revalidate = file->f_version != inode->i_version;
+	bool has_filetype;
 
 	if (pos > inode->i_size - EXT2_DIR_REC_LEN(1))
 		return 0;
 
-	if (EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE))
-		types = ext2_filetype_table;
+	has_filetype =
+		EXT2_HAS_INCOMPAT_FEATURE(sb, EXT2_FEATURE_INCOMPAT_FILETYPE);
 
 	for ( ; n < npages; n++, offset = 0) {
 		char *kaddr, *limit;
@@ -333,8 +310,8 @@ ext2_readdir(struct file *file, struct dir_context *ctx)
 			if (de->inode) {
 				unsigned char d_type = DT_UNKNOWN;
 
-				if (types && de->file_type < EXT2_FT_MAX)
-					d_type = types[de->file_type];
+				if (has_filetype)
+					d_type = fs_dtype(de->file_type);
 
 				if (!dir_emit(ctx, de->name, de->name_len,
 						le32_to_cpu(de->inode),
diff --git a/fs/ext2/ext2.h b/fs/ext2/ext2.h
index 37e2be7..c710e36 100644
--- a/fs/ext2/ext2.h
+++ b/fs/ext2/ext2.h
@@ -606,22 +606,6 @@ struct ext2_dir_entry_2 {
 };
 
 /*
- * Ext2 directory file types.  Only the low 3 bits are used.  The
- * other bits are reserved for now.
- */
-enum {
-	EXT2_FT_UNKNOWN		= 0,
-	EXT2_FT_REG_FILE	= 1,
-	EXT2_FT_DIR		= 2,
-	EXT2_FT_CHRDEV		= 3,
-	EXT2_FT_BLKDEV		= 4,
-	EXT2_FT_FIFO		= 5,
-	EXT2_FT_SOCK		= 6,
-	EXT2_FT_SYMLINK		= 7,
-	EXT2_FT_MAX
-};
-
-/*
  * EXT2_DIR_PAD defines the directory entries boundaries
  *
  * NOTE: It must be a multiple of 4
-- 
2.7.4

[RFC][PATCH 05/11] exofs: use common file type conversion

[Amir Goldstein]

deduplicate the exofs file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/exofs/common.h | 12 ------------
 fs/exofs/dir.c    | 34 ++--------------------------------
 2 files changed, 2 insertions(+), 44 deletions(-)

diff --git a/fs/exofs/common.h b/fs/exofs/common.h
index 7d88ef5..917decc9 100644
--- a/fs/exofs/common.h
+++ b/fs/exofs/common.h
@@ -204,18 +204,6 @@ struct exofs_dir_entry {
 	char		name[EXOFS_NAME_LEN];	/* file name              */
 };
 
-enum {
-	EXOFS_FT_UNKNOWN,
-	EXOFS_FT_REG_FILE,
-	EXOFS_FT_DIR,
-	EXOFS_FT_CHRDEV,
-	EXOFS_FT_BLKDEV,
-	EXOFS_FT_FIFO,
-	EXOFS_FT_SOCK,
-	EXOFS_FT_SYMLINK,
-	EXOFS_FT_MAX
-};
-
 #define EXOFS_DIR_PAD			4
 #define EXOFS_DIR_ROUND			(EXOFS_DIR_PAD - 1)
 #define EXOFS_DIR_REC_LEN(name_len) \
diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
index 42f9a0a..55fd31c 100644
--- a/fs/exofs/dir.c
+++ b/fs/exofs/dir.c
@@ -203,33 +203,10 @@ exofs_validate_entry(char *base, unsigned offset, unsigned mask)
 	return (char *)p - base;
 }
 
-static unsigned char exofs_filetype_table[EXOFS_FT_MAX] = {
-	[EXOFS_FT_UNKNOWN]	= DT_UNKNOWN,
-	[EXOFS_FT_REG_FILE]	= DT_REG,
-	[EXOFS_FT_DIR]		= DT_DIR,
-	[EXOFS_FT_CHRDEV]	= DT_CHR,
-	[EXOFS_FT_BLKDEV]	= DT_BLK,
-	[EXOFS_FT_FIFO]		= DT_FIFO,
-	[EXOFS_FT_SOCK]		= DT_SOCK,
-	[EXOFS_FT_SYMLINK]	= DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char exofs_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= EXOFS_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= EXOFS_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= EXOFS_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= EXOFS_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= EXOFS_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= EXOFS_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= EXOFS_FT_SYMLINK,
-};
-
 static inline
 void exofs_set_de_type(struct exofs_dir_entry *de, struct inode *inode)
 {
-	umode_t mode = inode->i_mode;
-	de->file_type = exofs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+	de->file_type = fs_umode_to_ftype(inode->i_mode);
 }
 
 static int
@@ -279,16 +256,9 @@ exofs_readdir(struct file *file, struct dir_context *ctx)
 				return -EIO;
 			}
 			if (de->inode_no) {
-				unsigned char t;
-
-				if (de->file_type < EXOFS_FT_MAX)
-					t = exofs_filetype_table[de->file_type];
-				else
-					t = DT_UNKNOWN;
-
 				if (!dir_emit(ctx, de->name, de->name_len,
 						le64_to_cpu(de->inode_no),
-						t)) {
+						fs_dtype(de->file_type))) {
 					exofs_put_page(page);
 					return 0;
 				}
-- 
2.7.4

[RFC][PATCH 06/11] ext4: use common file type conversion

[Amir Goldstein]

deduplicate the ext4 file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ext4/ext4.h | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 2163c1e..b15b8d4 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1932,17 +1932,18 @@ struct ext4_dir_entry_tail {
 /*
  * Ext4 directory file types.  Only the low 3 bits are used.  The
  * other bits are reserved for now.
+ * Values are taken from common file type implementation.
  */
-#define EXT4_FT_UNKNOWN		0
-#define EXT4_FT_REG_FILE	1
-#define EXT4_FT_DIR		2
-#define EXT4_FT_CHRDEV		3
-#define EXT4_FT_BLKDEV		4
-#define EXT4_FT_FIFO		5
-#define EXT4_FT_SOCK		6
-#define EXT4_FT_SYMLINK		7
+#define EXT4_FT_UNKNOWN		FT_UNKNOWN
+#define EXT4_FT_REG_FILE	FT_REG_FILE
+#define EXT4_FT_DIR		FT_DIR
+#define EXT4_FT_CHRDEV		FT_CHRDEV
+#define EXT4_FT_BLKDEV		FT_BLKDEV
+#define EXT4_FT_FIFO		FT_FIFO
+#define EXT4_FT_SOCK		FT_SOCK
+#define EXT4_FT_SYMLINK		FT_SYMLINK
 
-#define EXT4_FT_MAX		8
+#define EXT4_FT_MAX		FT_MAX
 
 #define EXT4_FT_DIR_CSUM	0xDE
 
@@ -2373,16 +2374,13 @@ static inline void ext4_update_dx_flag(struct inode *inode)
 	if (!ext4_has_feature_dir_index(inode->i_sb))
 		ext4_clear_inode_flag(inode, EXT4_INODE_INDEX);
 }
-static unsigned char ext4_filetype_table[] = {
-	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
-};
 
 static inline  unsigned char get_dtype(struct super_block *sb, int filetype)
 {
-	if (!ext4_has_feature_filetype(sb) || filetype >= EXT4_FT_MAX)
+	if (!ext4_has_feature_filetype(sb))
 		return DT_UNKNOWN;
 
-	return ext4_filetype_table[filetype];
+	return fs_dtype(filetype);
 }
 extern int ext4_check_all_de(struct inode *dir, struct buffer_head *bh,
 			     void *buf, int buf_size);
@@ -3057,22 +3055,12 @@ extern void initialize_dirent_tail(struct ext4_dir_entry_tail *t,
 extern int ext4_handle_dirty_dirent_node(handle_t *handle,
 					 struct inode *inode,
 					 struct buffer_head *bh);
-#define S_SHIFT 12
-static unsigned char ext4_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= EXT4_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= EXT4_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= EXT4_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= EXT4_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= EXT4_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= EXT4_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= EXT4_FT_SYMLINK,
-};
 
 static inline void ext4_set_de_type(struct super_block *sb,
 				struct ext4_dir_entry_2 *de,
 				umode_t mode) {
 	if (ext4_has_feature_filetype(sb))
-		de->file_type = ext4_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+		de->file_type = fs_umode_to_ftype(mode);
 }
 
 /* readpages.c */
-- 
2.7.4

[RFC][PATCH 07/11] ocfs2: use common file type conversion

[Amir Goldstein]

deduplicate the ocfs2 file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ocfs2/dir.c      | 18 +++---------------
 fs/ocfs2/ocfs2_fs.h | 32 +++++++++++---------------------
 2 files changed, 14 insertions(+), 36 deletions(-)

diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index 3ecb9f3..9321459 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -68,10 +68,6 @@
 #define NAMEI_RA_BLOCKS  4
 #define NAMEI_RA_SIZE        (NAMEI_RA_CHUNKS * NAMEI_RA_BLOCKS)
 
-static unsigned char ocfs2_filetype_table[] = {
-	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK
-};
-
 static int ocfs2_do_extend_dir(struct super_block *sb,
 			       handle_t *handle,
 			       struct inode *dir,
@@ -1802,13 +1798,9 @@ static int ocfs2_dir_foreach_blk_id(struct inode *inode,
 		}
 		offset += le16_to_cpu(de->rec_len);
 		if (le64_to_cpu(de->inode)) {
-			unsigned char d_type = DT_UNKNOWN;
-
-			if (de->file_type < OCFS2_FT_MAX)
-				d_type = ocfs2_filetype_table[de->file_type];
-
 			if (!dir_emit(ctx, de->name, de->name_len,
-				      le64_to_cpu(de->inode), d_type))
+				      le64_to_cpu(de->inode),
+				      fs_dtype(de->file_type)))
 				goto out;
 		}
 		ctx->pos += le16_to_cpu(de->rec_len);
@@ -1900,14 +1892,10 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode,
 				continue;
 			}
 			if (le64_to_cpu(de->inode)) {
-				unsigned char d_type = DT_UNKNOWN;
-
-				if (de->file_type < OCFS2_FT_MAX)
-					d_type = ocfs2_filetype_table[de->file_type];
 				if (!dir_emit(ctx, de->name,
 						de->name_len,
 						le64_to_cpu(de->inode),
-						d_type)) {
+						fs_dtype(de->file_type))) {
 					brelse(bh);
 					return 0;
 				}
diff --git a/fs/ocfs2/ocfs2_fs.h b/fs/ocfs2/ocfs2_fs.h
index 44d178b..742b7ab 100644
--- a/fs/ocfs2/ocfs2_fs.h
+++ b/fs/ocfs2/ocfs2_fs.h
@@ -395,17 +395,18 @@ static struct ocfs2_system_inode_info ocfs2_system_inodes[NUM_SYSTEM_INODES] = {
 /*
  * OCFS2 directory file types.  Only the low 3 bits are used.  The
  * other bits are reserved for now.
+ * Values are taken from common file type implementation.
  */
-#define OCFS2_FT_UNKNOWN	0
-#define OCFS2_FT_REG_FILE	1
-#define OCFS2_FT_DIR		2
-#define OCFS2_FT_CHRDEV		3
-#define OCFS2_FT_BLKDEV		4
-#define OCFS2_FT_FIFO		5
-#define OCFS2_FT_SOCK		6
-#define OCFS2_FT_SYMLINK	7
+#define OCFS2_FT_UNKNOWN	FT_UNKNOWN
+#define OCFS2_FT_REG_FILE	FT_REG_FILE
+#define OCFS2_FT_DIR		FT_DIR
+#define OCFS2_FT_CHRDEV		FT_CHRDEV
+#define OCFS2_FT_BLKDEV		FT_BLKDEV
+#define OCFS2_FT_FIFO		FT_FIFO
+#define OCFS2_FT_SOCK		FT_SOCK
+#define OCFS2_FT_SYMLINK	FT_SYMLINK
 
-#define OCFS2_FT_MAX		8
+#define OCFS2_FT_MAX		FT_MAX
 
 /*
  * OCFS2_DIR_PAD defines the directory entries boundaries
@@ -425,17 +426,6 @@ static struct ocfs2_system_inode_info ocfs2_system_inodes[NUM_SYSTEM_INODES] = {
 #define	OCFS2_LINKS_HI_SHIFT	16
 #define	OCFS2_DX_ENTRIES_MAX	(0xffffffffU)
 
-#define S_SHIFT			12
-static unsigned char ocfs2_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]  = OCFS2_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]  = OCFS2_FT_DIR,
-	[S_IFCHR >> S_SHIFT]  = OCFS2_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]  = OCFS2_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]  = OCFS2_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT] = OCFS2_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]  = OCFS2_FT_SYMLINK,
-};
-
 
 /*
  * Convenience casts
@@ -1630,7 +1620,7 @@ static inline int ocfs2_sprintf_system_inode_name(char *buf, int len,
 static inline void ocfs2_set_de_type(struct ocfs2_dir_entry *de,
 				    umode_t mode)
 {
-	de->file_type = ocfs2_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+	de->file_type = fs_umode_to_ftype(mode);
 }
 
 static inline int ocfs2_gd_is_discontig(struct ocfs2_group_desc *gd)
-- 
2.7.4

[RFC][PATCH 08/11] f2fs: use common file type conversion

[Amir Goldstein]

deduplicate the f2fs file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/f2fs/dir.c           | 27 ++-------------------------
 fs/f2fs/inline.c        |  2 +-
 include/linux/f2fs_fs.h |  8 +++++---
 3 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 827c5da..31bbad1 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -37,37 +37,14 @@ static unsigned int bucket_blocks(unsigned int level)
 		return 4;
 }
 
-static unsigned char f2fs_filetype_table[F2FS_FT_MAX] = {
-	[F2FS_FT_UNKNOWN]	= DT_UNKNOWN,
-	[F2FS_FT_REG_FILE]	= DT_REG,
-	[F2FS_FT_DIR]		= DT_DIR,
-	[F2FS_FT_CHRDEV]	= DT_CHR,
-	[F2FS_FT_BLKDEV]	= DT_BLK,
-	[F2FS_FT_FIFO]		= DT_FIFO,
-	[F2FS_FT_SOCK]		= DT_SOCK,
-	[F2FS_FT_SYMLINK]	= DT_LNK,
-};
-
-static unsigned char f2fs_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= F2FS_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= F2FS_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= F2FS_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= F2FS_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= F2FS_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= F2FS_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= F2FS_FT_SYMLINK,
-};
-
 void set_de_type(struct f2fs_dir_entry *de, umode_t mode)
 {
-	de->file_type = f2fs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
+	de->file_type = fs_umode_to_ftype(mode);
 }
 
 unsigned char get_de_type(struct f2fs_dir_entry *de)
 {
-	if (de->file_type < F2FS_FT_MAX)
-		return f2fs_filetype_table[de->file_type];
-	return DT_UNKNOWN;
+	return fs_dtype(de->file_type);
 }
 
 static unsigned long dir_block_index(unsigned int level,
diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
index e32a9e5..53148d6 100644
--- a/fs/f2fs/inline.c
+++ b/fs/f2fs/inline.c
@@ -424,7 +424,7 @@ static int f2fs_add_inline_entries(struct inode *dir,
 		new_name.len = le16_to_cpu(de->name_len);
 
 		ino = le32_to_cpu(de->ino);
-		fake_mode = get_de_type(de) << S_SHIFT;
+		fake_mode = get_de_type(de) << S_DT_SHIFT;
 
 		err = f2fs_add_regular_entry(dir, &new_name, NULL, NULL,
 							ino, fake_mode);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index cea41a1..3bfa4e2 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -504,7 +504,11 @@ struct f2fs_inline_dentry {
 	__u8 filename[NR_INLINE_DENTRY][F2FS_SLOT_LEN];
 } __packed;
 
-/* file types used in inode_info->flags */
+/*
+ * file types used in inode_info->flags
+ *
+ * Values should match common file type values in file_type.h.
+ */
 enum {
 	F2FS_FT_UNKNOWN,
 	F2FS_FT_REG_FILE,
@@ -517,6 +521,4 @@ enum {
 	F2FS_FT_MAX
 };
 
-#define S_SHIFT 12
-
 #endif  /* _LINUX_F2FS_FS_H */
-- 
2.7.4

[RFC][PATCH 09/11] nilfs2: use common file type conversion

[Amir Goldstein]

deduplicate the nilfs2 file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nilfs2/dir.c                    | 38 +++-----------------------------------
 include/uapi/linux/nilfs2_ondisk.h |  1 +
 2 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index 582831a..c4749db 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -238,35 +238,9 @@ static struct nilfs_dir_entry *nilfs_next_entry(struct nilfs_dir_entry *p)
 					  nilfs_rec_len_from_disk(p->rec_len));
 }
 
-static unsigned char
-nilfs_filetype_table[NILFS_FT_MAX] = {
-	[NILFS_FT_UNKNOWN]	= DT_UNKNOWN,
-	[NILFS_FT_REG_FILE]	= DT_REG,
-	[NILFS_FT_DIR]		= DT_DIR,
-	[NILFS_FT_CHRDEV]	= DT_CHR,
-	[NILFS_FT_BLKDEV]	= DT_BLK,
-	[NILFS_FT_FIFO]		= DT_FIFO,
-	[NILFS_FT_SOCK]		= DT_SOCK,
-	[NILFS_FT_SYMLINK]	= DT_LNK,
-};
-
-#define S_SHIFT 12
-static unsigned char
-nilfs_type_by_mode[S_IFMT >> S_SHIFT] = {
-	[S_IFREG >> S_SHIFT]	= NILFS_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]	= NILFS_FT_DIR,
-	[S_IFCHR >> S_SHIFT]	= NILFS_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]	= NILFS_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]	= NILFS_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]	= NILFS_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]	= NILFS_FT_SYMLINK,
-};
-
 static void nilfs_set_de_type(struct nilfs_dir_entry *de, struct inode *inode)
 {
-	umode_t mode = inode->i_mode;
-
-	de->file_type = nilfs_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
+	de->file_type = fs_umode_to_ftype(inode->i_mode);
 }
 
 static int nilfs_readdir(struct file *file, struct dir_context *ctx)
@@ -302,15 +276,9 @@ static int nilfs_readdir(struct file *file, struct dir_context *ctx)
 				return -EIO;
 			}
 			if (de->inode) {
-				unsigned char t;
-
-				if (de->file_type < NILFS_FT_MAX)
-					t = nilfs_filetype_table[de->file_type];
-				else
-					t = DT_UNKNOWN;
-
 				if (!dir_emit(ctx, de->name, de->name_len,
-						le64_to_cpu(de->inode), t)) {
+						le64_to_cpu(de->inode),
+						fs_dtype(de->file_type))) {
 					nilfs_put_page(page);
 					return 0;
 				}
diff --git a/include/uapi/linux/nilfs2_ondisk.h b/include/uapi/linux/nilfs2_ondisk.h
index 2a8a3ad..ab10aae 100644
--- a/include/uapi/linux/nilfs2_ondisk.h
+++ b/include/uapi/linux/nilfs2_ondisk.h
@@ -308,6 +308,7 @@ struct nilfs_dir_entry {
 /*
  * NILFS directory file types.  Only the low 3 bits are used.  The
  * other bits are reserved for now.
+ * Values should match common file type values in file_type.h.
  */
 enum {
 	NILFS_FT_UNKNOWN,
-- 
2.7.4

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

[Amir Goldstein]

deduplicate the btrfs file type conversion implementation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/btrfs/btrfs_inode.h          |  2 --
 fs/btrfs/delayed-inode.c        |  2 +-
 fs/btrfs/inode.c                | 19 ++-----------------
 include/uapi/linux/btrfs_tree.h |  2 ++
 4 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 1a8fa46..1230e5b 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -198,8 +198,6 @@ struct btrfs_inode {
 	struct inode vfs_inode;
 };
 
-extern unsigned char btrfs_filetype_table[];
-
 static inline struct btrfs_inode *BTRFS_I(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 80982a8..50260f0 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1717,7 +1717,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_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 f2b281a..45acb12 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -89,17 +89,6 @@ struct kmem_cache *btrfs_transaction_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);
 static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent);
@@ -5814,10 +5803,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
-};
-
 static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 {
 	struct inode *inode = file_inode(file);
@@ -5905,7 +5890,7 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx)
 		read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1),
 				   name_len);
 
-		d_type = btrfs_filetype_table[btrfs_dir_type(leaf, di)];
+		d_type = fs_dtype(btrfs_dir_type(leaf, di));
 		btrfs_dir_item_key_to_cpu(leaf, di, &location);
 
 		over = !dir_emit(ctx, name_ptr, name_len, location.objectid,
@@ -6301,7 +6286,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 d5ad15a..1335ff1 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -303,6 +303,8 @@
  *
  * Used by:
  * struct btrfs_dir_item.type
+ *
+ * Values 0..7 should match common file type values in file_type.h.
  */
 #define BTRFS_FT_UNKNOWN	0
 #define BTRFS_FT_REG_FILE	1
-- 
2.7.4

[RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

deduplicate the xfs file type conversion implementation.

xfs readdir code may expose DT_WHT type to user if that
type was set on disk, but xfs code never set a file type
of WHT on disk.

If it is acceptable to expose to user DT_UNKNOWN in case
WHT type somehow got to disk, then xfs_dir3_filetype_table
could also be replaced with the common fs_dtype() helper.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_da_format.h |  5 +++--
 fs/xfs/libxfs/xfs_dir2.c      | 17 -----------------
 fs/xfs/libxfs/xfs_dir2.h      |  6 ------
 fs/xfs/xfs_iops.c             |  2 +-
 4 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 9a492a9..c66c26f 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -169,8 +169,9 @@ struct xfs_da3_icnode_hdr {
 
 /*
  * Dirents in version 3 directories have a file type field. Additions to this
- * list are an on-disk format change, requiring feature bits. Valid values
- * are as follows:
+ * list are an on-disk format change, requiring feature bits.
+ * Values 0..7 should match common file type values in file_type.h.
+ * Valid values are as follows:
  */
 #define XFS_DIR3_FT_UNKNOWN		0
 #define XFS_DIR3_FT_REG_FILE		1
diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..645a542 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -36,23 +36,6 @@
 struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
 
 /*
- * @mode, if set, indicates that the type field needs to be set up.
- * This uses the transformation from file mode to DT_* as defined in linux/fs.h
- * for file type specification. This will be propagated into the directory
- * structure if appropriate for the given operation and filesystem config.
- */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
-};
-
-/*
  * ASCII case-insensitive (ie. A-Z) support for directories that was
  * used in IRIX.
  */
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..f9b9b50 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -32,12 +32,6 @@ struct xfs_dir2_data_unused;
 extern struct xfs_name	xfs_name_dotdot;
 
 /*
- * directory filetype conversion tables.
- */
-#define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
-
-/*
  * directory operations vector for encode/decode routines
  */
 struct xfs_dir_ops {
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..c122827 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@ xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = fs_umode_to_ftype(mode);
 }
 
 STATIC void
-- 
2.7.4

Re: [RFC][PATCH 01/11] fs: common implementation of file type conversions

[Darrick J. Wong]

On Mon, Dec 19, 2016 at 10:10:58PM +0200, Amir Goldstein wrote:
> Many file systems use a copy&paste implementation
> of dirent to on-disk file type conversions.
> 
> Create a common implementation to be used by file systems
> with some useful conversion helpers to reduce open coded
> file type conversions in file system code.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/file_type.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h        |  17 +-------
>  2 files changed, 108 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/file_type.h
> 
> diff --git a/include/linux/file_type.h b/include/linux/file_type.h
> new file mode 100644
> index 0000000..03ee1a1
> --- /dev/null
> +++ b/include/linux/file_type.h
> @@ -0,0 +1,107 @@
> +#ifndef _LINUX_FILE_TYPE_H
> +#define _LINUX_FILE_TYPE_H
> +
> +/*
> + * This is a common implementation of dirent to fs on-disk
> + * file type conversion.  Although the fs on-disk bits are
> + * specific to every file system, in practice, many file systems
> + * use the exact same on-disk format to describe the lower 3
> + * file type bits that represent the 7 POSIX file types.
> + * All those file systems can use this generic code for the
> + * conversions:
> + *  i_mode -> fs on-disk file type (ftype)
> + *  fs on-disk file type (ftype) -> dirent file type (dtype)
> + *  i_mode -> dirent file type (dtype)
> + */
> +
> +/*
> + * struct dirent file types
> + * exposed to user via getdents(2), readdir(3)
> + *
> + * These match bits 12..15 of stat.st_mode
> + * (ie "(i_mode >> 12) & 15").
> + */
> +#define S_DT_SHIFT	12
> +#define S_DT(mode)	(((mode) & S_IFMT) >> S_DT_SHIFT)
> +#define DT_MASK		(S_IFMT >> S_DT_SHIFT)
> +
> +#define DT_UNKNOWN	0
> +#define DT_FIFO		S_DT(S_IFIFO) /* 1 */
> +#define DT_CHR		S_DT(S_IFCHR) /* 2 */
> +#define DT_DIR		S_DT(S_IFDIR) /* 4 */
> +#define DT_BLK		S_DT(S_IFBLK) /* 6 */
> +#define DT_REG		S_DT(S_IFREG) /* 8 */
> +#define DT_LNK		S_DT(S_IFLNK) /* 10 */
> +#define DT_SOCK		S_DT(S_IFSOCK) /* 12 */
> +#define DT_WHT		14
> +
> +#define DT_MAX		(DT_MASK + 1) /* 16 */
> +
> +/*
> + * fs on-disk file types.
> + * Only the low 3 bits are used for the POSIX file types.
> + * Other bits are reserved for fs private use.
> + *
> + * Note that no fs currently stores the whiteout type on-disk,
> + * so whiteout dirents are exposed to user as DT_CHR.
> + */
> +#define FT_UNKNOWN	0
> +#define FT_REG_FILE	1
> +#define FT_DIR		2
> +#define FT_CHRDEV	3
> +#define FT_BLKDEV	4
> +#define FT_FIFO		5
> +#define FT_SOCK		6
> +#define FT_SYMLINK	7
> +
> +#define FT_MAX		8
> +
> +/*
> + * fs on-disk file type to dirent file type conversion
> + */
> +static unsigned char fs_dtype_by_ftype[] = {
> +	DT_UNKNOWN,

[FT_UNKNOWN] = DT_UNKNOWN, etc?

It's not strictly necessary but it makes the mapping more explicit.

--D

> +	DT_REG,
> +	DT_DIR,
> +	DT_CHR,
> +	DT_BLK,
> +	DT_FIFO,
> +	DT_SOCK,
> +	DT_LNK
> +};
> +
> +static inline unsigned char fs_dtype(int filetype)
> +{
> +	if (filetype >= FT_MAX)
> +		return DT_UNKNOWN;
> +
> +	return fs_dtype_by_ftype[filetype];
> +}
> +
> +/*
> + * dirent file type to fs on-disk file type conversion
> + * Values not initialized explicitly are FT_UNKNOWN (0).
> + */
> +static unsigned char fs_ftype_by_dtype[DT_MAX] = {
> +	[DT_REG]	= FT_REG_FILE,
> +	[DT_DIR]	= FT_DIR,
> +	[DT_LNK]	= FT_SYMLINK,
> +	[DT_CHR]	= FT_CHRDEV,
> +	[DT_BLK]	= FT_BLKDEV,
> +	[DT_FIFO]	= FT_FIFO,
> +	[DT_SOCK]	= FT_SOCK,
> +};
> +
> +/* st_mode to fs on-disk file type conversion */
> +static inline unsigned char fs_umode_to_ftype(umode_t mode)
> +{
> +	return fs_ftype_by_dtype[S_DT(mode)];
> +}
> +
> +/* st_mode to dirent file type conversion */
> +static inline unsigned char fs_umode_to_dtype(umode_t mode)
> +{
> +	return fs_dtype(fs_umode_to_ftype(mode));
> +}
> +
> +#endif
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e6e4146..8f1580d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -31,6 +31,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/percpu-rwsem.h>
>  #include <linux/delayed_call.h>
> +#include <linux/file_type.h>
>  
>  #include <asm/byteorder.h>
>  #include <uapi/linux/fs.h>
> @@ -1582,22 +1583,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
>  int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>  
>  /*
> - * File types
> - *
> - * NOTE! These match bits 12..15 of stat.st_mode
> - * (ie "(i_mode >> 12) & 15").
> - */
> -#define DT_UNKNOWN	0
> -#define DT_FIFO		1
> -#define DT_CHR		2
> -#define DT_DIR		4
> -#define DT_BLK		6
> -#define DT_REG		8
> -#define DT_LNK		10
> -#define DT_SOCK		12
> -#define DT_WHT		14
> -
> -/*
>   * This is the "filldir" function type, used by readdir() to let
>   * the kernel specify what kind of dirent layout it wants to have.
>   * This allows the kernel to read directories into kernel space or
> -- 
> 2.7.4
> 

Re: [RFC][PATCH 05/11] exofs: use common file type conversion

[Boaz Harrosh]

On 12/19/2016 10:11 PM, Amir Goldstein wrote:
> deduplicate the exofs file type conversion implementation.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Yes! thank you cool.

ACK-by: Boaz Harrosh <ooo@electrozaur.com>

Al please take via your tree
> ---
>  fs/exofs/common.h | 12 ------------
>  fs/exofs/dir.c    | 34 ++--------------------------------
>  2 files changed, 2 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/exofs/common.h b/fs/exofs/common.h
> index 7d88ef5..917decc9 100644
> --- a/fs/exofs/common.h
> +++ b/fs/exofs/common.h
> @@ -204,18 +204,6 @@ struct exofs_dir_entry {
>  	char		name[EXOFS_NAME_LEN];	/* file name              */
>  };
>  
> -enum {
> -	EXOFS_FT_UNKNOWN,
> -	EXOFS_FT_REG_FILE,
> -	EXOFS_FT_DIR,
> -	EXOFS_FT_CHRDEV,
> -	EXOFS_FT_BLKDEV,
> -	EXOFS_FT_FIFO,
> -	EXOFS_FT_SOCK,
> -	EXOFS_FT_SYMLINK,
> -	EXOFS_FT_MAX
> -};
> -
>  #define EXOFS_DIR_PAD			4
>  #define EXOFS_DIR_ROUND			(EXOFS_DIR_PAD - 1)
>  #define EXOFS_DIR_REC_LEN(name_len) \
> diff --git a/fs/exofs/dir.c b/fs/exofs/dir.c
> index 42f9a0a..55fd31c 100644
> --- a/fs/exofs/dir.c
> +++ b/fs/exofs/dir.c
> @@ -203,33 +203,10 @@ exofs_validate_entry(char *base, unsigned offset, unsigned mask)
>  	return (char *)p - base;
>  }
>  
> -static unsigned char exofs_filetype_table[EXOFS_FT_MAX] = {
> -	[EXOFS_FT_UNKNOWN]	= DT_UNKNOWN,
> -	[EXOFS_FT_REG_FILE]	= DT_REG,
> -	[EXOFS_FT_DIR]		= DT_DIR,
> -	[EXOFS_FT_CHRDEV]	= DT_CHR,
> -	[EXOFS_FT_BLKDEV]	= DT_BLK,
> -	[EXOFS_FT_FIFO]		= DT_FIFO,
> -	[EXOFS_FT_SOCK]		= DT_SOCK,
> -	[EXOFS_FT_SYMLINK]	= DT_LNK,
> -};
> -
> -#define S_SHIFT 12
> -static unsigned char exofs_type_by_mode[S_IFMT >> S_SHIFT] = {
> -	[S_IFREG >> S_SHIFT]	= EXOFS_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]	= EXOFS_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]	= EXOFS_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]	= EXOFS_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]	= EXOFS_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]	= EXOFS_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]	= EXOFS_FT_SYMLINK,
> -};
> -
>  static inline
>  void exofs_set_de_type(struct exofs_dir_entry *de, struct inode *inode)
>  {
> -	umode_t mode = inode->i_mode;
> -	de->file_type = exofs_type_by_mode[(mode & S_IFMT) >> S_SHIFT];
> +	de->file_type = fs_umode_to_ftype(inode->i_mode);
>  }
>  
>  static int
> @@ -279,16 +256,9 @@ exofs_readdir(struct file *file, struct dir_context *ctx)
>  				return -EIO;
>  			}
>  			if (de->inode_no) {
> -				unsigned char t;
> -
> -				if (de->file_type < EXOFS_FT_MAX)
> -					t = exofs_filetype_table[de->file_type];
> -				else
> -					t = DT_UNKNOWN;
> -
>  				if (!dir_emit(ctx, de->name, de->name_len,
>  						le64_to_cpu(de->inode_no),
> -						t)) {
> +						fs_dtype(de->file_type))) {
>  					exofs_put_page(page);
>  					return 0;
>  				}
> 

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Darrick J. Wong]

On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
> deduplicate the xfs file type conversion implementation.
> 
> xfs readdir code may expose DT_WHT type to user if that
> type was set on disk, but xfs code never set a file type
> of WHT on disk.
> 
> If it is acceptable to expose to user DT_UNKNOWN in case
> WHT type somehow got to disk, then xfs_dir3_filetype_table
> could also be replaced with the common fs_dtype() helper.

AFAIK XFS has never actually written XFS_DIR3_FT_WHT to disk.
I see that overlayfs whiteouts are now some sort of weird
chardev with rdev == 0, so I guess overlayfs doesn't either...?

--D

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_da_format.h |  5 +++--
>  fs/xfs/libxfs/xfs_dir2.c      | 17 -----------------
>  fs/xfs/libxfs/xfs_dir2.h      |  6 ------
>  fs/xfs/xfs_iops.c             |  2 +-
>  4 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 9a492a9..c66c26f 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -169,8 +169,9 @@ struct xfs_da3_icnode_hdr {
>  
>  /*
>   * Dirents in version 3 directories have a file type field. Additions to this
> - * list are an on-disk format change, requiring feature bits. Valid values
> - * are as follows:
> + * list are an on-disk format change, requiring feature bits.
> + * Values 0..7 should match common file type values in file_type.h.
> + * Valid values are as follows:
>   */
>  #define XFS_DIR3_FT_UNKNOWN		0
>  #define XFS_DIR3_FT_REG_FILE		1
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index c58d72c..645a542 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -36,23 +36,6 @@
>  struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>  
>  /*
> - * @mode, if set, indicates that the type field needs to be set up.
> - * This uses the transformation from file mode to DT_* as defined in linux/fs.h
> - * for file type specification. This will be propagated into the directory
> - * structure if appropriate for the given operation and filesystem config.
> - */
> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
> -	[0]			= XFS_DIR3_FT_UNKNOWN,
> -	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
> -};
> -
> -/*
>   * ASCII case-insensitive (ie. A-Z) support for directories that was
>   * used in IRIX.
>   */
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 0197590..f9b9b50 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -32,12 +32,6 @@ struct xfs_dir2_data_unused;
>  extern struct xfs_name	xfs_name_dotdot;
>  
>  /*
> - * directory filetype conversion tables.
> - */
> -#define S_SHIFT 12
> -extern const unsigned char xfs_mode_to_ftype[];
> -
> -/*
>   * directory operations vector for encode/decode routines
>   */
>  struct xfs_dir_ops {
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 308bebb..c122827 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>  {
>  	namep->name = dentry->d_name.name;
>  	namep->len = dentry->d_name.len;
> -	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
> +	namep->type = fs_umode_to_ftype(mode);
>  }
>  
>  STATIC void
> -- 
> 2.7.4
> 

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Darrick J. Wong]

On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
> deduplicate the xfs file type conversion implementation.
> 
> xfs readdir code may expose DT_WHT type to user if that
> type was set on disk, but xfs code never set a file type
> of WHT on disk.
> 
> If it is acceptable to expose to user DT_UNKNOWN in case
> WHT type somehow got to disk, then xfs_dir3_filetype_table
> could also be replaced with the common fs_dtype() helper.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_da_format.h |  5 +++--
>  fs/xfs/libxfs/xfs_dir2.c      | 17 -----------------
>  fs/xfs/libxfs/xfs_dir2.h      |  6 ------
>  fs/xfs/xfs_iops.c             |  2 +-
>  4 files changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 9a492a9..c66c26f 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -169,8 +169,9 @@ struct xfs_da3_icnode_hdr {
>  
>  /*
>   * Dirents in version 3 directories have a file type field. Additions to this
> - * list are an on-disk format change, requiring feature bits. Valid values
> - * are as follows:
> + * list are an on-disk format change, requiring feature bits.
> + * Values 0..7 should match common file type values in file_type.h.
> + * Valid values are as follows:

They have to stay in sync... but there's nothing here to enforce this.

Originally XFS takes the mode value passed in by the VFS and converts
that to an XFS_DIR3_FT_* equivalent, which is passed around internally
before being written to disk.  On the way up (readdir), the _DIR3_FT_
values are converted to their DT_* equivalents before being passed back
to the other VFS directory iterator functions.

With this patch applied, XFS no longer gets to write its own ftype
values to disk -- all that is now opaque in the VFS.  Effectively, we
lose control of that part of our own disk format.  You can subtly change
the range of fs_umode_to_ftype() and that'll get written to disk.

Normally we evaluate creating new feature flags when the on-disk format
changes, to try to minimize user problems.  These problems could arise
in xfs_dir3_get_dtype() which still relies on converting XFS_DIR3_FT_
values in to DT_ values, or with old versions of xfsprogs getting very
confused when it encounters a new value.

The proposed FT_* space also seems inflexible to the VFS -- the upper
5 bits are reserved for private use and the lower 3 bits are all in use,
which means that the VFS cannot later add more type codes bits unless
we can find bits that *nobody* else is using.  (The same theoretically
applies in reverse to XFS but as we don't have any private type codes,
it's not an issue yet.)

Furthermore, xfsprogs uses (roughly) the same libxfs code as the kernel.
Any code hoisted out of the kernel libxfs into the VFS must still be
provided for in xfsprogs so that we can build new xfsprogs on old kernel
headers.  If the hoist is into linux/fs.h, as is the case here, then
xfsprogs must retain a private version of the definitions, grow a bunch
of m4/autoconf macros to check for the presence of the linux/fs.h
versions, and wire up the private versions if linux/fs.h doesn't provide
one.  We also have to make sure that anything added to the VFS version
gets added to the xfsprogs version.

Note: We did this all this work a short time ago so that ext4 and XFS
could present the same FSGETXATTR ioctl to user programs (major benefit)
but it was kind of a pain to get right.  I don't see an upside to ceding
control of this part of the disk format to the VFS.

--D

>   */
>  #define XFS_DIR3_FT_UNKNOWN		0
>  #define XFS_DIR3_FT_REG_FILE		1
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index c58d72c..645a542 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -36,23 +36,6 @@
>  struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>  
>  /*
> - * @mode, if set, indicates that the type field needs to be set up.
> - * This uses the transformation from file mode to DT_* as defined in linux/fs.h
> - * for file type specification. This will be propagated into the directory
> - * structure if appropriate for the given operation and filesystem config.
> - */
> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
> -	[0]			= XFS_DIR3_FT_UNKNOWN,
> -	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
> -};
> -
> -/*
>   * ASCII case-insensitive (ie. A-Z) support for directories that was
>   * used in IRIX.
>   */
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 0197590..f9b9b50 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -32,12 +32,6 @@ struct xfs_dir2_data_unused;
>  extern struct xfs_name	xfs_name_dotdot;
>  
>  /*
> - * directory filetype conversion tables.
> - */
> -#define S_SHIFT 12
> -extern const unsigned char xfs_mode_to_ftype[];
> -
> -/*
>   * directory operations vector for encode/decode routines
>   */
>  struct xfs_dir_ops {
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 308bebb..c122827 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>  {
>  	namep->name = dentry->d_name.name;
>  	namep->len = dentry->d_name.len;
> -	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
> +	namep->type = fs_umode_to_ftype(mode);
>  }
>  
>  STATIC void
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC][PATCH 01/11] fs: common implementation of file type conversions

[Amir Goldstein]

On Mon, Dec 19, 2016 at 11:13 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Dec 19, 2016 at 10:10:58PM +0200, Amir Goldstein wrote:
>> Many file systems use a copy&paste implementation
>> of dirent to on-disk file type conversions.
>>
>> Create a common implementation to be used by file systems
>> with some useful conversion helpers to reduce open coded
>> file type conversions in file system code.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  include/linux/file_type.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/fs.h        |  17 +-------
>>  2 files changed, 108 insertions(+), 16 deletions(-)
>>  create mode 100644 include/linux/file_type.h
>>
>> diff --git a/include/linux/file_type.h b/include/linux/file_type.h
>> new file mode 100644
>> index 0000000..03ee1a1
>> --- /dev/null
>> +++ b/include/linux/file_type.h
>> @@ -0,0 +1,107 @@
>> +#ifndef _LINUX_FILE_TYPE_H
>> +#define _LINUX_FILE_TYPE_H
>> +
>> +/*
>> + * This is a common implementation of dirent to fs on-disk
>> + * file type conversion.  Although the fs on-disk bits are
>> + * specific to every file system, in practice, many file systems
>> + * use the exact same on-disk format to describe the lower 3
>> + * file type bits that represent the 7 POSIX file types.
>> + * All those file systems can use this generic code for the
>> + * conversions:
>> + *  i_mode -> fs on-disk file type (ftype)
>> + *  fs on-disk file type (ftype) -> dirent file type (dtype)
>> + *  i_mode -> dirent file type (dtype)
>> + */
>> +
>> +/*
>> + * struct dirent file types
>> + * exposed to user via getdents(2), readdir(3)
>> + *
>> + * These match bits 12..15 of stat.st_mode
>> + * (ie "(i_mode >> 12) & 15").
>> + */
>> +#define S_DT_SHIFT   12
>> +#define S_DT(mode)   (((mode) & S_IFMT) >> S_DT_SHIFT)
>> +#define DT_MASK              (S_IFMT >> S_DT_SHIFT)
>> +
>> +#define DT_UNKNOWN   0
>> +#define DT_FIFO              S_DT(S_IFIFO) /* 1 */
>> +#define DT_CHR               S_DT(S_IFCHR) /* 2 */
>> +#define DT_DIR               S_DT(S_IFDIR) /* 4 */
>> +#define DT_BLK               S_DT(S_IFBLK) /* 6 */
>> +#define DT_REG               S_DT(S_IFREG) /* 8 */
>> +#define DT_LNK               S_DT(S_IFLNK) /* 10 */
>> +#define DT_SOCK              S_DT(S_IFSOCK) /* 12 */
>> +#define DT_WHT               14
>> +
>> +#define DT_MAX               (DT_MASK + 1) /* 16 */
>> +
>> +/*
>> + * fs on-disk file types.
>> + * Only the low 3 bits are used for the POSIX file types.
>> + * Other bits are reserved for fs private use.
>> + *
>> + * Note that no fs currently stores the whiteout type on-disk,
>> + * so whiteout dirents are exposed to user as DT_CHR.
>> + */
>> +#define FT_UNKNOWN   0
>> +#define FT_REG_FILE  1
>> +#define FT_DIR               2
>> +#define FT_CHRDEV    3
>> +#define FT_BLKDEV    4
>> +#define FT_FIFO              5
>> +#define FT_SOCK              6
>> +#define FT_SYMLINK   7
>> +
>> +#define FT_MAX               8
>> +
>> +/*
>> + * fs on-disk file type to dirent file type conversion
>> + */
>> +static unsigned char fs_dtype_by_ftype[] = {
>> +     DT_UNKNOWN,
>
> [FT_UNKNOWN] = DT_UNKNOWN, etc?
>

sure.
> It's not strictly necessary but it makes the mapping more explicit.
>
> --D
>
>> +     DT_REG,
>> +     DT_DIR,
>> +     DT_CHR,
>> +     DT_BLK,
>> +     DT_FIFO,
>> +     DT_SOCK,
>> +     DT_LNK
>> +};
>> +
>> +static inline unsigned char fs_dtype(int filetype)
>> +{
>> +     if (filetype >= FT_MAX)
>> +             return DT_UNKNOWN;
>> +
>> +     return fs_dtype_by_ftype[filetype];
>> +}
>> +
>> +/*
>> + * dirent file type to fs on-disk file type conversion
>> + * Values not initialized explicitly are FT_UNKNOWN (0).
>> + */
>> +static unsigned char fs_ftype_by_dtype[DT_MAX] = {
>> +     [DT_REG]        = FT_REG_FILE,
>> +     [DT_DIR]        = FT_DIR,
>> +     [DT_LNK]        = FT_SYMLINK,
>> +     [DT_CHR]        = FT_CHRDEV,
>> +     [DT_BLK]        = FT_BLKDEV,
>> +     [DT_FIFO]       = FT_FIFO,
>> +     [DT_SOCK]       = FT_SOCK,
>> +};
>> +
>> +/* st_mode to fs on-disk file type conversion */
>> +static inline unsigned char fs_umode_to_ftype(umode_t mode)
>> +{
>> +     return fs_ftype_by_dtype[S_DT(mode)];
>> +}
>> +
>> +/* st_mode to dirent file type conversion */
>> +static inline unsigned char fs_umode_to_dtype(umode_t mode)
>> +{
>> +     return fs_dtype(fs_umode_to_ftype(mode));
>> +}
>> +
>> +#endif
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e6e4146..8f1580d 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -31,6 +31,7 @@
>>  #include <linux/workqueue.h>
>>  #include <linux/percpu-rwsem.h>
>>  #include <linux/delayed_call.h>
>> +#include <linux/file_type.h>
>>
>>  #include <asm/byteorder.h>
>>  #include <uapi/linux/fs.h>
>> @@ -1582,22 +1583,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
>>  int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
>>
>>  /*
>> - * File types
>> - *
>> - * NOTE! These match bits 12..15 of stat.st_mode
>> - * (ie "(i_mode >> 12) & 15").
>> - */
>> -#define DT_UNKNOWN   0
>> -#define DT_FIFO              1
>> -#define DT_CHR               2
>> -#define DT_DIR               4
>> -#define DT_BLK               6
>> -#define DT_REG               8
>> -#define DT_LNK               10
>> -#define DT_SOCK              12
>> -#define DT_WHT               14
>> -
>> -/*
>>   * This is the "filldir" function type, used by readdir() to let
>>   * the kernel specify what kind of dirent layout it wants to have.
>>   * This allows the kernel to read directories into kernel space or
>> --
>> 2.7.4
>>

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
>> deduplicate the xfs file type conversion implementation.
>>
>> xfs readdir code may expose DT_WHT type to user if that
>> type was set on disk, but xfs code never set a file type
>> of WHT on disk.
>>
>> If it is acceptable to expose to user DT_UNKNOWN in case
>> WHT type somehow got to disk, then xfs_dir3_filetype_table
>> could also be replaced with the common fs_dtype() helper.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/xfs/libxfs/xfs_da_format.h |  5 +++--
>>  fs/xfs/libxfs/xfs_dir2.c      | 17 -----------------
>>  fs/xfs/libxfs/xfs_dir2.h      |  6 ------
>>  fs/xfs/xfs_iops.c             |  2 +-
>>  4 files changed, 4 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
>> index 9a492a9..c66c26f 100644
>> --- a/fs/xfs/libxfs/xfs_da_format.h
>> +++ b/fs/xfs/libxfs/xfs_da_format.h
>> @@ -169,8 +169,9 @@ struct xfs_da3_icnode_hdr {
>>
>>  /*
>>   * Dirents in version 3 directories have a file type field. Additions to this
>> - * list are an on-disk format change, requiring feature bits. Valid values
>> - * are as follows:
>> + * list are an on-disk format change, requiring feature bits.
>> + * Values 0..7 should match common file type values in file_type.h.
>> + * Valid values are as follows:
>
> They have to stay in sync... but there's nothing here to enforce this.
>
> Originally XFS takes the mode value passed in by the VFS and converts
> that to an XFS_DIR3_FT_* equivalent, which is passed around internally
> before being written to disk.  On the way up (readdir), the _DIR3_FT_
> values are converted to their DT_* equivalents before being passed back
> to the other VFS directory iterator functions.
>
> With this patch applied, XFS no longer gets to write its own ftype
> values to disk -- all that is now opaque in the VFS.  Effectively, we
> lose control of that part of our own disk format.  You can subtly change
> the range of fs_umode_to_ftype() and that'll get written to disk.
>

No way that the common conversion code will change those values
too many fs depend on it. that should be made clear.

> Normally we evaluate creating new feature flags when the on-disk format
> changes, to try to minimize user problems.  These problems could arise
> in xfs_dir3_get_dtype() which still relies on converting XFS_DIR3_FT_
> values in to DT_ values, or with old versions of xfsprogs getting very
> confused when it encounters a new value.
>
> The proposed FT_* space also seems inflexible to the VFS -- the upper
> 5 bits are reserved for private use and the lower 3 bits are all in use,
> which means that the VFS cannot later add more type codes bits unless
> we can find bits that *nobody* else is using.  (The same theoretically
> applies in reverse to XFS but as we don't have any private type codes,
> it's not an issue yet.)
>

btrfs has a private flag on bit4 (FT_XTTR) and this implementation
does not prevent btrfs from using the private bit.
If XFS ever wants to write a private bit it will have to not blindly
translate mode to ftype.
The scope of the common implementation should remain only
the 3bits that translate directly to POSIX types.

> Furthermore, xfsprogs uses (roughly) the same libxfs code as the kernel.
> Any code hoisted out of the kernel libxfs into the VFS must still be
> provided for in xfsprogs so that we can build new xfsprogs on old kernel
> headers.  If the hoist is into linux/fs.h, as is the case here, then
> xfsprogs must retain a private version of the definitions, grow a bunch
> of m4/autoconf macros to check for the presence of the linux/fs.h
> versions, and wire up the private versions if linux/fs.h doesn't provide
> one.  We also have to make sure that anything added to the VFS version
> gets added to the xfsprogs version.
>

As a matter of fact, I would be very much interested to standardize
use the upper 5 bits some can remain private and some can be used
for generic fs purpose. But doing that will require shring a common
library (or at least a spec) between fs offline tools, so that's a
bigger challenge.

> Note: We did this all this work a short time ago so that ext4 and XFS
> could present the same FSGETXATTR ioctl to user programs (major benefit)
> but it was kind of a pain to get right.  I don't see an upside to ceding
> control of this part of the disk format to the VFS.
>

The answer "this is not wanted for XFS" is perfectly valid :)
The common implementation can be used by the small fs, which never
want anymore than the basic ext2 implementation.

However, since the DT_ values and  XFS_DIR3_FT_ values of 0..7
are already carved in stone, as long as comments in both common code
and libxfs code makes that clear, I see no harm in using the common
implementation in xfs, besides the need to yank more code from fs.h to
libxfs, but it's your decision.

Amir.

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

[adding linux-unionfs]

On Mon, Dec 19, 2016 at 11:55 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
>> deduplicate the xfs file type conversion implementation.
>>
>> xfs readdir code may expose DT_WHT type to user if that
>> type was set on disk, but xfs code never set a file type
>> of WHT on disk.
>>
>> If it is acceptable to expose to user DT_UNKNOWN in case
>> WHT type somehow got to disk, then xfs_dir3_filetype_table
>> could also be replaced with the common fs_dtype() helper.
>
> AFAIK XFS has never actually written XFS_DIR3_FT_WHT to disk.
> I see that overlayfs whiteouts are now some sort of weird
> chardev with rdev == 0, so I guess overlayfs doesn't either...?
>

Nope. overlayfs calls vfs_whiteout() which calls i_op->mknod(.. S_IFCHR, 0)
So AFAIK, there is no evidence of DT_WHT even being use in Linux.

>From overlayfs perspective, it could have been nice if conversion functions
took mode+rdev instead of just mode and produced the DT_WHT value,
but it is not all that easy to know how applications would react to this change.

I suppose there shouldn't be a problem to expose DT_WHT d_type in
iterate_dir() and convert it to DT_CHR in getdents' filldir().
It will be beneficial to overlayfs in case of a directory with tons of
whiteouts,
not having to stat all those inodes is a big win.
Not sure how common this use case is, but it is quite easy for users to
get to this sort of state when using inefficient container layouts.

How about xfs_repair? will it complain if it sees DT_WHT and a chardev
inode? does it check at all that the type and mode match?

Being able to play with these sort of mode+rdev+? generic conversions
is one of the reasons I proposed the common DT_ implementation.

Since XFS already has that WHT bit reserved on-disk I may as well
post some POC to use it correctly, so overlayfs can benefit from DT_WHT
in the best case scenario.

Amir.

[RFC][PATCH v2 01/11] fs: common implementation of file type conversions

[Amir Goldstein]

Many file systems use a copy&paste implementation
of dirent to on-disk file type conversions.

Create a common implementation to be used by file systems
with some useful conversion helpers to reduce open coded
file type conversions in file system code.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/file_type.h | 107 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h        |  17 +-------
 2 files changed, 108 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/file_type.h


Fixed a comment by Darrick and a build warning reported by kbuild robot.

These changes are local to the common implementation, so not re-spamming
with all the non changed fs patches.

v2:
- s/DT_MASK/S_DT_MASK to fix redefinition in drivers/scsi/qla2xxx/qla_def.h
- explicit initialization of fs_dtype_by_ftype[] using [FT_*] = DT_*

v1:
- initial implementation

diff --git a/include/linux/file_type.h b/include/linux/file_type.h
new file mode 100644
index 0000000..bb218f4
--- /dev/null
+++ b/include/linux/file_type.h
@@ -0,0 +1,107 @@
+#ifndef _LINUX_FILE_TYPE_H
+#define _LINUX_FILE_TYPE_H
+
+/*
+ * This is a common implementation of dirent to fs on-disk
+ * file type conversion.  Although the fs on-disk bits are
+ * specific to every file system, in practice, many file systems
+ * use the exact same on-disk format to describe the lower 3
+ * file type bits that represent the 7 POSIX file types.
+ * All those file systems can use this generic code for the
+ * conversions:
+ *  i_mode -> fs on-disk file type (ftype)
+ *  fs on-disk file type (ftype) -> dirent file type (dtype)
+ *  i_mode -> dirent file type (dtype)
+ */
+
+/*
+ * struct dirent file types
+ * exposed to user via getdents(2), readdir(3)
+ *
+ * These match bits 12..15 of stat.st_mode
+ * (ie "(i_mode >> 12) & 15").
+ */
+#define S_DT_SHIFT	12
+#define S_DT(mode)	(((mode) & S_IFMT) >> S_DT_SHIFT)
+#define S_DT_MASK	(S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN	0
+#define DT_FIFO		S_DT(S_IFIFO) /* 1 */
+#define DT_CHR		S_DT(S_IFCHR) /* 2 */
+#define DT_DIR		S_DT(S_IFDIR) /* 4 */
+#define DT_BLK		S_DT(S_IFBLK) /* 6 */
+#define DT_REG		S_DT(S_IFREG) /* 8 */
+#define DT_LNK		S_DT(S_IFLNK) /* 10 */
+#define DT_SOCK		S_DT(S_IFSOCK) /* 12 */
+#define DT_WHT		14
+
+#define DT_MAX		(S_DT_MASK + 1) /* 16 */
+
+/*
+ * fs on-disk file types.
+ * Only the low 3 bits are used for the POSIX file types.
+ * Other bits are reserved for fs private use.
+ *
+ * Note that no fs currently stores the whiteout type on-disk,
+ * so whiteout dirents are exposed to user as DT_CHR.
+ */
+#define FT_UNKNOWN	0
+#define FT_REG_FILE	1
+#define FT_DIR		2
+#define FT_CHRDEV	3
+#define FT_BLKDEV	4
+#define FT_FIFO		5
+#define FT_SOCK		6
+#define FT_SYMLINK	7
+
+#define FT_MAX		8
+
+/*
+ * fs on-disk file type to dirent file type conversion
+ */
+static unsigned char fs_dtype_by_ftype[FT_MAX] = {
+	[FT_UNKNOWN]	= DT_UNKNOWN,
+	[FT_REG_FILE]	= DT_REG,
+	[FT_DIR]	= DT_DIR,
+	[FT_CHRDEV]	= DT_CHR,
+	[FT_BLKDEV]	= DT_BLK,
+	[FT_FIFO]	= DT_FIFO,
+	[FT_SOCK]	= DT_SOCK,
+	[FT_SYMLINK]	= DT_LNK
+};
+
+static inline unsigned char fs_dtype(int filetype)
+{
+	if (filetype >= FT_MAX)
+		return DT_UNKNOWN;
+
+	return fs_dtype_by_ftype[filetype];
+}
+
+/*
+ * dirent file type to fs on-disk file type conversion
+ * Values not initialized explicitly are FT_UNKNOWN (0).
+ */
+static unsigned char fs_ftype_by_dtype[DT_MAX] = {
+	[DT_REG]	= FT_REG_FILE,
+	[DT_DIR]	= FT_DIR,
+	[DT_LNK]	= FT_SYMLINK,
+	[DT_CHR]	= FT_CHRDEV,
+	[DT_BLK]	= FT_BLKDEV,
+	[DT_FIFO]	= FT_FIFO,
+	[DT_SOCK]	= FT_SOCK,
+};
+
+/* st_mode to fs on-disk file type conversion */
+static inline unsigned char fs_umode_to_ftype(umode_t mode)
+{
+	return fs_ftype_by_dtype[S_DT(mode)];
+}
+
+/* st_mode to dirent file type conversion */
+static inline unsigned char fs_umode_to_dtype(umode_t mode)
+{
+	return fs_dtype(fs_umode_to_ftype(mode));
+}
+
+#endif
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e6e4146..8f1580d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -31,6 +31,7 @@
 #include <linux/workqueue.h>
 #include <linux/percpu-rwsem.h>
 #include <linux/delayed_call.h>
+#include <linux/file_type.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -1582,22 +1583,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 int fiemap_check_flags(struct fiemap_extent_info *fieinfo, u32 fs_flags);
 
 /*
- * File types
- *
- * NOTE! These match bits 12..15 of stat.st_mode
- * (ie "(i_mode >> 12) & 15").
- */
-#define DT_UNKNOWN	0
-#define DT_FIFO		1
-#define DT_CHR		2
-#define DT_DIR		4
-#define DT_BLK		6
-#define DT_REG		8
-#define DT_LNK		10
-#define DT_SOCK		12
-#define DT_WHT		14
-
-/*
  * This is the "filldir" function type, used by readdir() to let
  * the kernel specify what kind of dirent layout it wants to have.
  * This allows the kernel to read directories into kernel space or
-- 
2.7.4

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Tue, Dec 20, 2016 at 8:17 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> [adding linux-unionfs]
>
> On Mon, Dec 19, 2016 at 11:55 PM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
>> On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
>>> deduplicate the xfs file type conversion implementation.
>>>
>>> xfs readdir code may expose DT_WHT type to user if that
>>> type was set on disk, but xfs code never set a file type
>>> of WHT on disk.
>>>
>>> If it is acceptable to expose to user DT_UNKNOWN in case
>>> WHT type somehow got to disk, then xfs_dir3_filetype_table
>>> could also be replaced with the common fs_dtype() helper.
>>
>> AFAIK XFS has never actually written XFS_DIR3_FT_WHT to disk.
>> I see that overlayfs whiteouts are now some sort of weird
>> chardev with rdev == 0, so I guess overlayfs doesn't either...?
>>
>
> Nope. overlayfs calls vfs_whiteout() which calls i_op->mknod(.. S_IFCHR, 0)
> So AFAIK, there is no evidence of DT_WHT even being use in Linux.
>
> From overlayfs perspective, it could have been nice if conversion functions
> took mode+rdev instead of just mode and produced the DT_WHT value,
> but it is not all that easy to know how applications would react to this change.
>
> I suppose there shouldn't be a problem to expose DT_WHT d_type in
> iterate_dir() and convert it to DT_CHR in getdents' filldir().
> It will be beneficial to overlayfs in case of a directory with tons of
> whiteouts,
> not having to stat all those inodes is a big win.
> Not sure how common this use case is, but it is quite easy for users to
> get to this sort of state when using inefficient container layouts.
>
> How about xfs_repair? will it complain if it sees DT_WHT and a chardev
> inode? does it check at all that the type and mode match?
>

To answer my own question, yes, xfs_repair would complain and fix this,
so not possible to set DT_WHT type for the VFS whiteout creature
without adding a new feature flag.

Amir.

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Dave Chinner]

On Tue, Dec 20, 2016 at 07:20:22AM +0200, Amir Goldstein wrote:
> On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong
> <darrick.wong@oracle.com> wrote:
> > On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
> > Note: We did this all this work a short time ago so that ext4 and XFS
> > could present the same FSGETXATTR ioctl to user programs (major benefit)
> > but it was kind of a pain to get right.  I don't see an upside to ceding
> > control of this part of the disk format to the VFS.
> >
> 
> The answer "this is not wanted for XFS" is perfectly valid :)
> The common implementation can be used by the small fs, which never
> want anymore than the basic ext2 implementation.
> 
> However, since the DT_ values and  XFS_DIR3_FT_ values of 0..7
> are already carved in stone, as long as comments in both common code
> and libxfs code makes that clear, I see no harm in using the common
> implementation in xfs, besides the need to yank more code from fs.h to
> libxfs, but it's your decision.

It's a philoophical and architectural issue. We currently have a
distinct separation between generic functionality and per-filesystem
specific definitions. The on-disk definitions are owned by the
filesystem not the generic code, and the generic code /never/ sees
what the filesystem stores on disk. THe VFS is supposed to be
completely independent of what the filesystems store on disk - it's
an abstract concept - so that it can morph into whatever is required
to support the functionality the different filesystem provides.

The way we normally handle this is a method callout of some kind
into the filesystem to do the filesystem specific function, and
if there are multiple filesystems that do the same thing, they use a
common function. So that part of the patchset - providing common
helpers to inode mode/filesytem d_type conversion - is fine.

The part that isn't fine, IMO, is defining the filesystem d_type
values in generic code. They should be defined by the filesystem and
passed to the generic conversion functions as a constant. It may
require a different structure to do this cleanly (i.e. something
other than a sparse array keyed on S_IFMT), but I think that having
the VFS define on-disk formats like this is a slippery slope that
only leads to long term pain and ossification.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Wed, Dec 21, 2016 at 7:23 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Dec 20, 2016 at 07:20:22AM +0200, Amir Goldstein wrote:
>> On Tue, Dec 20, 2016 at 2:28 AM, Darrick J. Wong
>> <darrick.wong@oracle.com> wrote:
>> > On Mon, Dec 19, 2016 at 10:11:08PM +0200, Amir Goldstein wrote:
>> > Note: We did this all this work a short time ago so that ext4 and XFS
>> > could present the same FSGETXATTR ioctl to user programs (major benefit)
>> > but it was kind of a pain to get right.  I don't see an upside to ceding
>> > control of this part of the disk format to the VFS.
>> >
>>
>> The answer "this is not wanted for XFS" is perfectly valid :)
>> The common implementation can be used by the small fs, which never
>> want anymore than the basic ext2 implementation.
>>
>> However, since the DT_ values and  XFS_DIR3_FT_ values of 0..7
>> are already carved in stone, as long as comments in both common code
>> and libxfs code makes that clear, I see no harm in using the common
>> implementation in xfs, besides the need to yank more code from fs.h to
>> libxfs, but it's your decision.
>
> It's a philoophical and architectural issue. We currently have a
> distinct separation between generic functionality and per-filesystem
> specific definitions. The on-disk definitions are owned by the
> filesystem not the generic code, and the generic code /never/ sees
> what the filesystem stores on disk. THe VFS is supposed to be
> completely independent of what the filesystems store on disk - it's
> an abstract concept - so that it can morph into whatever is required
> to support the functionality the different filesystem provides.
>

Technically, this abstraction is not within the scope of VFS, but
more something of a "libfs", or code that belongs under fs/common,
much like the new fs/crypto and in theory those common bits
should also belong to a userspace libfs library, but that's taking
this a bit too far now.

> The way we normally handle this is a method callout of some kind
> into the filesystem to do the filesystem specific function, and
> if there are multiple filesystems that do the same thing, they use a
> common function. So that part of the patchset - providing common
> helpers to inode mode/filesytem d_type conversion - is fine.
>
> The part that isn't fine, IMO, is defining the filesystem d_type
> values in generic code. They should be defined by the filesystem and
> passed to the generic conversion functions as a constant. It may
> require a different structure to do this cleanly (i.e. something
> other than a sparse array keyed on S_IFMT), but I think that having
> the VFS define on-disk formats like this is a slippery slope that
> only leads to long term pain and ossification.
>

I agree.
I will add this fs-specfic-type-conversion to the common helpers.

common code will convert i_mode => DT_ => common FT_
and if fs provides a specific conversion table that will also
be used to convert common FT_ values to FS_FT_ values.

I'll see if that turns up to look useful. if not I will just leave the
common conversions from sparse DT values in common code
and leave the rest in fs specific code.

Thanks for the feedback.


Ted, Chris,

Do you share a similar "philosophy" on the matter?

[RFC][PATCH v2 11/11] xfs: use common file type conversion

[Amir Goldstein]

Deduplicate the common bits of the file type conversion implementation.

Use private conversion tables to convert from fs common FT_* file types
to on-disk file types and from on-disk file types to readdir(2) DT_* file
types.

NOTE that the common_to_dir3_ftype table is an indentity transformation -
the common FT_* values are an exact reflection of the v3 on-disk values.
The dir3_ftype_to_dtype table is identical to the common fs_dtype_by_ftype
table in linux/file_type.h with an additional single entry for the
never used value of XFS_DIR3_FT_WHT.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c  | 48 +++++++++++++++++++++++++++++++++++------------
 fs/xfs/libxfs/xfs_dir2.h  |  4 ++--
 fs/xfs/xfs_dir2_readdir.c |  7 +------
 fs/xfs/xfs_iops.c         |  2 +-
 4 files changed, 40 insertions(+), 21 deletions(-)

Ted, Chris and Dave,

I would appreciate a NACK or ACK on this xfs example implementation.
If the major fs maintainers are on board, I will apply this approach
to all other fs.

This is a draft for the mid grounds approach suggested by Dave, which
keeps the control of on-disk bits in the hands of the the file system
code and still uses some of the common implementation.

As a code deduplication patch, one can argue that it does a pretty lousy
job, removing only a single line of common code (#define S_SHIFT 12), but
I do think that the result is cleaner without all the open coded >> S_SHIFT
conversions.

In any case, all for all the small file systems, this cleanup should be a
win (exofs ACKed already), so it would be nice to get feedback from more
maintainers.

Amir.

Extra food for thought:
=======================
Beyond the obvious open coded bits cleanup, going forward, this schema
will allow adding more common FT_ values and for file systems to catch up
with them at will. For example, if FT_WHT were to be added to the common
file types, xfs could join the party with existing v3 format by adding
a single line to private conversion table:
       [FT_WHT]    = XFS_DIR3_FT_WHT,
and other fs could join the FT_WHT party after sorting out whatever
feature flags that need to be sorted out.

I envision adding some opaque 'hint bits' to common ftype -
file systems that will support the opaque hint bits will store them as is
on-disk and repair tools will mask them out when comparing to inode mode.
Think of those bits as a way to white list or black list objects on a
file system for a very fast low-level directory tree iteration.

There is one such use case for overlayfs, but I am sure there are more.
The hint bits are not supposed to be set nor cleared during the life time
of the directory entry. Setting a file type hint bit should only be possible
at directory entry creation time (i.e. create,mknod,mkdir,symlink,rename).


v2:
- add private conversion from common to on-disk values

v1:
- use common conversion functions to get on-disk values

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..ae711ad 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -37,21 +37,45 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
 
 /*
  * @mode, if set, indicates that the type field needs to be set up.
- * This uses the transformation from file mode to DT_* as defined in linux/fs.h
- * for file type specification. This will be propagated into the directory
- * structure if appropriate for the given operation and filesystem config.
+ * readdir(2) DT_* file types and fs common FT_* file types are defined
+ * in linux/file_type.h.
+ *
+ * These tables define the transformation from common FT_* file types to on-disk
+ * file types and from on-disk file types to readdir(2) DT_* file types.
+ *
+ * The on-disk file types will be propagated into the directory structure if
+ * appropriate for the given operation and filesystem config.
+ *
+ * NOTE that the common_to_dir3_ftype table is an indentity transformation -
+ * the common values are an exact reflection on the xfs v3 on-disk values.
+ * The dir3_ftype_to_dtype table is identical to the common fs_dtype_by_ftype
+ * table in linux/file_type.h with an additional single entry for the
+ * never used value of XFS_DIR3_FT_WHT.
  */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
+const unsigned char xfs_common_to_dir3_ftype[FT_MAX] = {
+	[FT_UNKNOWN]	= XFS_DIR3_FT_UNKNOWN,
+	[FT_REG_FILE]	= XFS_DIR3_FT_REG_FILE,
+	[FT_DIR]	= XFS_DIR3_FT_DIR,
+	[FT_CHRDEV]	= XFS_DIR3_FT_CHRDEV,
+	[FT_BLKDEV]	= XFS_DIR3_FT_BLKDEV,
+	[FT_FIFO]	= XFS_DIR3_FT_FIFO,
+	[FT_SOCK]	= XFS_DIR3_FT_SOCK,
+	[FT_SYMLINK]	= XFS_DIR3_FT_SYMLINK,
 };
 
+const unsigned char xfs_dir3_ftype_to_dtype[XFS_DIR3_FT_MAX] = {
+	[XFS_DIR3_FT_UNKNOWN]	= DT_UNKNOWN,
+	[XFS_DIR3_FT_REG_FILE]	= DT_REG,
+	[XFS_DIR3_FT_DIR]	= DT_DIR,
+	[XFS_DIR3_FT_CHRDEV]	= DT_CHR,
+	[XFS_DIR3_FT_BLKDEV]	= DT_BLK,
+	[XFS_DIR3_FT_FIFO]	= DT_FIFO,
+	[XFS_DIR3_FT_SOCK]	= DT_SOCK,
+	[XFS_DIR3_FT_SYMLINK]	= DT_LNK,
+	[XFS_DIR3_FT_WHT]	= DT_WHT,
+};
+
+
 /*
  * ASCII case-insensitive (ie. A-Z) support for directories that was
  * used in IRIX.
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..5018850 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -34,8 +34,8 @@ extern struct xfs_name	xfs_name_dotdot;
 /*
  * directory filetype conversion tables.
  */
-#define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+extern const unsigned char xfs_common_to_dir3_ftype[];
+extern const unsigned char xfs_dir3_ftype_to_dtype[];
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 003a99b..6d8a741 100644
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -36,11 +36,6 @@
 /*
  * Directory file type support functions
  */
-static unsigned char xfs_dir3_filetype_table[] = {
-	DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK,
-	DT_FIFO, DT_SOCK, DT_LNK, DT_WHT,
-};
-
 static unsigned char
 xfs_dir3_get_dtype(
 	struct xfs_mount	*mp,
@@ -52,7 +47,7 @@ xfs_dir3_get_dtype(
 	if (filetype >= XFS_DIR3_FT_MAX)
 		return DT_UNKNOWN;
 
-	return xfs_dir3_filetype_table[filetype];
+	return xfs_dir3_ftype_to_dtype[filetype];
 }
 
 STATIC int
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..36742fb 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@ xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = xfs_common_to_dir3_ftype[fs_umode_to_ftype(mode)];
 }
 
 STATIC void
-- 
2.7.4

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Theodore Ts'o]

On Wed, Dec 21, 2016 at 04:23:17PM +1100, Dave Chinner wrote:
> It's a philoophical and architectural issue. We currently have a
> distinct separation between generic functionality and per-filesystem
> specific definitions. The on-disk definitions are owned by the
> filesystem not the generic code, and the generic code /never/ sees
> what the filesystem stores on disk. THe VFS is supposed to be
> completely independent of what the filesystems store on disk - it's
> an abstract concept - so that it can morph into whatever is required
> to support the functionality the different filesystem provides.

I had the same concerns as Dave and Darrick --- which is that it just
"feels" wrong to define file system encoding semantics in generic
code.  So it's really much more of a taste issue than anything else.
I'll note that we've already started taking some steps down this
slippery path with the definition of whiteout --- where we define
WHITEOUT_MODE and WHITEOUT_DEV to be 0, and both the generic code and
the file system code need to agree that the on-disk encoding of a
whiteout is an inode with mode S_IFCHR | WHITEOUT_MODE, and with the
device number set to WHITEOUT_DEV.

(Why we define WHITEOUT_MODE to be zero when everyone has to *know*
and use the same on-disk encoding of S_IFCHR | WhiTEOUT_MODE doesn't
make any sense to me; it would be much simpler to #define
WHITEOUT_MODE to be S_IFCHR since if it ever changed, all file on-disk
encodings of overlay file systems would go *boom*.)

The fact that the three bit encoding of the directory file types is
fully defined, and can *not* ever be extended without breaking things
means that the chances that it would be a problem is much less.  So I
don't object quite as much as Dave and Darrick --- but I'll also point
out the benefits are quite small.  All we are saving is 16 bytes per
file system compiled into the kernel.

So it's a lot of discussion / changes for very little gain.

> The way we normally handle this is a method callout of some kind
> into the filesystem to do the filesystem specific function, and
> if there are multiple filesystems that do the same thing, they use a
> common function. So that part of the patchset - providing common
> helpers to inode mode/filesytem d_type conversion - is fine.

The common helpers are inline functions that do an array lookup.  If
we provide some other more generic way of letting the file systems
provide conversion functions, at that point we're not really saving
anything, and just adding complexity for no real benefit.

Which perhaps is my biggest argument against it; making this be
generic adds complexity where it's not really buying us anything
(except for 16 bytes of data segment).  But whether we think it's a
bad idea due to a slippery slope argument or due to a complexity
argument, it's clear that the gains are marginal, and while I
personally am willing compromise a little on the slippery slope
argument if the gains are large enough --- for example, the fs/crypto
code is certainly adding some commonality in ways that affect on-disk
encodings, in this particular case, the wins just don't seem high
enough.

Cheers,

						- Ted

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Wed, Dec 21, 2016 at 5:06 PM, Theodore Ts'o <tytso@mit.edu> wrote:
...

>
> The fact that the three bit encoding of the directory file types is
> fully defined, and can *not* ever be extended without breaking things
> means that the chances that it would be a problem is much less.  So I
> don't object quite as much as Dave and Darrick --- but I'll also point
> out the benefits are quite small.  All we are saving is 16 bytes per
> file system compiled into the kernel.
>
> So it's a lot of discussion / changes for very little gain.
>

Right. never claimed that saving data segment space is the issue
here. To be completely honest, I started looking at ways to optimize
whiteout iteration and was appalled to see all that code copy&paste,
so went on an OCD cleanup spree.

So it is really a lot more about clumsiness of the current code then
about saving actual lines on code or space in data segment.

Perhaps I should have settled with defining this common section:

+#define S_DT_SHIFT     12
+#define S_DT(mode)     (((mode) & S_IFMT) >> S_DT_SHIFT)
+#define S_DT_MASK      (S_IFMT >> S_DT_SHIFT)
+
+#define DT_UNKNOWN     0
+#define DT_FIFO                S_DT(S_IFIFO) /* 1 */
+#define DT_CHR         S_DT(S_IFCHR) /* 2 */
+#define DT_DIR         S_DT(S_IFDIR) /* 4 */
+#define DT_BLK         S_DT(S_IFBLK) /* 6 */
+#define DT_REG         S_DT(S_IFREG) /* 8 */
+#define DT_LNK         S_DT(S_IFLNK) /* 10 */
+#define DT_SOCK                S_DT(S_IFSOCK) /* 12 */
+#define DT_WHT         14
+
+#define DT_MAX         (S_DT_MASK + 1) /* 16 */

and using S_DT(mode) and DT_MAX in place of all the many places in the
code that open code the >> S_SHIFT.

Note that all those file system copy&pasted a potential bug.

This is an array of size 15:

static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {

and it is always addressed this way:

ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];

Which *can* try to address ext2_type_by_mode[15]

Now, can you say for certain that you can never get a malformed i_mode
with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
some code calling into VFS functions, which do not sanity check the
file type of the mode argument.

Regardless, setting an array size of 15 is just as buggy as setting an
array size of DT_SOCK+1 (13) because no syscall sets a larger value
for the type bits.


>> The way we normally handle this is a method callout of some kind
>> into the filesystem to do the filesystem specific function, and
>> if there are multiple filesystems that do the same thing, they use a
>> common function. So that part of the patchset - providing common
>> helpers to inode mode/filesytem d_type conversion - is fine.
>
> The common helpers are inline functions that do an array lookup.  If
> we provide some other more generic way of letting the file systems
> provide conversion functions, at that point we're not really saving
> anything, and just adding complexity for no real benefit.
>

You are absolutely right. Not sure if you replied after seeing
v2 of this patch, but I did not use any fs provided complex conversion
functions.

I left the XFS code mostly as it was (same complexity) and sorted
it out to use some common defines and helpers in a way that
seem cleaner to me. At least that minor bug has been addressed.

I could further simplify the patch to use the S_DT(mode) macro
instead of the fs_umode_to_ftype(mode) helper.
If I do that, then the small snippet above that defines S_DT()
and DT_MAX is the only code that needs to be carried from
linux/fs.h to xfsprogs for building libxfs (same goes for e2fsprogs).

That's would be a technical code cleanup that does not mess
with on-disk format definitions.
I will send out an ext4 sample patch for you to consider.

Thanks for the feedback,
Amir.

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Miklos Szeredi]

On Wed, Dec 21, 2016 at 4:06 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Wed, Dec 21, 2016 at 04:23:17PM +1100, Dave Chinner wrote:
>> It's a philoophical and architectural issue. We currently have a
>> distinct separation between generic functionality and per-filesystem
>> specific definitions. The on-disk definitions are owned by the
>> filesystem not the generic code, and the generic code /never/ sees
>> what the filesystem stores on disk. THe VFS is supposed to be
>> completely independent of what the filesystems store on disk - it's
>> an abstract concept - so that it can morph into whatever is required
>> to support the functionality the different filesystem provides.
>
> I had the same concerns as Dave and Darrick --- which is that it just
> "feels" wrong to define file system encoding semantics in generic
> code.  So it's really much more of a taste issue than anything else.
> I'll note that we've already started taking some steps down this
> slippery path with the definition of whiteout --- where we define
> WHITEOUT_MODE and WHITEOUT_DEV to be 0, and both the generic code and
> the file system code need to agree that the on-disk encoding of a
> whiteout is an inode with mode S_IFCHR | WHITEOUT_MODE, and with the
> device number set to WHITEOUT_DEV.

WHITEOUT_DEV is not meant to be an encoding, it's meant to be an API.

There are reasons why we didn't want to invent a new kind of object
for whiteouts and so the choice fell on some special kind of already
existing object type.  So it's now a char dev with 0 device number and
that's set in stone, at least as far as the userspace API goes.

Filesystems are, on the other hand, free to encode this object in
whatever way they wish.  And in the VFS we could represent it with a
new kind of object to be converted to the char dev on the userspace
interface, if that turns out to be a better design choice.

Thanks,
Miklos

[PATCH v3 11/11] xfs: use common file type conversion

[Amir Goldstein]

Use common mode to file type conversion macros.

Fix the size of the mode_to_ftype conversion table,
which was too small to handle the malformed value
of mode=S_IFMT.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c | 17 ++++++++---------
 fs/xfs/libxfs/xfs_dir2.h |  3 +--
 fs/xfs/xfs_iops.c        |  2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

Ted and Dave,

How's this for a less controversial cleanup?

I would send the ext4 patch, but it looks exactly the same,
so if you guys ACK this sample patch, I will re-post the
entire series.

There is still a question of whether or not to leave the
common FT_* definitions and conversion helpers in file_type.h.
I see no harm in that. Even if no current fs will want to use
them (exofs maintainer already indicated otherwise), the next
file system of the day may decide to use the common values
rather then re-defining its own set of identical on-disk values.

Amir.


v3:
- resort to simpler cleanup with macros DT_MAX and S_DT()
- mention the minor bug fix in commit message

v2:
- add private conversion from common to on-disk values

v1:
- use common conversion functions to get on-disk values

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..984530e 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -41,15 +41,14 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
  * for file type specification. This will be propagated into the directory
  * structure if appropriate for the given operation and filesystem config.
  */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
+const unsigned char xfs_dtype_to_ftype[DT_MAX] = {
+	[DT_REG]    = XFS_DIR3_FT_REG_FILE,
+	[DT_DIR]    = XFS_DIR3_FT_DIR,
+	[DT_CHR]    = XFS_DIR3_FT_CHRDEV,
+	[DT_BLK]    = XFS_DIR3_FT_BLKDEV,
+	[DT_FIFO]   = XFS_DIR3_FT_FIFO,
+	[DT_SOCK]   = XFS_DIR3_FT_SOCK,
+	[DT_LNK]    = XFS_DIR3_FT_SYMLINK,
 };
 
 /*
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..bb5a527 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -34,8 +34,7 @@ extern struct xfs_name	xfs_name_dotdot;
 /*
  * directory filetype conversion tables.
  */
-#define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+extern const unsigned char xfs_dtype_to_ftype[];
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..d2da9ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@ xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = xfs_dtype_to_ftype[S_DT(mode)];
 }
 
 STATIC void
-- 
2.7.4

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Theodore Ts'o]

On Wed, Dec 21, 2016 at 06:37:42PM +0200, Amir Goldstein wrote:
> 
> Right. never claimed that saving data segment space is the issue
> here. To be completely honest, I started looking at ways to optimize
> whiteout iteration and was appalled to see all that code copy&paste,
> so went on an OCD cleanup spree.
> 
> So it is really a lot more about clumsiness of the current code then
> about saving actual lines on code or space in data segment.
> 
> Perhaps I should have settled with defining this common section:

> +#define DT_WHT         14

What are you trying to accomplish here?  Unless we actually encoding
DT_WHT into the on-disk format, it's not really going to buy you much.

And if you are going to encode this into the on-disk format, each file
system is going to have to set some kind of flag in the superblock to
indicate whether it's doing this new thing or not.  And this is
*precisely* why Dave and Darrick are objecting --- if we are going to
make any kind of file system encoding change, the file system has to
know about it.

> Note that all those file system copy&pasted a potential bug.
> 
> This is an array of size 15:
> 
> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
> 
> and it is always addressed this way:
> 
> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
> 
> Which *can* try to address ext2_type_by_mode[15]
> 
> Now, can you say for certain that you can never get a malformed i_mode
> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
> some code calling into VFS functions, which do not sanity check the
> file type of the mode argument.

It's not a problem and not a bug, because we only assign the file type
when the inode is created, and at that point, the i_mode is set by the
*kernel*, and not by the on-disk encoding.

And BTW this is why DT_WHT makes no sense.  We the DT encodings come
from the directory entry, and *never* come from the inode i_mode read
from disk --- since at the time when we do the readdir(2), the inode
may not have been read into memory.  So we can't add DT_WHT unless we
also change the on-disk encoding of the directory entry on a file
system by file system basis.

Cheers,

						- Ted

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o <tytso@mit.edu> wrote:
> On Wed, Dec 21, 2016 at 06:37:42PM +0200, Amir Goldstein wrote:
>>
>> Right. never claimed that saving data segment space is the issue
>> here. To be completely honest, I started looking at ways to optimize
>> whiteout iteration and was appalled to see all that code copy&paste,
>> so went on an OCD cleanup spree.
>>
>> So it is really a lot more about clumsiness of the current code then
>> about saving actual lines on code or space in data segment.
>>
>> Perhaps I should have settled with defining this common section:
>
>> +#define DT_WHT         14
>
> What are you trying to accomplish here?  Unless we actually encoding
> DT_WHT into the on-disk format, it's not really going to buy you much.
>
> And if you are going to encode this into the on-disk format, each file
> system is going to have to set some kind of flag in the superblock to
> indicate whether it's doing this new thing or not.  And this is
> *precisely* why Dave and Darrick are objecting --- if we are going to
> make any kind of file system encoding change, the file system has to
> know about it.
>

Ted,

There is a bit of a confusion here.
Although I would have liked to be able to add DT_WHT to dirent
I found there is no east way to do it without adding fs feature flags.

I did not add the DT_WHT definion, I kept it as is just moved
from fs.h to file_type.h because some fs (e.g. xfs) still use this
ghost define.

>> Note that all those file system copy&pasted a potential bug.
>>
>> This is an array of size 15:
>>
>> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
>>
>> and it is always addressed this way:
>>
>> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
>>
>> Which *can* try to address ext2_type_by_mode[15]
>>
>> Now, can you say for certain that you can never get a malformed i_mode
>> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
>> some code calling into VFS functions, which do not sanity check the
>> file type of the mode argument.
>
> It's not a problem and not a bug, because we only assign the file type
> when the inode is created, and at that point, the i_mode is set by the
> *kernel*, and not by the on-disk encoding.
>

Not entirley true.
On rename some fs set dentry type of target by converting the mode of
the renamed object.
I don't remember what ext4 does, but I think it does the same,
so malformed imode on disk could potentially get you there.

> And BTW this is why DT_WHT makes no sense.  We the DT encodings come
> from the directory entry, and *never* come from the inode i_mode read
> from disk --- since at the time when we do the readdir(2), the inode
> may not have been read into memory.  So we can't add DT_WHT unless we
> also change the on-disk encoding of the directory entry on a file
> system by file system basis.
>


True. it makes no sense at all, but we can't remove it.

XFS can decide to start treating it as an error if found on disk,
but that's another story.

Amir.

Re: [RFC][PATCH 11/11] xfs: use common file type conversion

[Amir Goldstein]

On Thu, Dec 22, 2016 at 7:54 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Thu, Dec 22, 2016 at 12:56 AM, Theodore Ts'o <tytso@mit.edu> wrote:
...

>>> Note that all those file system copy&pasted a potential bug.
>>>
>>> This is an array of size 15:
>>>
>>> static unsigned char ext2_type_by_mode[S_IFMT >> S_SHIFT] = {
>>>
>>> and it is always addressed this way:
>>>
>>> ext2_type_by_mode[(inode->i_mode & S_IFMT)>>S_SHIFT];
>>>
>>> Which *can* try to address ext2_type_by_mode[15]
>>>
>>> Now, can you say for certain that you can never get a malformed i_mode
>>> with ((inode->i_mode & S_IFMT) == S_IFMT) either from disk or from
>>> some code calling into VFS functions, which do not sanity check the
>>> file type of the mode argument.
>>
>> It's not a problem and not a bug, because we only assign the file type
>> when the inode is created, and at that point, the i_mode is set by the
>> *kernel*, and not by the on-disk encoding.
>>
>
> Not entirley true.
> On rename some fs set dentry type of target by converting the mode of
> the renamed object.
> I don't remember what ext4 does, but I think it does the same,
> so malformed imode on disk could potentially get you there.
>

So I checked the code.

ext4 is off the hook because it sanitizes mode in ext4_iget and
returns -ECORRUPTEDFS

e2fsprogs' ext2_file_type() is off the hook as well, not using a
conversion table at all.

ext2 on the other hand just calls init_special_inode() for the else case
which in turn print a debug message "init_special_inode: bogus i_mode (%o)"
and doesn't do anything about it, so a later rename can certainly try to convert
a bogus i_mode of S_IFMT and escape the boundaries of the conversion table.

Anyway, I won't try to force feed anyone fix patches.

I am going to re-send the xfs patch as an independent fix patch and will
send a matching patch to xfsprogs.

Any other fs maintainers that want the fix are welcome to apply their
own version
or ask me to send a fix their way.

Cheers,
Amir.

[PATCH v4] xfs: fix the size of xfs_mode_to_ftype table

[Amir Goldstein]

Fix the size of the xfs_mode_to_ftype conversion table,
which was too small to handle a malformed on-disk value
of mode=S_IFMT.

Use a convenience macros S_DT(mode) to convert from
mode to dirent file type and change the name of the table
to xfs_dtype_to_ftype to correctly describe its index values.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/xfs/libxfs/xfs_dir2.c | 17 ++++++++---------
 fs/xfs/libxfs/xfs_dir2.h |  4 +++-
 fs/xfs/xfs_iops.c        |  2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

Darrick,

So my holiday season cleanup is now down to this one patch.
I pulled back the common code and added the needed macros in
xfs code, so this can be safely applied to xfsprogs as well.
I will send a patch to xfsprogs later.

Tested with generic/396 with -n ftype=0|1.

Amir.

v4:
- independent fix patch for xfs

v3:
- resort to simpler cleanup with macros DT_MAX and S_DT()
- mention the minor bug fix in commit message

v2:
- add private conversion from common to on-disk values

v1:
- use common conversion functions to get on-disk values

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index c58d72c..539f498 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -41,15 +41,14 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
  * for file type specification. This will be propagated into the directory
  * structure if appropriate for the given operation and filesystem config.
  */
-const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
-	[0]			= XFS_DIR3_FT_UNKNOWN,
-	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
-	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
-	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
-	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
-	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
-	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
-	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
+const unsigned char xfs_dtype_to_ftype[S_DT_MAX] = {
+	[DT_REG]    = XFS_DIR3_FT_REG_FILE,
+	[DT_DIR]    = XFS_DIR3_FT_DIR,
+	[DT_CHR]    = XFS_DIR3_FT_CHRDEV,
+	[DT_BLK]    = XFS_DIR3_FT_BLKDEV,
+	[DT_FIFO]   = XFS_DIR3_FT_FIFO,
+	[DT_SOCK]   = XFS_DIR3_FT_SOCK,
+	[DT_LNK]    = XFS_DIR3_FT_SYMLINK,
 };
 
 /*
diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
index 0197590..a069c3e 100644
--- a/fs/xfs/libxfs/xfs_dir2.h
+++ b/fs/xfs/libxfs/xfs_dir2.h
@@ -35,7 +35,9 @@ extern struct xfs_name	xfs_name_dotdot;
  * directory filetype conversion tables.
  */
 #define S_SHIFT 12
-extern const unsigned char xfs_mode_to_ftype[];
+#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
+#define S_DT_MAX       (S_DT(S_IFMT) + 1)
+extern const unsigned char xfs_dtype_to_ftype[];
 
 /*
  * directory operations vector for encode/decode routines
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 308bebb..d2da9ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -103,7 +103,7 @@ xfs_dentry_to_name(
 {
 	namep->name = dentry->d_name.name;
 	namep->len = dentry->d_name.len;
-	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
+	namep->type = xfs_dtype_to_ftype[S_DT(mode)];
 }
 
 STATIC void
-- 
2.7.4

Re: [PATCH v4] xfs: fix the size of xfs_mode_to_ftype table

[Darrick J. Wong]

On Thu, Dec 22, 2016 at 11:07:21PM +0200, Amir Goldstein wrote:
> Fix the size of the xfs_mode_to_ftype conversion table,
> which was too small to handle a malformed on-disk value
> of mode=S_IFMT.
> 
> Use a convenience macros S_DT(mode) to convert from
> mode to dirent file type and change the name of the table
> to xfs_dtype_to_ftype to correctly describe its index values.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/xfs/libxfs/xfs_dir2.c | 17 ++++++++---------
>  fs/xfs/libxfs/xfs_dir2.h |  4 +++-
>  fs/xfs/xfs_iops.c        |  2 +-
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> Darrick,
> 
> So my holiday season cleanup is now down to this one patch.
> I pulled back the common code and added the needed macros in
> xfs code, so this can be safely applied to xfsprogs as well.
> I will send a patch to xfsprogs later.
> 
> Tested with generic/396 with -n ftype=0|1.

Hm... the 396 test looks ok, but I think we'd need a fs-specific
testcase to go write a corrupt i_mode = S_IFMT filesystem to try
to trigger the verifiers, right?

> Amir.
> 
> v4:
> - independent fix patch for xfs
> 
> v3:
> - resort to simpler cleanup with macros DT_MAX and S_DT()
> - mention the minor bug fix in commit message
> 
> v2:
> - add private conversion from common to on-disk values
> 
> v1:
> - use common conversion functions to get on-disk values
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index c58d72c..539f498 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -41,15 +41,14 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>   * for file type specification. This will be propagated into the directory
>   * structure if appropriate for the given operation and filesystem config.
>   */
> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
> -	[0]			= XFS_DIR3_FT_UNKNOWN,
> -	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
> +const unsigned char xfs_dtype_to_ftype[S_DT_MAX] = {
> +	[DT_REG]    = XFS_DIR3_FT_REG_FILE,
> +	[DT_DIR]    = XFS_DIR3_FT_DIR,
> +	[DT_CHR]    = XFS_DIR3_FT_CHRDEV,
> +	[DT_BLK]    = XFS_DIR3_FT_BLKDEV,
> +	[DT_FIFO]   = XFS_DIR3_FT_FIFO,
> +	[DT_SOCK]   = XFS_DIR3_FT_SOCK,
> +	[DT_LNK]    = XFS_DIR3_FT_SYMLINK,
>  };
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 0197590..a069c3e 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -35,7 +35,9 @@ extern struct xfs_name	xfs_name_dotdot;
>   * directory filetype conversion tables.
>   */
>  #define S_SHIFT 12
> -extern const unsigned char xfs_mode_to_ftype[];
> +#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
> +#define S_DT_MAX       (S_DT(S_IFMT) + 1)
> +extern const unsigned char xfs_dtype_to_ftype[];
>  
>  /*
>   * directory operations vector for encode/decode routines
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 308bebb..d2da9ca 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>  {
>  	namep->name = dentry->d_name.name;
>  	namep->len = dentry->d_name.len;
> -	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
> +	namep->type = xfs_dtype_to_ftype[S_DT(mode)];

So I think your concern here is that we could read a inode in from disk
that has an invalid i_mode such that we'd overflow xfs_mode_to_ftype
when trying to set the dirent ftype while linking the inode into a
directory, correct?

In that case, xfs_iread calls xfs_iformat_fork as part of pulling the
inode in off disk, and xfs_iformat_fork validates that the S_IFMT part
of i_mode actually corresponds to a known inode type and sends back 
-EFSCORRUPTED if not.

I think that checking suffices to handle this.  I prefer that we look
for invalid on-disk metadata and prevent it from being loaded into
memory at all, rather than try to catch all the problems that happen as
a result of insufficient validation.

--D

>  }
>  
>  STATIC void
> -- 
> 2.7.4
> 

Re: [PATCH v4] xfs: fix the size of xfs_mode_to_ftype table

[Amir Goldstein]

On Fri, Dec 23, 2016 at 11:01 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Thu, Dec 22, 2016 at 11:07:21PM +0200, Amir Goldstein wrote:
>> Fix the size of the xfs_mode_to_ftype conversion table,
>> which was too small to handle a malformed on-disk value
>> of mode=S_IFMT.
>>
>> Use a convenience macros S_DT(mode) to convert from
>> mode to dirent file type and change the name of the table
>> to xfs_dtype_to_ftype to correctly describe its index values.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/xfs/libxfs/xfs_dir2.c | 17 ++++++++---------
>>  fs/xfs/libxfs/xfs_dir2.h |  4 +++-
>>  fs/xfs/xfs_iops.c        |  2 +-
>>  3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> Darrick,
>>
>> So my holiday season cleanup is now down to this one patch.
>> I pulled back the common code and added the needed macros in
>> xfs code, so this can be safely applied to xfsprogs as well.
>> I will send a patch to xfsprogs later.
>>
>> Tested with generic/396 with -n ftype=0|1.
>
> Hm... the 396 test looks ok, but I think we'd need a fs-specific
> testcase to go write a corrupt i_mode = S_IFMT filesystem to try
> to trigger the verifiers, right?
>

Right. I will write that testcase.

>> Amir.
>>
>> v4:
>> - independent fix patch for xfs
>>
>> v3:
>> - resort to simpler cleanup with macros DT_MAX and S_DT()
>> - mention the minor bug fix in commit message
>>
>> v2:
>> - add private conversion from common to on-disk values
>>
>> v1:
>> - use common conversion functions to get on-disk values
>>
>> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
>> index c58d72c..539f498 100644
>> --- a/fs/xfs/libxfs/xfs_dir2.c
>> +++ b/fs/xfs/libxfs/xfs_dir2.c
>> @@ -41,15 +41,14 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>>   * for file type specification. This will be propagated into the directory
>>   * structure if appropriate for the given operation and filesystem config.
>>   */
>> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
>> -     [0]                     = XFS_DIR3_FT_UNKNOWN,
>> -     [S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
>> -     [S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
>> -     [S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
>> -     [S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
>> -     [S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
>> -     [S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
>> -     [S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
>> +const unsigned char xfs_dtype_to_ftype[S_DT_MAX] = {
>> +     [DT_REG]    = XFS_DIR3_FT_REG_FILE,
>> +     [DT_DIR]    = XFS_DIR3_FT_DIR,
>> +     [DT_CHR]    = XFS_DIR3_FT_CHRDEV,
>> +     [DT_BLK]    = XFS_DIR3_FT_BLKDEV,
>> +     [DT_FIFO]   = XFS_DIR3_FT_FIFO,
>> +     [DT_SOCK]   = XFS_DIR3_FT_SOCK,
>> +     [DT_LNK]    = XFS_DIR3_FT_SYMLINK,
>>  };
>>
>>  /*
>> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
>> index 0197590..a069c3e 100644
>> --- a/fs/xfs/libxfs/xfs_dir2.h
>> +++ b/fs/xfs/libxfs/xfs_dir2.h
>> @@ -35,7 +35,9 @@ extern struct xfs_name      xfs_name_dotdot;
>>   * directory filetype conversion tables.
>>   */
>>  #define S_SHIFT 12
>> -extern const unsigned char xfs_mode_to_ftype[];
>> +#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
>> +#define S_DT_MAX       (S_DT(S_IFMT) + 1)
>> +extern const unsigned char xfs_dtype_to_ftype[];
>>
>>  /*
>>   * directory operations vector for encode/decode routines
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index 308bebb..d2da9ca 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>>  {
>>       namep->name = dentry->d_name.name;
>>       namep->len = dentry->d_name.len;
>> -     namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
>> +     namep->type = xfs_dtype_to_ftype[S_DT(mode)];
>
> So I think your concern here is that we could read a inode in from disk
> that has an invalid i_mode such that we'd overflow xfs_mode_to_ftype
> when trying to set the dirent ftype while linking the inode into a
> directory, correct?
>
> In that case, xfs_iread calls xfs_iformat_fork as part of pulling the
> inode in off disk, and xfs_iformat_fork validates that the S_IFMT part
> of i_mode actually corresponds to a known inode type and sends back
> -EFSCORRUPTED if not.
>
> I think that checking suffices to handle this.  I prefer that we look
> for invalid on-disk metadata and prevent it from being loaded into
> memory at all, rather than try to catch all the problems that happen as
> a result of insufficient validation.
>

In the context of the justification that I provided in the commit message of
this patch, I agree with you. Like ext4, xfs also provides proper protection
against reading malformed i_mode data from disk (unlike ext2 and its clones).
So I shouldn't have included the malformed i_mode argument in my patch,
when I copied the commit message over from ext2 patch.

OTOH, is it really wise to keep the table at size 15 when fixing its size as
the patch suggests costs practically nothing? better safe then sorry.

And now for my real reasoning behind this patch.

>From the point it time when xfs reflink is declared stable,
using it as underlying (host) fs for overlayfs containers, is going to become
a superior alternative to other local fs, because of:
 2ea9846 ovl: use vfs_clone_file_range() for copy up if possible

This head start paves the way to more xfs+overlayfs synergy.

For one such feature, I posted an early POC to linux-unionfs:
  ovl: use RENAME_DT_WHT to optimize ovl_dir_read_merged()
https://marc.info/?l=linux-unionfs&m=148242757001436&w=3

This requires fs specific support, for which I implemented a demo with xfs:
  xfs: support RENAME_VFS_DTYPE flag
https://marc.info/?l=linux-unionfs&m=148242757001433&w=3

I wanted to avoid cross posting to lists, but I probably should've CC'ed
you for context.

So now you understand my reasoning for fixing xfs_mode_to_ftype[]
to not assume anything about input mode.

Of course, this xfs_mode_to_ftype size fix can wait in by topic branch
until the future time when the said optimization will be considered and
after I propose the needed feature flag to xfs.

The reason I sent it out as is, detached from context and independent
of future developments is that I believe fixing the table size does have
merit on its own, certainly with nothing to loose.

FYI, at this moment, neither vfs_mknod() nor xfs_vn_mknod() sanitize
the mode argument. I know that overlayfs does not abuse this, but I did
not check if the 2 other call sites that call vfs_mknod (ecryptfs and nfsd)
sanitize mode.
That could be fixes of course in either vfs_mknod() and xfs_vn_mknod()
as well as audited in calling code.

Cheers,
Amir.

Re: [PATCH v4] xfs: fix the size of xfs_mode_to_ftype table

[Brian Foster]

On Thu, Dec 22, 2016 at 11:07:21PM +0200, Amir Goldstein wrote:
> Fix the size of the xfs_mode_to_ftype conversion table,
> which was too small to handle a malformed on-disk value
> of mode=S_IFMT.
> 
> Use a convenience macros S_DT(mode) to convert from
> mode to dirent file type and change the name of the table
> to xfs_dtype_to_ftype to correctly describe its index values.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

A couple nits...

>  fs/xfs/libxfs/xfs_dir2.c | 17 ++++++++---------
>  fs/xfs/libxfs/xfs_dir2.h |  4 +++-
>  fs/xfs/xfs_iops.c        |  2 +-
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> Darrick,
> 
> So my holiday season cleanup is now down to this one patch.
> I pulled back the common code and added the needed macros in
> xfs code, so this can be safely applied to xfsprogs as well.
> I will send a patch to xfsprogs later.
> 
> Tested with generic/396 with -n ftype=0|1.
> 
> Amir.
> 
> v4:
> - independent fix patch for xfs
> 
> v3:
> - resort to simpler cleanup with macros DT_MAX and S_DT()
> - mention the minor bug fix in commit message
> 
> v2:
> - add private conversion from common to on-disk values
> 
> v1:
> - use common conversion functions to get on-disk values
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index c58d72c..539f498 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -41,15 +41,14 @@ struct xfs_name xfs_name_dotdot = { (unsigned char *)"..", 2, XFS_DIR3_FT_DIR };
>   * for file type specification. This will be propagated into the directory
>   * structure if appropriate for the given operation and filesystem config.
>   */
> -const unsigned char xfs_mode_to_ftype[S_IFMT >> S_SHIFT] = {
> -	[0]			= XFS_DIR3_FT_UNKNOWN,
> -	[S_IFREG >> S_SHIFT]    = XFS_DIR3_FT_REG_FILE,
> -	[S_IFDIR >> S_SHIFT]    = XFS_DIR3_FT_DIR,
> -	[S_IFCHR >> S_SHIFT]    = XFS_DIR3_FT_CHRDEV,
> -	[S_IFBLK >> S_SHIFT]    = XFS_DIR3_FT_BLKDEV,
> -	[S_IFIFO >> S_SHIFT]    = XFS_DIR3_FT_FIFO,
> -	[S_IFSOCK >> S_SHIFT]   = XFS_DIR3_FT_SOCK,
> -	[S_IFLNK >> S_SHIFT]    = XFS_DIR3_FT_SYMLINK,
> +const unsigned char xfs_dtype_to_ftype[S_DT_MAX] = {

Please retain the use of FT_UNKNOWN.

> +	[DT_REG]    = XFS_DIR3_FT_REG_FILE,
> +	[DT_DIR]    = XFS_DIR3_FT_DIR,
> +	[DT_CHR]    = XFS_DIR3_FT_CHRDEV,
> +	[DT_BLK]    = XFS_DIR3_FT_BLKDEV,
> +	[DT_FIFO]   = XFS_DIR3_FT_FIFO,
> +	[DT_SOCK]   = XFS_DIR3_FT_SOCK,
> +	[DT_LNK]    = XFS_DIR3_FT_SYMLINK,
>  };
>  
>  /*
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index 0197590..a069c3e 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -35,7 +35,9 @@ extern struct xfs_name	xfs_name_dotdot;
>   * directory filetype conversion tables.
>   */
>  #define S_SHIFT 12
> -extern const unsigned char xfs_mode_to_ftype[];
> +#define S_DT(mode)     (((mode) & S_IFMT) >> S_SHIFT)
> +#define S_DT_MAX       (S_DT(S_IFMT) + 1)

I think I'd prefer to see the '+ 1' in the xfs_dtype_to_ftype()
definition.

Those aside, this patch seems fine to me with the simple justification
that the array size doesn't match the set of index values we
limit/validate/scope mode against. AFAIU, this isn't a known bug and
applies to mode values either read from disk or passed in externally.
Darrick has pointed out that we already validate mode values on disk. I
assume we (the kernel) do the same thing for mode values that are passed
to XFS, but I don't see anything wrong with considering invalid input
sufficiently to prevent us from doing something stupid (going off the
end of the array and crashing or, perhaps worse, reading a valid ftype
value) due to potential external (or future) brokenness or corruption.

IOW, it's a landmine fixup in a currently non-reproducible error
condition bundled with a minor cleanup to avoid all of the ugly shifting
in the array initialization.

Brian

> +extern const unsigned char xfs_dtype_to_ftype[];
>  
>  /*
>   * directory operations vector for encode/decode routines
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 308bebb..d2da9ca 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -103,7 +103,7 @@ xfs_dentry_to_name(
>  {
>  	namep->name = dentry->d_name.name;
>  	namep->len = dentry->d_name.len;
> -	namep->type = xfs_mode_to_ftype[(mode & S_IFMT) >> S_SHIFT];
> +	namep->type = xfs_dtype_to_ftype[S_DT(mode)];
>  }
>  
>  STATIC void
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Phillip Potter]

On Thu, Oct 25, 2018 at 01:08:04PM +0200, Jan Kara wrote:
> On Wed 24-10-18 09:43:21, Amir Goldstein wrote:
> > 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 can take these patches through my tree if Al takes too long to respond
> (but he apparently already had a look at the patches which is a good sign
> ;) and if patches are reviewed by respective fs maintainers.
> 
> 								Honza
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Dear Jan,

Thank you - and thank you Al as well for taking a look :-) I will publish
a new series later on (possibly tomorrow) with all the feedback I've
gathered.

Regards,
Phil

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

[Jan Kara]

On Wed 24-10-18 09:43:21, Amir Goldstein wrote:
> 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 can take these patches through my tree if Al takes too long to respond
(but he apparently already had a look at the patches which is a good sign
;) and if patches are reviewed by respective fs maintainers.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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@philpotter.co.uk> 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
>

[RFC][PATCH 00/11] 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.
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.

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.

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