[RFC][PATCH v3 01/10] fs: common implementation of file type

[RFC][PATCH v3 01/10] fs: common implementation of file type

[Phillip Potter]

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.

Original patch written by Amir Goldstein.

v3:
- Rebased against Linux 4.19 by Phillip Potter
- Added SPDX tag to new include/linux/file_type.h
- Added include/linux/file_type.h to MAINTAINERS

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

Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 MAINTAINERS               |   1 +
 include/linux/file_type.h | 108 ++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h        |  17 +-----
 3 files changed, 110 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/file_type.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b2f710eee67a..8e5b029886e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5689,6 +5689,7 @@ L:	linux-fsdevel@vger.kernel.org
 S:	Maintained
 F:	fs/*
 F:	include/linux/fs.h
+F:	include/linux/file_type.h
 F:	include/uapi/linux/fs.h
 
 FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
diff --git a/include/linux/file_type.h b/include/linux/file_type.h
new file mode 100644
index 000000000000..f015c41ca90c
--- /dev/null
+++ b/include/linux/file_type.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#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 897eae8faee1..b42f04acf06e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -37,6 +37,7 @@
 #include <linux/uuid.h>
 #include <linux/errseq.h>
 #include <linux/ioprio.h>
+#include <linux/file_type.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>
@@ -1663,22 +1664,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
 			    u64 phys, u64 len, u32 flags);
 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.
-- 
2.17.2

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Amir Goldstein]

On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter <phil@philpotter.co.uk> 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.
>
> Original patch written by Amir Goldstein.

Looks good.
I guess you used 'git apply' or just 'patch'
What you usually do when applying someone else mostly unchanged
patches is use 'git am -s -3' so you preserve the original author and
original commit message including the Signed-of-by line.
You can edit your patch by hand to change the From: line to change the
author and add
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
(you sign below me as you changed the patch last)

>
> v3:
> - Rebased against Linux 4.19 by Phillip Potter
> - Added SPDX tag to new include/linux/file_type.h
> - Added include/linux/file_type.h to MAINTAINERS
>

As you can see in my original posting, this part comes after the ---
line which means it will not be included in the commit message,
because the specific development history of this patch may be interesting
for reviewers but it is less interesting in the context of git log.

> 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
>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
>  MAINTAINERS               |   1 +
>  include/linux/file_type.h | 108 ++++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h        |  17 +-----
>  3 files changed, 110 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/file_type.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b2f710eee67a..8e5b029886e6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5689,6 +5689,7 @@ L:        linux-fsdevel@vger.kernel.org
>  S:     Maintained
>  F:     fs/*
>  F:     include/linux/fs.h
> +F:     include/linux/file_type.h
>  F:     include/uapi/linux/fs.h
>
>  FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
> diff --git a/include/linux/file_type.h b/include/linux/file_type.h
> new file mode 100644
> index 000000000000..f015c41ca90c
> --- /dev/null
> +++ b/include/linux/file_type.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#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 897eae8faee1..b42f04acf06e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -37,6 +37,7 @@
>  #include <linux/uuid.h>
>  #include <linux/errseq.h>
>  #include <linux/ioprio.h>
> +#include <linux/file_type.h>
>
>  #include <asm/byteorder.h>
>  #include <uapi/linux/fs.h>
> @@ -1663,22 +1664,6 @@ int fiemap_fill_next_extent(struct fiemap_extent_info *info, u64 logical,
>                             u64 phys, u64 len, u32 flags);
>  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.
> --
> 2.17.2
>

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Phillip Potter]

On Wed, Oct 24, 2018 at 09:16:20AM +0300, Amir Goldstein wrote:
> On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter <phil@philpotter.co.uk> 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.
> >
> > Original patch written by Amir Goldstein.
> 
> Looks good.
> I guess you used 'git apply' or just 'patch'
> What you usually do when applying someone else mostly unchanged
> patches is use 'git am -s -3' so you preserve the original author and
> original commit message including the Signed-of-by line.
> You can edit your patch by hand to change the From: line to change the
> author and add
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> (you sign below me as you changed the patch last)
> 

Dear Amir,

Yes, I applied each patch manually to my tree, fixed it up where needed,
then after rebuilding and testing each one I committed it and regenerated
each patch. Thank you very much for your advice, I will take it into
account and make the necessary changes. In the meantime, do I add other
tags in the order they are received also (such as Reviewed-by:) and am
I safe to add these in when I re-send the patches with the changes you
and others have suggested (or would that offend people that have
offered the tags)?

Regards,
Phil

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Amir Goldstein]

On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter <phil@philpotter.co.uk> wrote:
>
> On Wed, Oct 24, 2018 at 09:16:20AM +0300, Amir Goldstein wrote:
> > On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter <phil@philpotter.co.uk> 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.
> > >
> > > Original patch written by Amir Goldstein.
> >
> > Looks good.
> > I guess you used 'git apply' or just 'patch'
> > What you usually do when applying someone else mostly unchanged
> > patches is use 'git am -s -3' so you preserve the original author and
> > original commit message including the Signed-of-by line.
> > You can edit your patch by hand to change the From: line to change the
> > author and add
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > (you sign below me as you changed the patch last)
> >
>
> Dear Amir,
>
> Yes, I applied each patch manually to my tree, fixed it up where needed,
> then after rebuilding and testing each one I committed it and regenerated
> each patch. Thank you very much for your advice, I will take it into
> account and make the necessary changes. In the meantime, do I add other
> tags in the order they are received also (such as Reviewed-by:) and am
> I safe to add these in when I re-send the patches with the changes you
> and others have suggested (or would that offend people that have
> offered the tags)?
>

Reviewed-by before of after Signed-off-by.
I prefer Signed-off-by last which conceptually covers the entire patch,
the commit message including all the review tags that you may have added.

Some developers add Reviewed-by after Signed-off-by signifying the
order that things happened, so choose your own preference.

As a reviewer, and I speak only for myself, if I offered my Reviewed-by
I expect it to be removed if a future revision of the patch has changed
so I have an indication of patches that I need to re-review.
But if the patch changed very lightly, like small edits to commit message
and code nits in general, that would not invalidate my review.
When in doubt, you can always explicitly ask the reviewer.

Thanks,
Amir.

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Phillip Potter]

On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote:
> On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter <phil@philpotter.co.uk> wrote:
> > Dear Amir,
> >
> > Yes, I applied each patch manually to my tree, fixed it up where needed,
> > then after rebuilding and testing each one I committed it and regenerated
> > each patch. Thank you very much for your advice, I will take it into
> > account and make the necessary changes. In the meantime, do I add other
> > tags in the order they are received also (such as Reviewed-by:) and am
> > I safe to add these in when I re-send the patches with the changes you
> > and others have suggested (or would that offend people that have
> > offered the tags)?
> >
> 
> Reviewed-by before of after Signed-off-by.
> I prefer Signed-off-by last which conceptually covers the entire patch,
> the commit message including all the review tags that you may have added.
> 
> Some developers add Reviewed-by after Signed-off-by signifying the
> order that things happened, so choose your own preference.
> 
> As a reviewer, and I speak only for myself, if I offered my Reviewed-by
> I expect it to be removed if a future revision of the patch has changed
> so I have an indication of patches that I need to re-review.
> But if the patch changed very lightly, like small edits to commit message
> and code nits in general, that would not invalidate my review.
> When in doubt, you can always explicitly ask the reviewer.
> 
> Thanks,
> Amir.

Dear Amir,

Thanks - I am just going to fix up the commit messages as you suggested
using git am etc. The content of the patches themselves will not change
(until further feedback is received).

Regards,
Phil

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Amir Goldstein]

On Wed, Oct 24, 2018 at 12:31 PM Phillip Potter <phil@philpotter.co.uk> wrote:
>
> On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote:
> > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter <phil@philpotter.co.uk> wrote:
> > > Dear Amir,
> > >
> > > Yes, I applied each patch manually to my tree, fixed it up where needed,
> > > then after rebuilding and testing each one I committed it and regenerated
> > > each patch. Thank you very much for your advice, I will take it into
> > > account and make the necessary changes. In the meantime, do I add other
> > > tags in the order they are received also (such as Reviewed-by:) and am
> > > I safe to add these in when I re-send the patches with the changes you
> > > and others have suggested (or would that offend people that have
> > > offered the tags)?
> > >
> >
> > Reviewed-by before of after Signed-off-by.
> > I prefer Signed-off-by last which conceptually covers the entire patch,
> > the commit message including all the review tags that you may have added.
> >
> > Some developers add Reviewed-by after Signed-off-by signifying the
> > order that things happened, so choose your own preference.
> >
> > As a reviewer, and I speak only for myself, if I offered my Reviewed-by
> > I expect it to be removed if a future revision of the patch has changed
> > so I have an indication of patches that I need to re-review.
> > But if the patch changed very lightly, like small edits to commit message
> > and code nits in general, that would not invalidate my review.
> > When in doubt, you can always explicitly ask the reviewer.
> >
> > Thanks,
> > Amir.
>
> Dear Amir,
>
> Thanks - I am just going to fix up the commit messages as you suggested
> using git am etc. The content of the patches themselves will not change
> (until further feedback is received).
>

Well, I did request to change some content (the location and the comment
above BUILD_BUG_ON section) which is relevant for several patches.
However, so far affected patched did not get any Reviewed-by.

Thanks,
Amir.

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Phillip Potter]

On Wed, Oct 24, 2018 at 12:44:50PM +0300, Amir Goldstein wrote:
> On Wed, Oct 24, 2018 at 12:31 PM Phillip Potter <phil@philpotter.co.uk> wrote:
> >
> > On Wed, Oct 24, 2018 at 12:20:14PM +0300, Amir Goldstein wrote:
> > > On Wed, Oct 24, 2018 at 11:21 AM Phillip Potter <phil@philpotter.co.uk> wrote:
> > > > Dear Amir,
> > > >
> > > > Yes, I applied each patch manually to my tree, fixed it up where needed,
> > > > then after rebuilding and testing each one I committed it and regenerated
> > > > each patch. Thank you very much for your advice, I will take it into
> > > > account and make the necessary changes. In the meantime, do I add other
> > > > tags in the order they are received also (such as Reviewed-by:) and am
> > > > I safe to add these in when I re-send the patches with the changes you
> > > > and others have suggested (or would that offend people that have
> > > > offered the tags)?
> > > >
> > >
> > > Reviewed-by before of after Signed-off-by.
> > > I prefer Signed-off-by last which conceptually covers the entire patch,
> > > the commit message including all the review tags that you may have added.
> > >
> > > Some developers add Reviewed-by after Signed-off-by signifying the
> > > order that things happened, so choose your own preference.
> > >
> > > As a reviewer, and I speak only for myself, if I offered my Reviewed-by
> > > I expect it to be removed if a future revision of the patch has changed
> > > so I have an indication of patches that I need to re-review.
> > > But if the patch changed very lightly, like small edits to commit message
> > > and code nits in general, that would not invalidate my review.
> > > When in doubt, you can always explicitly ask the reviewer.
> > >
> > > Thanks,
> > > Amir.
> >
> > Dear Amir,
> >
> > Thanks - I am just going to fix up the commit messages as you suggested
> > using git am etc. The content of the patches themselves will not change
> > (until further feedback is received).
> >
> 
> Well, I did request to change some content (the location and the comment
> above BUILD_BUG_ON section) which is relevant for several patches.
> However, so far affected patched did not get any Reviewed-by.
> 
> Thanks,
> Amir.

Sorry, I missed that bit at the end, was too keen to click through to the
note about the alleged ext2 bug :-) I will make sure those changes are made
as well. By location do you mean the location of the v3, v2, etc. stuff and
your point about including it in the main [0/10] message rather than the
patches themselves? Again, thank you for your feedback and for being patient
with me, I really do appreciate it.

Regards,
Phil

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Amir Goldstein]

On Wed, Oct 24, 2018 at 12:56 PM Phillip Potter <phil@philpotter.co.uk> wrote:
>
> On Wed, Oct 24, 2018 at 12:44:50PM +0300, Amir Goldstein wrote:
...
> > Well, I did request to change some content (the location and the comment
> > above BUILD_BUG_ON section) which is relevant for several patches.
> > However, so far affected patched did not get any Reviewed-by.
> >
> > Thanks,
> > Amir.
>
> Sorry, I missed that bit at the end, was too keen to click through to the
> note about the alleged ext2 bug :-) I will make sure those changes are made
> as well. By location do you mean the location of the v3, v2, etc. stuff and
> your point about including it in the main [0/10] message rather than the
> patches themselves? Again, thank you for your feedback and for being patient
> with me, I really do appreciate it.
>

By location I meant place BUILD_BUG_ON() at the beginning of
ext2_set_de_type() and not nested inside inner scope of ext2_readdir().
And find similar appropriate places for other patches.

And one more thing for next posting - LKML is probably a too wide forum for
this filesystems cleanup series.

Cheers,
Amir.

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Al Viro]

On Tue, Oct 23, 2018 at 09:19:53PM +0100, Phillip Potter wrote:

> +static inline unsigned char fs_dtype(int filetype)

That "int" is asking for trouble, especially since negative
argument will blow up.  And it comes from untrusted source...

> +{
> +	if (filetype >= FT_MAX)
> +		return DT_UNKNOWN;
> +
> +	return fs_dtype_by_ftype[filetype];
> +}

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Theodore Y. Ts'o]

On Tue, Oct 23, 2018 at 09:19:53PM +0100, Phillip Potter wrote:
> diff --git a/include/linux/file_type.h b/include/linux/file_type.h

Shouldn't this be in include/uapi/linux/fs_types.h?

One of things which must be made crystal clear is these definitions
MUST NOT ever change.  It would break the Userspace ABI, and would
break file systems on-disk format.

It might also be useful to be clear *why* we are making this change in
the first place.  Code refactorization is good from a code maintenance
perspective (either to fix bugs, although this code is pretty
trivial), or to make it easier to make changes in a single central
place (which MUST NOT) happen, or to make the compiled code more
compact.

So some documentation of how much text is actually saved might be
worthwhile.

						- Ted

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Amir Goldstein]

On Wed, Oct 24, 2018 at 4:02 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Tue, Oct 23, 2018 at 09:19:53PM +0100, Phillip Potter wrote:
> > diff --git a/include/linux/file_type.h b/include/linux/file_type.h
>
> Shouldn't this be in include/uapi/linux/fs_types.h?
>

IDGI. Why do we want this file in uapi?
The DT_ constants are already defined by glibc dirent.h
and the FT_ constants and macros we don't want to expose
to uapi at all. Right?

Maybe all we need is a comment above DT_ constants
that those are defined by POSIX and in glibc dirent.h?

> One of things which must be made crystal clear is these definitions
> MUST NOT ever change.  It would break the Userspace ABI, and would
> break file systems on-disk format.
>
> It might also be useful to be clear *why* we are making this change in
> the first place.  Code refactorization is good from a code maintenance
> perspective (either to fix bugs, although this code is pretty
> trivial),

Very trivial code that has had an out of bounds access bug for two
decades and bug was duplicated to 7 filesystems. IMO, fixing the bug in
one place instead of 7 is a good enough reason for re-factoring.

Thanks,
Amir.

Re: [RFC][PATCH v3 01/10] fs: common implementation of file type

[Jan Kara]

On Tue 23-10-18 21:19:53, Phillip Potter 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.

Thanks for the patch. I like the cleanup. Some comments inline...

> diff --git a/include/linux/file_type.h b/include/linux/file_type.h
> new file mode 100644
> index 000000000000..f015c41ca90c
> --- /dev/null
> +++ b/include/linux/file_type.h
> @@ -0,0 +1,108 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#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)

I don't think these three lines above are really useful. If you want to
make it easier for fs developers, add Docbook coments at function
implementations.

> + */
> +
> +/*
> + * 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 */

Why not keep the original definition by absolute number. After all these
must match glibc...

> +#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
> +};

So you define static variable in a header file. That will make it appear in
each and every object that includes this header (when not used it will get
optimized away but still...). IMO that is not good and I don't think
anything here is so performance super-crittical, that the cost of
additional function call would matter (correct me if I'm wrong here). So
I'd rather see these tables and associated functions in some C file.

> +static inline unsigned char fs_dtype(int filetype)

This function name is not very descriptive and consistent with the other
two. It should be like fs_ftype_to_dtype(), right?

> +{
> +	if (filetype >= FT_MAX)
> +		return DT_UNKNOWN;
> +
> +	return fs_dtype_by_ftype[filetype];
> +}

Otherwise the patch looks good to me.

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