[PATCH] fs/ufs: Use LUT for dir entry types

[PATCH] fs/ufs: Use LUT for dir entry types

[Luis Felipe Hernandez]

As per the original TODO, replacing the switch statement with a lookup
table results in more concise mapping logic and improved performance.

ufs_mode_to_dt maps file mode bits to directory entry types. Indices are
created by right shifting 12 bits to isolate the higher-order bits in
order to use smaller indexing values.
e.g. S_IFSOCK >> FT_SHIFT => 0140000 >> 12 = 1100

Signed-off-by: Luis Felipe Hernandez <luis.hernandez093@gmail.com>
---
 fs/ufs/util.h | 46 +++++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/fs/ufs/util.h b/fs/ufs/util.h
index 0ecd2ed792f5..caa843373191 100644
--- a/fs/ufs/util.h
+++ b/fs/ufs/util.h
@@ -7,10 +7,13 @@
  * Charles University, Faculty of Mathematics and Physics
  */
 
+#include <linux/array_size.h>
 #include <linux/buffer_head.h>
 #include <linux/fs.h>
 #include "swab.h"
 
+#define FT_SHIFT 12
+
 /*
  * functions used for retyping
  */
@@ -146,40 +149,29 @@ ufs_set_de_namlen(struct super_block *sb, struct ufs_dir_entry *de, u16 value)
 		de->d_u.d_44.d_namlen = value; /* XXX this seems wrong */
 }
 
+static const unsigned char ufs_mode_to_dt[] = {
+	[S_IFSOCK >> FT_SHIFT] = DT_SOCK,
+	[S_IFLNK >> FT_SHIFT] = DT_LNK,
+	[S_IFREG >> FT_SHIFT] = DT_REG,
+	[S_IFBLK >> FT_SHIFT] = DT_BLK,
+	[S_IFDIR >> FT_SHIFT] = DT_DIR,
+	[S_IFCHR >> FT_SHIFT] = DT_CHR,
+	[S_IFIFO >> FT_SHIFT] = DT_FIFO,
+	[0] = DT_UNKNOWN
+};
+
 static inline void
 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:
+	unsigned int mode_index = (mode & S_IFMT) >> FT_SHIFT;
+
+	if (mode_index < ARRAY_SIZE(ufs_mode_to_dt))
+		de->d_u.d_44.d_type = ufs_mode_to_dt[mode_index];
+	else
 		de->d_u.d_44.d_type = DT_UNKNOWN;
-	}
 }
 
 static inline u32
-- 
2.45.2

Re: [PATCH] fs/ufs: Use LUT for dir entry types

[Markus Elfring]

> As per the original TODO, replacing the switch statement with a lookup
> table results in more concise mapping logic and improved performance.
…

Can imperative wordings be relevant for another improved change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94


…
> +++ b/fs/ufs/util.h
…
>  static inline void
>  ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
…
> +	if (mode_index < ARRAY_SIZE(ufs_mode_to_dt))
> +		de->d_u.d_44.d_type = ufs_mode_to_dt[mode_index];
> +	else
>  		de->d_u.d_44.d_type = DT_UNKNOWN;
…

May a conditional operator expression be applied at this source code place?

Regards,
Markus

Re: [PATCH] fs/ufs: Use LUT for dir entry types

[Greg KH]

On Mon, Jun 10, 2024 at 09:48:09AM +0200, Markus Elfring wrote:
> > As per the original TODO, replacing the switch statement with a lookup
> > table results in more concise mapping logic and improved performance.
> …
> 
> Can imperative wordings be relevant for another improved change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94
> 
> 
> …
> > +++ b/fs/ufs/util.h
> …
> >  static inline void
> >  ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
> …
> > +	if (mode_index < ARRAY_SIZE(ufs_mode_to_dt))
> > +		de->d_u.d_44.d_type = ufs_mode_to_dt[mode_index];
> > +	else
> >  		de->d_u.d_44.d_type = DT_UNKNOWN;
> …
> 
> May a conditional operator expression be applied at this source code place?
> 
> Regards,
> Markus
> 

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot

Re: [PATCH] fs/ufs: Use LUT for dir entry types

[Luis Felipe Hernandez]

On Mon, Jun 10, 2024 at 10:00:18AM +0200, Greg KH wrote:
> On Mon, Jun 10, 2024 at 09:48:09AM +0200, Markus Elfring wrote:
> > > As per the original TODO, replacing the switch statement with a lookup
> > > table results in more concise mapping logic and improved performance.
> > …
> > 
> > Can imperative wordings be relevant for another improved change description?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc2#n94
> > 
> > 
> > …
> > > +++ b/fs/ufs/util.h
> > …
> > >  static inline void
> > >  ufs_set_de_type(struct super_block *sb, struct ufs_dir_entry *de, int mode)
> > …
> > > +	if (mode_index < ARRAY_SIZE(ufs_mode_to_dt))
> > > +		de->d_u.d_44.d_type = ufs_mode_to_dt[mode_index];
> > > +	else
> > >  		de->d_u.d_44.d_type = DT_UNKNOWN;
> > …
> > 
> > May a conditional operator expression be applied at this source code place?
> > 
> > Regards,
> > Markus
> > 
> 
> Hi,
> 
> This is the semi-friendly patch-bot of Greg Kroah-Hartman.
> 
> Markus, you seem to have sent a nonsensical or otherwise pointless
> review comment to a patch submission on a Linux kernel developer mailing
> list.  I strongly suggest that you not do this anymore.  Please do not
> bother developers who are actively working to produce patches and
> features with comments that, in the end, are a waste of time.
> 
> Patch submitter, please ignore Markus's suggestion; you do not need to
> follow it at all.  The person/bot/AI that sent it is being ignored by
> almost all Linux kernel maintainers for having a persistent pattern of
> behavior of producing distracting and pointless commentary, and
> inability to adapt to feedback.  Please feel free to also ignore emails
> from them.
> 
> thanks,
> 
> greg k-h's patch email bot

Hi Evgeniy,

I am writing to follow up on my patch submission from June 9th. The patch addresses a TODO, "turn this (switch statement) into a table lookup", originally introduced on 2005-04-16. I believe the original commit message body may have not been written in the correct form and wanted to offer an improved version:

"Replace the switch statement with a lookup table (LUT) as suggested in the original TODO. This change results in more concise mapping logic and improves performance.

The ufs_mode_to_dt function now maps file mode bits to directory entry types using a LUT. Indices are created by right shifting the mode bits by 12 (FT_SHIFT), isolating the higher-order bits for indexing.

This enhancement simplifies the code and optimizes the mapping process."

Any feedback or guidance on the patch would be greatly appreciated.

Thank you for your time and consideration.

Best regards,

Felipe