* [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
@ 2018-10-09 13:16 Phillip Potter
0 siblings, 0 replies; 14+ messages in thread
From: Phillip Potter @ 2018-10-09 13:16 UTC (permalink / raw)
To: dushistov; +Cc: linux-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-21 11:02 ` Amir Goldstein
@ 2018-10-22 8:20 ` Phillip Potter
0 siblings, 0 replies; 14+ messages in thread
From: Phillip Potter @ 2018-10-22 8:20 UTC (permalink / raw)
To: Amir Goldstein
Cc: willy, dushistov, viro, David.Laight, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-21 9:57 ` Phillip Potter
@ 2018-10-21 11:02 ` Amir Goldstein
2018-10-22 8:20 ` Phillip Potter
0 siblings, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2018-10-21 11:02 UTC (permalink / raw)
To: Phillip Potter
Cc: Matthew Wilcox, Evgeniy Dushistov, Al Viro, David.Laight,
linux-kernel, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-21 5:30 ` Amir Goldstein
@ 2018-10-21 9:57 ` Phillip Potter
2018-10-21 11:02 ` Amir Goldstein
0 siblings, 1 reply; 14+ messages in thread
From: Phillip Potter @ 2018-10-21 9:57 UTC (permalink / raw)
To: Amir Goldstein
Cc: willy, dushistov, viro, David.Laight, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-20 22:26 ` Matthew Wilcox
2018-10-20 23:07 ` Al Viro
@ 2018-10-21 5:30 ` Amir Goldstein
2018-10-21 9:57 ` Phillip Potter
1 sibling, 1 reply; 14+ messages in thread
From: Amir Goldstein @ 2018-10-21 5:30 UTC (permalink / raw)
To: Matthew Wilcox
Cc: phil, Evgeniy Dushistov, Al Viro, David.Laight, linux-kernel,
linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-20 22:26 ` Matthew Wilcox
@ 2018-10-20 23:07 ` Al Viro
2018-10-21 5:30 ` Amir Goldstein
1 sibling, 0 replies; 14+ messages in thread
From: Al Viro @ 2018-10-20 23:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Phillip Potter, dushistov, David.Laight, linux-kernel, linux-fsdevel
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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-20 22:09 Phillip Potter
@ 2018-10-20 22:26 ` Matthew Wilcox
2018-10-20 23:07 ` Al Viro
2018-10-21 5:30 ` Amir Goldstein
0 siblings, 2 replies; 14+ messages in thread
From: Matthew Wilcox @ 2018-10-20 22:26 UTC (permalink / raw)
To: Phillip Potter; +Cc: dushistov, viro, David.Laight, linux-kernel, linux-fsdevel
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
@ 2018-10-20 22:09 Phillip Potter
2018-10-20 22:26 ` Matthew Wilcox
0 siblings, 1 reply; 14+ messages in thread
From: Phillip Potter @ 2018-10-20 22:09 UTC (permalink / raw)
To: dushistov; +Cc: viro, David.Laight, linux-kernel, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-17 23:33 ` Al Viro
@ 2018-10-18 10:19 ` Phillip Potter
0 siblings, 0 replies; 14+ messages in thread
From: Phillip Potter @ 2018-10-18 10:19 UTC (permalink / raw)
To: Al Viro; +Cc: dushistov, David.Laight, linux-kernel, linux-fsdevel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-17 10:11 ` David Laight
@ 2018-10-17 23:33 ` Al Viro
2018-10-18 10:19 ` Phillip Potter
0 siblings, 1 reply; 14+ messages in thread
From: Al Viro @ 2018-10-17 23:33 UTC (permalink / raw)
To: David Laight
Cc: 'Phillip Potter', dushistov, linux-kernel, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
@ 2018-10-17 12:34 Phillip Potter
0 siblings, 0 replies; 14+ messages in thread
From: Phillip Potter @ 2018-10-17 12:34 UTC (permalink / raw)
To: dushistov, David.Laight; +Cc: linux-kernel, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
2018-10-17 10:08 Phillip Potter
@ 2018-10-17 10:11 ` David Laight
2018-10-17 23:33 ` Al Viro
0 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2018-10-17 10:11 UTC (permalink / raw)
To: 'Phillip Potter', dushistov; +Cc: linux-kernel, linux-fsdevel
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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
@ 2018-10-17 10:08 Phillip Potter
2018-10-17 10:11 ` David Laight
0 siblings, 1 reply; 14+ messages in thread
From: Phillip Potter @ 2018-10-17 10:08 UTC (permalink / raw)
To: dushistov; +Cc: linux-kernel, linux-fsdevel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
@ 2018-10-02 16:41 Phillip Potter
0 siblings, 0 replies; 14+ messages in thread
From: Phillip Potter @ 2018-10-02 16:41 UTC (permalink / raw)
To: dushistov; +Cc: linux-kernel, viro
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-10-22 8:20 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 13:16 [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function Phillip Potter
-- strict thread matches above, loose matches on Subject: below --
2018-10-20 22:09 Phillip Potter
2018-10-20 22:26 ` Matthew Wilcox
2018-10-20 23:07 ` Al Viro
2018-10-21 5:30 ` Amir Goldstein
2018-10-21 9:57 ` Phillip Potter
2018-10-21 11:02 ` Amir Goldstein
2018-10-22 8:20 ` Phillip Potter
2018-10-17 12:34 Phillip Potter
2018-10-17 10:08 Phillip Potter
2018-10-17 10:11 ` David Laight
2018-10-17 23:33 ` Al Viro
2018-10-18 10:19 ` Phillip Potter
2018-10-02 16:41 Phillip Potter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.