fs: ufs: Remove switch statement from ufs_set_de_type function

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 <[email protected]>

---
 fs/ufs/util.h | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

fs/ufs/util.h +2 -28

index 1fd3011ea623..7e0c0878b9f9 100644
@@ -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

Signature · 1 line

2.17.2

Quoted 9 lines · Phillip Potter

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.

Quoted 9 lines · Phillip Potter

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.

Quoted 12 lines · Matthew Wilcox

Quoted 9 lines · Phillip Potter

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.

Quoted 4 lines

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.

Quoted 41 lines · Amir Goldstein

Quoted 12 lines · Matthew Wilcox

Quoted 9 lines · Phillip Potter

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.

Quoted 4 lines

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.

Quoted 2 lines

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.