On Sun, Oct 21, 2018 at 12:57 PM Phillip Potter <phil@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.
