[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[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. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.

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.0

RE: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[David Laight]

From: Phillip Potter
> Sent: 17 October 2018 11:08
> 
> 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. For invalid cases, upper layer validation
> catches this anyway, so this improves readability and arguably
> performance by assigning (mode & S_IFMT) >> 12 directly.
> 
...
> -	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;

This requires that the two sets of constants are correctly aligned.
If they aren't defined in terms of each other then you probably
ought to add compile-time asserts that the values match.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Al Viro]

On Wed, Oct 17, 2018 at 10:11:47AM +0000, David Laight wrote:
> From: Phillip Potter
> > Sent: 17 October 2018 11:08
> > 
> > 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. For invalid cases, upper layer validation
> > catches this anyway, so this improves readability and arguably
> > performance by assigning (mode & S_IFMT) >> 12 directly.
> > 
> ...
> > -	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;
> 
> This requires that the two sets of constants are correctly aligned.

They are.  BSD folks had (sanely, IMO) put the 'type' bits of st_mode
into directory entry verbatim.  Again, "symbolic constant" != "can be
expected to change"...  If, e.g., some port decides to change S_IFIFO,
they'll have no end of fun accessing ext*, xfs, ufs, etc. since that
value is stored in the on-disk inode and in effect pinned down by
that.

All S_... constants are universal and going to remain unchanged on
any Unices.

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Phillip Potter]

On Thu, Oct 18, 2018 at 12:33:05AM +0100, Al Viro wrote:
> They are.  BSD folks had (sanely, IMO) put the 'type' bits of st_mode
> into directory entry verbatim.  Again, "symbolic constant" != "can be
> expected to change"...  If, e.g., some port decides to change S_IFIFO,
> they'll have no end of fun accessing ext*, xfs, ufs, etc. since that
> value is stored in the on-disk inode and in effect pinned down by
> that.
> 
> All S_... constants are universal and going to remain unchanged on
> any Unices.

Dear Al,

Shall I resubmit without the compile-time checks in the latest patch then?

Regards,
Phil

[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[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.

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

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Matthew Wilcox]

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
> 

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Al Viro]

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?

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Amir Goldstein]

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.

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Phillip Potter]

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

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Amir Goldstein]

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.

Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[Phillip Potter]

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

[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[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. This is verified at compile-time with
BUILD_BUG_ON macro calls. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.

Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 fs/ufs/util.h | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 1fd3011ea623..dcf86829edc2 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,16 @@ 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;
-	}
+	/* Compile-time assertions that S_IFx >> S_SHIFT == DT_x */
+	BUILD_BUG_ON(S_IFIFO >> S_SHIFT != DT_FIFO);
+	BUILD_BUG_ON(S_IFCHR >> S_SHIFT != DT_CHR);
+	BUILD_BUG_ON(S_IFDIR >> S_SHIFT != DT_DIR);
+	BUILD_BUG_ON(S_IFBLK >> S_SHIFT != DT_BLK);
+	BUILD_BUG_ON(S_IFREG >> S_SHIFT != DT_REG);
+	BUILD_BUG_ON(S_IFLNK >> S_SHIFT != DT_LNK);
+	BUILD_BUG_ON(S_IFSOCK >> S_SHIFT != DT_SOCK);
+
+	de->d_u.d_44.d_type = (mode & S_IFMT) >> S_SHIFT;
 }
 
 static inline u32
-- 
2.17.2

[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[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. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.

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.0

[PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function

[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. For invalid cases, upper layer validation
catches this anyway, so this improves readability and arguably
performance by assigning (mode & S_IFMT) >> 12 directly.

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.0