| Message ID | 20181023201953.GA15687@pathfinder (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v3,01/10] fs: common implementation of file type | expand |
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 >
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
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.
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
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.
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
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.
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]; > +}
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
On Wed, Oct 24, 2018 at 01:37:40PM +0100, Al Viro wrote: > 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]; > > +} Dear Al, Thank you, good point, I will change to unsigned int and republish as part of new series. Regards, Phil
On Wed, Oct 24, 2018 at 09:02:06AM -0400, Theodore Y. Ts'o 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? > > 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 Dear Ted, Regarding location of the additional header, thank you for this suggestion - I will move it. As for making it extra clear that the definitions must not change, I will add this to the comment at the beginning of the file, and also in the intro message of the new series I am about to publish. I can't speak for Amir, but regarding why I think this would be a good change, my thought process is that it would make adding new file systems that use these on-disk type layouts in future more easy, as the central generic implementation can be used rather than copying and pasting and changing names etc. as needed, which is more error prone. I am happy to add more documentation in this regard, and also mention savings where appropriate. Regards, Phil
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.
On Wed, Oct 24, 2018 at 05:41:15PM +0300, Amir Goldstein wrote: > 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. Dear Amir, Regarding the location of the extra header file, I'm happy to stick it in either include/linux or include/uapi/linux before sending out the revised series. You make a valid point about the FT_ constants and macros being kernel only though, so perhaps the comment is a better option? I will wait a bit before sending out as I'm interested to see what Ted thinks. Many thanks. Regards, Phil
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
On Thu, Oct 25, 2018 at 01:20:44PM +0200, Jan Kara wrote: > 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 Dear Jan, Thanks for your comments/feedback, really appreciate it. All good points and I will make sure I act on it all before republishing the series. I'd be interested to know your thoughts on Ted T'so's suggestion about moving the new header to the uapi area - yes or no in your opinion? Happy with either, but looking for the widest possible acceptance. Thanks. Regards, Phil
On Thu 25-10-18 13:56:23, Phillip Potter wrote: > Thanks for your comments/feedback, really appreciate it. All good points > and I will make sure I act on it all before republishing the series. I'd > be interested to know your thoughts on Ted T'so's suggestion about moving > the new header to the uapi area - yes or no in your opinion? Happy with > either, but looking for the widest possible acceptance. Thanks. I don't think moving this to uapi headers makes sense. As Amir wrote, all necessary definitions for userspace already come from glibc headers. Honza
On Thu, Oct 25, 2018 at 03:42:59PM +0200, Jan Kara wrote: > On Thu 25-10-18 13:56:23, Phillip Potter wrote: > > Thanks for your comments/feedback, really appreciate it. All good points > > and I will make sure I act on it all before republishing the series. I'd > > be interested to know your thoughts on Ted T'so's suggestion about moving > > the new header to the uapi area - yes or no in your opinion? Happy with > > either, but looking for the widest possible acceptance. Thanks. > > I don't think moving this to uapi headers makes sense. As Amir wrote, all > necessary definitions for userspace already come from glibc headers. > > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR Thanks, I will leave it in include/linux for now then and separate out the relevant parts to a separate C file as you suggested. Regards, Phil
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, I am almost ready to send out the new series with all the suggestions so far incorporated. Would you be happy for me to add to the commit messages slightly to mention David Sterba's point about a brief explanation? It would be the same in each case and I would keep it brief. Also, I have split the functions into a C file as per Jan Kara's suggestion. Are you still happy for me to still include your Signed-off-by tag or would you rather I keep it out until you've had a look? Thanks. Regards, Phil
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.
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