Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
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
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
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
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Sun, Oct 21, 2018 at 12:57 PM Phillip Potter 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 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 from ext2 to
> > many other fs:
> > https://marc.info/?l=linux-fsdevel=148217829301701=2
> > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2
> >
> > Some fs maintainers were happy about the initial version, but I did not
> > pursue pushing that cleanup:
> > https://marc.info/?l=linux-fsdevel=14824386620=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
On Sun, Oct 21, 2018 at 12:57 PM Phillip Potter 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 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 from ext2 to
> > many other fs:
> > https://marc.info/?l=linux-fsdevel=148217829301701=2
> > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2
> >
> > Some fs maintainers were happy about the initial version, but I did not
> > pursue pushing that cleanup:
> > https://marc.info/?l=linux-fsdevel=14824386620=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
On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > On Sun, Oct 21, 2018 at 1:27 AM 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. > > > > 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 from ext2 to > many other fs: > https://marc.info/?l=linux-fsdevel=148217829301701=2 > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2 > > Some fs maintainers were happy about the initial version, but I did not > pursue pushing that cleanup: > https://marc.info/?l=linux-fsdevel=14824386620=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
On Sun, Oct 21, 2018 at 08:30:35AM +0300, Amir Goldstein wrote: > On Sun, Oct 21, 2018 at 1:27 AM 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. > > > > 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 from ext2 to > many other fs: > https://marc.info/?l=linux-fsdevel=148217829301701=2 > https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2 > > Some fs maintainers were happy about the initial version, but I did not > pursue pushing that cleanup: > https://marc.info/?l=linux-fsdevel=14824386620=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
On Sun, Oct 21, 2018 at 1:27 AM 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. > 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 from ext2 to many other fs: https://marc.info/?l=linux-fsdevel=148217829301701=2 https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2 Some fs maintainers were happy about the initial version, but I did not pursue pushing that cleanup: https://marc.info/?l=linux-fsdevel=14824386620=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
On Sun, Oct 21, 2018 at 1:27 AM 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. > 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 from ext2 to many other fs: https://marc.info/?l=linux-fsdevel=148217829301701=2 https://marc.info/?l=linux-fsdevel=2=1=amir73il+file+type+conversion=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=14824386620=2 Some fs maintainers were happy about the initial version, but I did not pursue pushing that cleanup: https://marc.info/?l=linux-fsdevel=14824386620=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
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
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
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
> ---
> 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
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
> ---
> 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
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
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
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
Re: [PATCH] fs: ufs: Remove switch statement from ufs_set_de_type function
On Wed, Oct 17, 2018 at 10:11:47AM +, 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
On Wed, Oct 17, 2018 at 10:11:47AM +, 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
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
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)
