| Message ID | 20181020220957.GA7267@pathfinder (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: ufs: Remove switch statement from ufs_set_de_type function | expand |
On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > header and replace with simple assignment. For each case, S_IFx >> 12 > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > us the correct file type. It is expected that for *nix compatibility > reasons, the relation between S_IFx and DT_x will not change. For > cases where the mode is invalid, upper layer validation catches this > anyway, so this improves readability and arguably performance by > assigning (mode & S_IFMT) >> 12 directly without any jump table > or conditional logic. Shouldn't we also do this for other filesystems? A quick scan suggests someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > --- > fs/ufs/util.h | 30 ++---------------------------- > 1 file changed, 2 insertions(+), 28 deletions(-) > > diff --git a/fs/ufs/util.h b/fs/ufs/util.h > index 1fd3011ea623..7e0c0878b9f9 100644 > --- a/fs/ufs/util.h > +++ b/fs/ufs/util.h > @@ -16,6 +16,7 @@ > * some useful macros > */ > #define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len)) > +#define S_SHIFT 12 > > /* > * functions used for retyping > @@ -158,34 +159,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 = (mode & S_IFMT) >> S_SHIFT; > } > > static inline u32 > -- > 2.17.2 >
On Sat, Oct 20, 2018 at 03:26:37PM -0700, Matthew Wilcox wrote: > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > header and replace with simple assignment. For each case, S_IFx >> 12 > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > us the correct file type. It is expected that for *nix compatibility > > reasons, the relation between S_IFx and DT_x will not change. For > > cases where the mode is invalid, upper layer validation catches this > > anyway, so this improves readability and arguably performance by > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > or conditional logic. > > Shouldn't we also do this for other filesystems? A quick scan suggests > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. Do what for other filesystems? E.g. ext* has the type cached in directory entry - as a straight enum, so encoding is needed. Which we do. ocfs2 is pretty certain to have it in ext*-compatible layout. ubifs stores them in another enum of its own (also handled). XFS - another enum (present on some layout variants), etc. And... CIFS? Seriously? This stuff is about encoding used to cache the file type in directory entry; how the bleeding hell would CIFS _client_ get anywhere near that?
On Sun, Oct 21, 2018 at 1:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > header and replace with simple assignment. For each case, S_IFx >> 12 > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > us the correct file type. It is expected that for *nix compatibility > > reasons, the relation between S_IFx and DT_x will not change. For > > cases where the mode is invalid, upper layer validation catches this > > anyway, so this improves readability and arguably performance by > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > or conditional logic. > > Shouldn't we also do this for other filesystems? A quick scan suggests > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > I've tried to do that 2 years ago, not even for readability, but to fix a lurking bug in conversion table implementation that was copy&pasted from ext2 to many other fs: https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2 https://marc.info/?l=linux-fsdevel&w=2&r=1&s=amir73il+file+type+conversion&q=b The push back from ext4/xfs maintainers was that while all fs use common values to encode d_type in on disk format, those on-disk format values are private to the fs. Eventually, the fix that got merged (only to xfs) did the opposite, it converted the conversion table lookup to a switch statement: https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 Some fs maintainers were happy about the initial version, but I did not pursue pushing that cleanup: https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 Phillip, you are welcome to re-spin those patches if you like. Note that the generic conversion helpers do not rely on upper layer to catch invalid mode values. Cheers, Amir.
On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > On Sun, Oct 21, 2018 at 1:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > > header and replace with simple assignment. For each case, S_IFx >> 12 > > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > > us the correct file type. It is expected that for *nix compatibility > > > reasons, the relation between S_IFx and DT_x will not change. For > > > cases where the mode is invalid, upper layer validation catches this > > > anyway, so this improves readability and arguably performance by > > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > > or conditional logic. > > > > Shouldn't we also do this for other filesystems? A quick scan suggests > > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > > > > I've tried to do that 2 years ago, not even for readability, but to > fix a lurking > bug in conversion table implementation that was copy&pasted from ext2 to > many other fs: > https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2 > https://marc.info/?l=linux-fsdevel&w=2&r=1&s=amir73il+file+type+conversion&q=b > > The push back from ext4/xfs maintainers was that while all fs use common > values to encode d_type in on disk format, those on-disk format values > are private to the fs. > > Eventually, the fix that got merged (only to xfs) did the opposite, it converted > the conversion table lookup to a switch statement: > https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 > > Some fs maintainers were happy about the initial version, but I did not > pursue pushing that cleanup: > https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 > > Phillip, you are welcome to re-spin those patches if you like. > Note that the generic conversion helpers do not rely on upper layer > to catch invalid mode values. > > Cheers, > Amir. I only changed this code in the first place because of the comment in the code - I was looking for things to do basically, and this caught my eye because I play with FreeBSD now and then which of course uses UFS. I didn't consider any use to the other filesystems - by re-spin do you just mean fix up and make sure the kernel still builds etc? Regards, Phil
On Sun, Oct 21, 2018 at 12:57 PM Phillip Potter <phil@philpotter.co.uk> wrote: > > On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > > On Sun, Oct 21, 2018 at 1:27 AM Matthew Wilcox <willy@infradead.org> wrote: > > > > > > On Sat, Oct 20, 2018 at 11:09:57PM +0100, Phillip Potter wrote: > > > > Remove switch statement from ufs_set_de_type function in fs/ufs/util.h > > > > header and replace with simple assignment. For each case, S_IFx >> 12 > > > > is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give > > > > us the correct file type. It is expected that for *nix compatibility > > > > reasons, the relation between S_IFx and DT_x will not change. For > > > > cases where the mode is invalid, upper layer validation catches this > > > > anyway, so this improves readability and arguably performance by > > > > assigning (mode & S_IFMT) >> 12 directly without any jump table > > > > or conditional logic. > > > > > > Shouldn't we also do this for other filesystems? A quick scan suggests > > > someone should at least look at xfs, ubifs, squashfs, romfs, ocfs2, > > > nilfs2, hfsplus, f2fs, ext4, ext2, exofs, coda, cifs & btrfs. > > > > > > > I've tried to do that 2 years ago, not even for readability, but to > > fix a lurking > > bug in conversion table implementation that was copy&pasted from ext2 to > > many other fs: > > https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2 > > https://marc.info/?l=linux-fsdevel&w=2&r=1&s=amir73il+file+type+conversion&q=b > > > > The push back from ext4/xfs maintainers was that while all fs use common > > values to encode d_type in on disk format, those on-disk format values > > are private to the fs. > > > > Eventually, the fix that got merged (only to xfs) did the opposite, it converted > > the conversion table lookup to a switch statement: > > https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 > > > > Some fs maintainers were happy about the initial version, but I did not > > pursue pushing that cleanup: > > https://marc.info/?l=linux-fsdevel&m=148243866204444&w=2 > > > > Phillip, you are welcome to re-spin those patches if you like. > > Note that the generic conversion helpers do not rely on upper layer > > to catch invalid mode values. > > > > Cheers, > > Amir. > > I only changed this code in the first place because of the comment in the code - > I was looking for things to do basically, and this caught my eye because I play > with FreeBSD now and then which of course uses UFS. I didn't consider any use > to the other filesystems - by re-spin do you just mean fix up and make sure > the kernel still builds etc? > Yes. If you are looking for a cleanup task, you can apply relevant patches from my series, starting with: https://patchwork.kernel.org/patch/9481237/ (Leave the xfs patch [11/11] out) But besides verifying that patches still apply and build, you will need to address the concerns of fs maintainers. Take for example the btrfs patch: https://patchwork.kernel.org/patch/9480725/ It says: + * + * Values 0..7 should match common file type values in file_type.h. */ #define BTRFS_FT_UNKNOWN 0 #define BTRFS_FT_REG_FILE 1 But that is not enough. When converting code to use the generic defines FT_*, instead of filesystem defined we need to leave in the code build time assertions that will catch an attempt to change fs contancts in the future, e.g.: static inline u8 btrfs_inode_type(struct inode *inode) { - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT]; + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN); + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE); ... + return fs_umode_to_ftype(inode->i_mode); } Same should be done for all relevant filesystems. Then you need to hope that fs maintainers will like this cleanup and want to take the patches ;-) Cheers, Amir.
On Sun, Oct 21, 2018 at 02:02:57PM +0300, Amir Goldstein wrote: > Yes. If you are looking for a cleanup task, you can > apply relevant patches from my series, starting with: > https://patchwork.kernel.org/patch/9481237/ > (Leave the xfs patch [11/11] out) > > But besides verifying that patches still apply and build, > you will need to address the concerns of fs maintainers. > Take for example the btrfs patch: > https://patchwork.kernel.org/patch/9480725/ > > It says: > + * > + * Values 0..7 should match common file type values in file_type.h. > */ > #define BTRFS_FT_UNKNOWN 0 > #define BTRFS_FT_REG_FILE 1 > > But that is not enough. > When converting code to use the generic defines FT_*, instead of > filesystem defined we need to leave in the code build time assertions > that will catch an attempt to change fs contancts in the future, e.g.: > > static inline u8 btrfs_inode_type(struct inode *inode) > { > - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT]; > + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN); > + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE); > ... > + return fs_umode_to_ftype(inode->i_mode); > } > > Same should be done for all relevant filesystems. > Then you need to hope that fs maintainers will like this cleanup and > want to take the patches ;-) > > Cheers, > Amir. Dear Amir, I will give it a go and see how far I get :-) Regards, Phil
diff --git a/fs/ufs/util.h b/fs/ufs/util.h index 1fd3011ea623..7e0c0878b9f9 100644 --- a/fs/ufs/util.h +++ b/fs/ufs/util.h @@ -16,6 +16,7 @@ * some useful macros */ #define in_range(b,first,len) ((b)>=(first)&&(b)<(first)+(len)) +#define S_SHIFT 12 /* * functions used for retyping @@ -158,34 +159,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 = (mode & S_IFMT) >> S_SHIFT; } static inline u32
Remove switch statement from ufs_set_de_type function in fs/ufs/util.h header and replace with simple assignment. For each case, S_IFx >> 12 is equal to DT_x, so in valid cases (mode & S_IFMT) >> 12 should give us the correct file type. It is expected that for *nix compatibility reasons, the relation between S_IFx and DT_x will not change. For cases where the mode is invalid, upper layer validation catches this anyway, so this improves readability and arguably performance by assigning (mode & S_IFMT) >> 12 directly without any jump table or conditional logic. Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- fs/ufs/util.h | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-)