* [RFC][PATCH 00/11] common implementation of dirent file types @ 2018-10-23 20:19 Phillip Potter 2018-10-23 21:17 ` [RFC][PATCH v2 10/10] btrfs: use common file type conversion Phillip Potter 2018-10-24 6:43 ` [RFC][PATCH 00/11] common implementation of dirent file types Amir Goldstein 0 siblings, 2 replies; 8+ messages in thread From: Phillip Potter @ 2018-10-23 20:19 UTC (permalink / raw) To: viro; +Cc: linux-kernel, amir73il, linux-fsdevel This cleanup series is a respin of Amir Goldstein's work, created in late 2016. It removes several instances of duplicated code. Most of the duplication dates back to git pre-historic era. The controversial aspect of this cleanup is that it uses common code to handle file system specific on-disk format bits. All 7 file systems use a single byte to store dirent file type on-disk and all of them use the same conversion routines from i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants. Patch 1 places a common implementation in file_type.h and add some useful conversion helpers. Patches 2-3 make use of some helpers in ufs and hfsplus without any on-disk implications. Patches 4-10 replace the specific implementations in ext2, exofs, ext4, ocfs2, f2fs, nilfs and btrfs with the common implementation. These patches also now include compile-time checks to ensure that the file system specific on-disk bits are equivalent to the common implementation FT_* bits. These compile-time checks are only included once per file system, as my reasoning is that regardless of their location, the build will fail/succeed anyway. In addition, where needed (for patches which no longer apply), I've rebased them to apply to the newest 4.19 kernel sources. Each patch is independent of the others, except for the common implementation itself which they all depend on. I would love feedback as newish kernel contributor, particularly from the original author Amir who suggested this task for me. I hope the various subsystem/fs maintainers will see fit to accept this work also. Phillip Potter (10): fs: common implementation of file type conversions ufs: use fs_umode_to_dtype() helper hfsplus: use fs_umode_to_dtype() helper ext2: use common file type conversion exofs: use common file type conversion ext4: use common file type conversion ocfs2: use common file type conversion f2fs: use common file type conversion nilfs2: use common file type conversion btrfs: use common file type conversion --- MAINTAINERS | 1 + fs/btrfs/btrfs_inode.h | 2 - fs/btrfs/delayed-inode.c | 2 +- fs/btrfs/inode.c | 35 +++++----- fs/exofs/dir.c | 49 +++++-------- fs/ext2/dir.c | 51 ++++++-------- fs/ext4/ext4.h | 35 +++++----- fs/f2fs/dir.c | 43 +++++------- fs/f2fs/inline.c | 2 +- fs/hfsplus/dir.c | 16 +---- fs/nilfs2/dir.c | 54 +++++---------- fs/ocfs2/dir.c | 32 +++++---- fs/ocfs2/ocfs2_fs.h | 13 +--- fs/ufs/util.h | 29 +------- include/linux/f2fs_fs.h | 8 ++- include/linux/file_type.h | 108 +++++++++++++++++++++++++++++ include/linux/fs.h | 17 +---- include/uapi/linux/btrfs_tree.h | 2 + include/uapi/linux/nilfs2_ondisk.h | 1 + 19 files changed, 256 insertions(+), 244 deletions(-) create mode 100644 include/linux/file_type.h -- 2.17.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC][PATCH v2 10/10] btrfs: use common file type conversion @ 2018-10-23 21:17 ` Phillip Potter 2018-10-24 10:11 ` David Sterba 0 siblings, 1 reply; 8+ messages in thread From: Phillip Potter @ 2018-10-23 21:17 UTC (permalink / raw) To: clm Cc: linux-kernel, amir73il, viro, jbacik, dsterba, linux-fsdevel, linux-btrfs Deduplicate the btrfs file type conversion implementation. Original patch by Amir Goldstein. v2: - Rebased against Linux 4.19 by Phillip Potter - Compile-time checks added by Phillip Potter to make sure the BTRFS_FT_x enum values stay same as FT_x values v1: - Initial implementation Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- fs/btrfs/btrfs_inode.h | 2 -- fs/btrfs/delayed-inode.c | 2 +- fs/btrfs/inode.c | 35 +++++++++++++++++---------------- include/uapi/linux/btrfs_tree.h | 2 ++ 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index 1343ac57b438..c7c6db6b4a35 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -196,8 +196,6 @@ struct btrfs_inode { struct inode vfs_inode; }; -extern unsigned char btrfs_filetype_table[]; - static inline struct btrfs_inode *BTRFS_I(const struct inode *inode) { return container_of(inode, struct btrfs_inode, vfs_inode); diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index f51b509f2d9b..c1da34e3a775 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -1691,7 +1691,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx, name = (char *)(di + 1); name_len = btrfs_stack_dir_name_len(di); - d_type = btrfs_filetype_table[di->type]; + d_type = fs_dtype(di->type); btrfs_disk_key_to_cpu(&location, &di->location); over = !dir_emit(ctx, name, name_len, diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 3ea5339603cf..4cbdcba4799a 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -73,17 +73,6 @@ struct kmem_cache *btrfs_trans_handle_cachep; struct kmem_cache *btrfs_path_cachep; struct kmem_cache *btrfs_free_space_cachep; -#define S_SHIFT 12 -static const unsigned char btrfs_type_by_mode[S_IFMT >> S_SHIFT] = { - [S_IFREG >> S_SHIFT] = BTRFS_FT_REG_FILE, - [S_IFDIR >> S_SHIFT] = BTRFS_FT_DIR, - [S_IFCHR >> S_SHIFT] = BTRFS_FT_CHRDEV, - [S_IFBLK >> S_SHIFT] = BTRFS_FT_BLKDEV, - [S_IFIFO >> S_SHIFT] = BTRFS_FT_FIFO, - [S_IFSOCK >> S_SHIFT] = BTRFS_FT_SOCK, - [S_IFLNK >> S_SHIFT] = BTRFS_FT_SYMLINK, -}; - static int btrfs_setsize(struct inode *inode, struct iattr *attr); static int btrfs_truncate(struct inode *inode, bool skip_writeback); static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent); @@ -5803,10 +5792,6 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, return d_splice_alias(inode, dentry); } -unsigned char btrfs_filetype_table[] = { - DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK -}; - /* * All this infrastructure exists because dir_emit can fault, and we are holding * the tree lock when doing readdir. For now just allocate a buffer and copy @@ -5945,7 +5930,23 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx) name_ptr = (char *)(entry + 1); read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1), name_len); - put_unaligned(btrfs_filetype_table[btrfs_dir_type(leaf, di)], + + /* + * compile-time asserts that generic FT_x types + * still match BTRFS_FT_x types - no need to list + * in other functions as well as build will + * fail either way + */ + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN); + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE); + BUILD_BUG_ON(BTRFS_FT_DIR != FT_DIR); + BUILD_BUG_ON(BTRFS_FT_CHRDEV != FT_CHRDEV); + BUILD_BUG_ON(BTRFS_FT_BLKDEV != FT_BLKDEV); + BUILD_BUG_ON(BTRFS_FT_FIFO != FT_FIFO); + BUILD_BUG_ON(BTRFS_FT_SOCK != FT_SOCK); + BUILD_BUG_ON(BTRFS_FT_SYMLINK != FT_SYMLINK); + + put_unaligned(fs_dtype(btrfs_dir_type(leaf, di)), &entry->type); btrfs_dir_item_key_to_cpu(leaf, di, &location); put_unaligned(location.objectid, &entry->ino); @@ -6350,7 +6351,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans, static inline u8 btrfs_inode_type(struct inode *inode) { - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT]; + return fs_umode_to_ftype(inode->i_mode); } /* diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h index aff1356c2bb8..5747cffa09fa 100644 --- a/include/uapi/linux/btrfs_tree.h +++ b/include/uapi/linux/btrfs_tree.h @@ -307,6 +307,8 @@ * * Used by: * struct btrfs_dir_item.type + * + * Values 0..7 should match common file type values in file_type.h. */ #define BTRFS_FT_UNKNOWN 0 #define BTRFS_FT_REG_FILE 1 -- 2.17.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH v2 10/10] btrfs: use common file type conversion 2018-10-23 21:17 ` [RFC][PATCH v2 10/10] btrfs: use common file type conversion Phillip Potter @ 2018-10-24 10:11 ` David Sterba 2018-10-24 13:28 ` Phillip Potter 0 siblings, 1 reply; 8+ messages in thread From: David Sterba @ 2018-10-24 10:11 UTC (permalink / raw) To: Phillip Potter Cc: clm, linux-kernel, amir73il, viro, jbacik, dsterba, linux-fsdevel, linux-btrfs On Tue, Oct 23, 2018 at 10:17:28PM +0100, Phillip Potter wrote: > Deduplicate the btrfs file type conversion implementation. The per-filesystem changelogs need a brief explanation why this is done, like "Filesystems that use the same filetypes as defined by POSIX do not need to define their own versions and use the common defines in file_type.h", rephrase at you like. > Original patch by Amir Goldstein. > > v2: > - Rebased against Linux 4.19 by Phillip Potter > - Compile-time checks added by Phillip Potter to make > sure the BTRFS_FT_x enum values stay same as FT_x values > > v1: > - Initial implementation > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > --- > fs/btrfs/btrfs_inode.h | 2 -- > fs/btrfs/delayed-inode.c | 2 +- > fs/btrfs/inode.c | 35 +++++++++++++++++---------------- > include/uapi/linux/btrfs_tree.h | 2 ++ > 4 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index 1343ac57b438..c7c6db6b4a35 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -196,8 +196,6 @@ struct btrfs_inode { > struct inode vfs_inode; > }; > > -extern unsigned char btrfs_filetype_table[]; > - > static inline struct btrfs_inode *BTRFS_I(const struct inode *inode) > { > return container_of(inode, struct btrfs_inode, vfs_inode); > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c > index f51b509f2d9b..c1da34e3a775 100644 > --- a/fs/btrfs/delayed-inode.c > +++ b/fs/btrfs/delayed-inode.c > @@ -1691,7 +1691,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx, > name = (char *)(di + 1); > name_len = btrfs_stack_dir_name_len(di); > > - d_type = btrfs_filetype_table[di->type]; > + d_type = fs_dtype(di->type); > btrfs_disk_key_to_cpu(&location, &di->location); > > over = !dir_emit(ctx, name, name_len, > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 3ea5339603cf..4cbdcba4799a 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -73,17 +73,6 @@ struct kmem_cache *btrfs_trans_handle_cachep; > struct kmem_cache *btrfs_path_cachep; > struct kmem_cache *btrfs_free_space_cachep; > > -#define S_SHIFT 12 > -static const unsigned char btrfs_type_by_mode[S_IFMT >> S_SHIFT] = { > - [S_IFREG >> S_SHIFT] = BTRFS_FT_REG_FILE, > - [S_IFDIR >> S_SHIFT] = BTRFS_FT_DIR, > - [S_IFCHR >> S_SHIFT] = BTRFS_FT_CHRDEV, > - [S_IFBLK >> S_SHIFT] = BTRFS_FT_BLKDEV, > - [S_IFIFO >> S_SHIFT] = BTRFS_FT_FIFO, > - [S_IFSOCK >> S_SHIFT] = BTRFS_FT_SOCK, > - [S_IFLNK >> S_SHIFT] = BTRFS_FT_SYMLINK, > -}; > - > static int btrfs_setsize(struct inode *inode, struct iattr *attr); > static int btrfs_truncate(struct inode *inode, bool skip_writeback); > static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent); > @@ -5803,10 +5792,6 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, > return d_splice_alias(inode, dentry); > } > > -unsigned char btrfs_filetype_table[] = { > - DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK > -}; > - > /* > * All this infrastructure exists because dir_emit can fault, and we are holding > * the tree lock when doing readdir. For now just allocate a buffer and copy > @@ -5945,7 +5930,23 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx) > name_ptr = (char *)(entry + 1); > read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1), > name_len); > - put_unaligned(btrfs_filetype_table[btrfs_dir_type(leaf, di)], > + > + /* > + * compile-time asserts that generic FT_x types > + * still match BTRFS_FT_x types - no need to list > + * in other functions as well as build will > + * fail either way > + */ Please format comments to 80 columns. > + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN); > + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE); > + BUILD_BUG_ON(BTRFS_FT_DIR != FT_DIR); > + BUILD_BUG_ON(BTRFS_FT_CHRDEV != FT_CHRDEV); > + BUILD_BUG_ON(BTRFS_FT_BLKDEV != FT_BLKDEV); > + BUILD_BUG_ON(BTRFS_FT_FIFO != FT_FIFO); > + BUILD_BUG_ON(BTRFS_FT_SOCK != FT_SOCK); > + BUILD_BUG_ON(BTRFS_FT_SYMLINK != FT_SYMLINK); > + > + put_unaligned(fs_dtype(btrfs_dir_type(leaf, di)), > &entry->type); > btrfs_dir_item_key_to_cpu(leaf, di, &location); > put_unaligned(location.objectid, &entry->ino); > @@ -6350,7 +6351,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans, > > static inline u8 btrfs_inode_type(struct inode *inode) > { > - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT]; > + return fs_umode_to_ftype(inode->i_mode); > } > > /* > diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h > index aff1356c2bb8..5747cffa09fa 100644 > --- a/include/uapi/linux/btrfs_tree.h > +++ b/include/uapi/linux/btrfs_tree.h > @@ -307,6 +307,8 @@ > * > * Used by: > * struct btrfs_dir_item.type > + * > + * Values 0..7 should match common file type values in file_type.h. I think it's rather 'must' than 'should' as there are the compile-time checks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH v2 10/10] btrfs: use common file type conversion 2018-10-24 10:11 ` David Sterba @ 2018-10-24 13:28 ` Phillip Potter 0 siblings, 0 replies; 8+ messages in thread From: Phillip Potter @ 2018-10-24 13:28 UTC (permalink / raw) To: David Sterba Cc: clm, amir73il, viro, jbacik, dsterba, linux-fsdevel, linux-btrfs On Wed, Oct 24, 2018 at 12:11:28PM +0200, David Sterba wrote: > On Tue, Oct 23, 2018 at 10:17:28PM +0100, Phillip Potter wrote: > > Deduplicate the btrfs file type conversion implementation. > > The per-filesystem changelogs need a brief explanation why this is done, > like "Filesystems that use the same filetypes as defined by POSIX do not > need to define their own versions and use the common defines in > file_type.h", rephrase at you like. > > > Original patch by Amir Goldstein. > > > > v2: > > - Rebased against Linux 4.19 by Phillip Potter > > - Compile-time checks added by Phillip Potter to make > > sure the BTRFS_FT_x enum values stay same as FT_x values > > > > v1: > > - Initial implementation > > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > --- > > fs/btrfs/btrfs_inode.h | 2 -- > > fs/btrfs/delayed-inode.c | 2 +- > > fs/btrfs/inode.c | 35 +++++++++++++++++---------------- > > include/uapi/linux/btrfs_tree.h | 2 ++ > > 4 files changed, 21 insertions(+), 20 deletions(-) > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > index 1343ac57b438..c7c6db6b4a35 100644 > > --- a/fs/btrfs/btrfs_inode.h > > +++ b/fs/btrfs/btrfs_inode.h > > @@ -196,8 +196,6 @@ struct btrfs_inode { > > struct inode vfs_inode; > > }; > > > > -extern unsigned char btrfs_filetype_table[]; > > - > > static inline struct btrfs_inode *BTRFS_I(const struct inode *inode) > > { > > return container_of(inode, struct btrfs_inode, vfs_inode); > > diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c > > index f51b509f2d9b..c1da34e3a775 100644 > > --- a/fs/btrfs/delayed-inode.c > > +++ b/fs/btrfs/delayed-inode.c > > @@ -1691,7 +1691,7 @@ int btrfs_readdir_delayed_dir_index(struct dir_context *ctx, > > name = (char *)(di + 1); > > name_len = btrfs_stack_dir_name_len(di); > > > > - d_type = btrfs_filetype_table[di->type]; > > + d_type = fs_dtype(di->type); > > btrfs_disk_key_to_cpu(&location, &di->location); > > > > over = !dir_emit(ctx, name, name_len, > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 3ea5339603cf..4cbdcba4799a 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -73,17 +73,6 @@ struct kmem_cache *btrfs_trans_handle_cachep; > > struct kmem_cache *btrfs_path_cachep; > > struct kmem_cache *btrfs_free_space_cachep; > > > > -#define S_SHIFT 12 > > -static const unsigned char btrfs_type_by_mode[S_IFMT >> S_SHIFT] = { > > - [S_IFREG >> S_SHIFT] = BTRFS_FT_REG_FILE, > > - [S_IFDIR >> S_SHIFT] = BTRFS_FT_DIR, > > - [S_IFCHR >> S_SHIFT] = BTRFS_FT_CHRDEV, > > - [S_IFBLK >> S_SHIFT] = BTRFS_FT_BLKDEV, > > - [S_IFIFO >> S_SHIFT] = BTRFS_FT_FIFO, > > - [S_IFSOCK >> S_SHIFT] = BTRFS_FT_SOCK, > > - [S_IFLNK >> S_SHIFT] = BTRFS_FT_SYMLINK, > > -}; > > - > > static int btrfs_setsize(struct inode *inode, struct iattr *attr); > > static int btrfs_truncate(struct inode *inode, bool skip_writeback); > > static int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent); > > @@ -5803,10 +5792,6 @@ static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry, > > return d_splice_alias(inode, dentry); > > } > > > > -unsigned char btrfs_filetype_table[] = { > > - DT_UNKNOWN, DT_REG, DT_DIR, DT_CHR, DT_BLK, DT_FIFO, DT_SOCK, DT_LNK > > -}; > > - > > /* > > * All this infrastructure exists because dir_emit can fault, and we are holding > > * the tree lock when doing readdir. For now just allocate a buffer and copy > > @@ -5945,7 +5930,23 @@ static int btrfs_real_readdir(struct file *file, struct dir_context *ctx) > > name_ptr = (char *)(entry + 1); > > read_extent_buffer(leaf, name_ptr, (unsigned long)(di + 1), > > name_len); > > - put_unaligned(btrfs_filetype_table[btrfs_dir_type(leaf, di)], > > + > > + /* > > + * compile-time asserts that generic FT_x types > > + * still match BTRFS_FT_x types - no need to list > > + * in other functions as well as build will > > + * fail either way > > + */ > > Please format comments to 80 columns. > > > + BUILD_BUG_ON(BTRFS_FT_UNKNOWN != FT_UNKNOWN); > > + BUILD_BUG_ON(BTRFS_FT_REG_FILE != FT_REG_FILE); > > + BUILD_BUG_ON(BTRFS_FT_DIR != FT_DIR); > > + BUILD_BUG_ON(BTRFS_FT_CHRDEV != FT_CHRDEV); > > + BUILD_BUG_ON(BTRFS_FT_BLKDEV != FT_BLKDEV); > > + BUILD_BUG_ON(BTRFS_FT_FIFO != FT_FIFO); > > + BUILD_BUG_ON(BTRFS_FT_SOCK != FT_SOCK); > > + BUILD_BUG_ON(BTRFS_FT_SYMLINK != FT_SYMLINK); > > + > > + put_unaligned(fs_dtype(btrfs_dir_type(leaf, di)), > > &entry->type); > > btrfs_dir_item_key_to_cpu(leaf, di, &location); > > put_unaligned(location.objectid, &entry->ino); > > @@ -6350,7 +6351,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans, > > > > static inline u8 btrfs_inode_type(struct inode *inode) > > { > > - return btrfs_type_by_mode[(inode->i_mode & S_IFMT) >> S_SHIFT]; > > + return fs_umode_to_ftype(inode->i_mode); > > } > > > > /* > > diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h > > index aff1356c2bb8..5747cffa09fa 100644 > > --- a/include/uapi/linux/btrfs_tree.h > > +++ b/include/uapi/linux/btrfs_tree.h > > @@ -307,6 +307,8 @@ > > * > > * Used by: > > * struct btrfs_dir_item.type > > + * > > + * Values 0..7 should match common file type values in file_type.h. > > I think it's rather 'must' than 'should' as there are the compile-time > checks. Dear David, Good points, thank you, I will make these changes and republish as part of a new series. Regards, Phil ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH 00/11] common implementation of dirent file types 2018-10-23 20:19 [RFC][PATCH 00/11] common implementation of dirent file types Phillip Potter 2018-10-23 21:17 ` [RFC][PATCH v2 10/10] btrfs: use common file type conversion Phillip Potter @ 2018-10-24 6:43 ` Amir Goldstein 2018-10-25 11:08 ` Jan Kara 1 sibling, 1 reply; 8+ messages in thread From: Amir Goldstein @ 2018-10-24 6:43 UTC (permalink / raw) To: Phillip Potter; +Cc: Al Viro, linux-kernel, linux-fsdevel, Jan Kara On Tue, Oct 23, 2018 at 11:19 PM Phillip Potter <phil@philpotter.co.uk> wrote: > > This cleanup series is a respin of Amir Goldstein's work, created > in late 2016. It removes several instances of duplicated code. Most > of the duplication dates back to git pre-historic era. > > The controversial aspect of this cleanup is that it uses common > code to handle file system specific on-disk format bits. We handled this aspect with care by placing compile time checks. > All 7 file systems use a single byte to store dirent file type > on-disk and all of them use the same conversion routines from > i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants. > > Patch 1 places a common implementation in file_type.h and > add some useful conversion helpers. > > Patches 2-3 make use of some helpers in ufs and hfsplus > without any on-disk implications. > > Patches 4-10 replace the specific implementations in ext2, exofs, > ext4, ocfs2, f2fs, nilfs and btrfs with the common implementation. > These patches also now include compile-time checks to ensure that > the file system specific on-disk bits are equivalent to the common > implementation FT_* bits. These compile-time checks are only > included once per file system, as my reasoning is that regardless > of their location, the build will fail/succeed anyway. > > In addition, where needed (for patches which no longer apply), > I've rebased them to apply to the newest 4.19 kernel sources. > Each patch is independent of the others, except for the > common implementation itself which they all depend on. > This is still RFC, but since this is an exercise let's talk about social engineering. First (now) you want to gather ACKs and review comments from fs developers/maintainers. Once all comments are addressed and assuming no objections, you have two possible ways forward: 1. Wait for VFS maintainer to take the common code and patches ACKed by individual maintainers (Al practically maintains some of the small fs himself). Al's backlog is long, so this may be a long wait. 2. Ask a filesystem maintainer to carry the common patch along with his specific fs patch through his tree and let other maintainers apply the patch at their own will on the following development cycle. The ext2 tree would be a good candidate for such a move, if Jan accepts this change, considering that all other fs copied the buggy code from ext2. > I would love feedback as newish kernel contributor, particularly > from the original author Amir who suggested this task for me. I > hope the various subsystem/fs maintainers will see fit to accept > this work also. > I usually prefer to have the revisions log in the cover letter for a 10 patch series, much less work and more appropriate when revision log looks mostly the same on all patches, e.g.: v2: - Rebased against Linux 4.19 by Phillip Potter - This version does not remove filesystem specific XXX_FT_x definitions, as these values are now used in compile-time checks added by Phillip Potter to make sure they remain the same as FT_x values - Removed xfs patch (a variant of original patch has already been applied) - Anything else relevant to the context of the series... v1: - Initial implementation by Amir Goldstein: https://marc.info/?l=linux-fsdevel&m=148217829301701&w=2 > Phillip Potter (10): > fs: common implementation of file type conversions > ufs: use fs_umode_to_dtype() helper > hfsplus: use fs_umode_to_dtype() helper > ext2: use common file type conversion > exofs: use common file type conversion > ext4: use common file type conversion > ocfs2: use common file type conversion > f2fs: use common file type conversion > nilfs2: use common file type conversion > btrfs: use common file type conversion > --- > MAINTAINERS | 1 + > fs/btrfs/btrfs_inode.h | 2 - > fs/btrfs/delayed-inode.c | 2 +- > fs/btrfs/inode.c | 35 +++++----- > fs/exofs/dir.c | 49 +++++-------- > fs/ext2/dir.c | 51 ++++++-------- > fs/ext4/ext4.h | 35 +++++----- > fs/f2fs/dir.c | 43 +++++------- > fs/f2fs/inline.c | 2 +- > fs/hfsplus/dir.c | 16 +---- > fs/nilfs2/dir.c | 54 +++++---------- > fs/ocfs2/dir.c | 32 +++++---- > fs/ocfs2/ocfs2_fs.h | 13 +--- > fs/ufs/util.h | 29 +------- > include/linux/f2fs_fs.h | 8 ++- > include/linux/file_type.h | 108 +++++++++++++++++++++++++++++ > include/linux/fs.h | 17 +---- > include/uapi/linux/btrfs_tree.h | 2 + > include/uapi/linux/nilfs2_ondisk.h | 1 + > 19 files changed, 256 insertions(+), 244 deletions(-) > create mode 100644 include/linux/file_type.h > > -- > 2.17.2 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH 00/11] common implementation of dirent file types 2018-10-24 6:43 ` [RFC][PATCH 00/11] common implementation of dirent file types Amir Goldstein @ 2018-10-25 11:08 ` Jan Kara 2018-10-25 12:48 ` Phillip Potter 0 siblings, 1 reply; 8+ messages in thread From: Jan Kara @ 2018-10-25 11:08 UTC (permalink / raw) To: Amir Goldstein Cc: Phillip Potter, Al Viro, linux-kernel, linux-fsdevel, Jan Kara On Wed 24-10-18 09:43:21, Amir Goldstein wrote: > Once all comments are addressed and assuming no objections, you have > two possible ways forward: > > 1. Wait for VFS maintainer to take the common code and patches ACKed > by individual maintainers (Al practically maintains some of the small fs > himself). Al's backlog is long, so this may be a long wait. > > 2. Ask a filesystem maintainer to carry the common patch along with his > specific fs patch through his tree and let other maintainers apply the patch > at their own will on the following development cycle. > The ext2 tree would be a good candidate for such a move, if Jan accepts > this change, considering that all other fs copied the buggy code from ext2. I can take these patches through my tree if Al takes too long to respond (but he apparently already had a look at the patches which is a good sign ;) and if patches are reviewed by respective fs maintainers. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC][PATCH 00/11] common implementation of dirent file types 2018-10-25 11:08 ` Jan Kara @ 2018-10-25 12:48 ` Phillip Potter 0 siblings, 0 replies; 8+ messages in thread From: Phillip Potter @ 2018-10-25 12:48 UTC (permalink / raw) To: Jan Kara; +Cc: viro, linux-fsdevel, amir73il On Thu, Oct 25, 2018 at 01:08:04PM +0200, Jan Kara wrote: > On Wed 24-10-18 09:43:21, Amir Goldstein wrote: > > Once all comments are addressed and assuming no objections, you have > > two possible ways forward: > > > > 1. Wait for VFS maintainer to take the common code and patches ACKed > > by individual maintainers (Al practically maintains some of the small fs > > himself). Al's backlog is long, so this may be a long wait. > > > > 2. Ask a filesystem maintainer to carry the common patch along with his > > specific fs patch through his tree and let other maintainers apply the patch > > at their own will on the following development cycle. > > The ext2 tree would be a good candidate for such a move, if Jan accepts > > this change, considering that all other fs copied the buggy code from ext2. > > I can take these patches through my tree if Al takes too long to respond > (but he apparently already had a look at the patches which is a good sign > ;) and if patches are reviewed by respective fs maintainers. > > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR Dear Jan, Thank you - and thank you Al as well for taking a look :-) I will publish a new series later on (possibly tomorrow) with all the feedback I've gathered. Regards, Phil ^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC][PATCH 00/11] common implementation of dirent file types @ 2016-12-19 20:10 Amir Goldstein 0 siblings, 0 replies; 8+ messages in thread From: Amir Goldstein @ 2016-12-19 20:10 UTC (permalink / raw) To: Jan Kara, Theodore Ts'o, Dave Chinner Cc: Darrick J . Wong, Chris Mason, Boaz Harrosh, Jaegeuk Kim, Ryusuke Konishi, Mark Fasheh, Evgeniy Dushistov, Miklos Szeredi, Al Viro, linux-fsdevel This cleanup series removes 8 instances of duplicated code. Most of the duplication dates back to git pre-historic era. The controversial aspect of this cleanup is that it uses common code to handle file system specific on-disk format bits. All 8 file systems use a single byte to store dirent file type on-disk and all of them use the same conversion routines from i_mode to 4bits DT_* constants to 3bits on-disk FT_* constants. Patch 1 hoists a common implementation to file_type.h and add some useful conversion helpers. Patches 2-3 makes use of some helpers in ufs and hfsplus without any on-disk implications. Patches 4-3 replace the specific implementation in ext2 and exofs with the common implementation. In this case, the fs specific constants (EXT2_FT_*) are defined in local include file and never used, so the constants have been removed. Patches 5-6 replace the specific implementation in ext4 and ocfs2 with the common implementation. In this case, the fs specific constants (EXT4_FT_*) are defined in local include file that look like it is a version of an include file used in a library. The constants remained in the include file and were defined to refer to the common constants (EXT4_FT_DIR => FT_DIR). Patches 7-8 replace the specific implementation in f2fs and nilfs2 with the common implementation. In this case, the fs specific constants (F2FS_FT_*) are defined in a global include file. The constants remained in the include file a comment was added to keep them in sync with the common constants. Patches 9-10 replace the specific implementation in btrfs and xfs with the common implementation. In this case, the fs specific constants (BTRFS_FT_*) are defined in an include file that is also used by a library. Also, both btrfs and xfs use the 4th bit of the file type on-disk. The constants remained in the include file and a comment was added that constants 0..7 should be kept in sync with the common constants. I posted xfstest generic/396 to sanity test d_type in readdir. Tested these patches on ext2, ext4, xfs. In any case, all the individual fs patches are completely independent of each other and all depend only on patch 1, so each fs maintainer can test and apply the relevant patch to his tree after the common implementation is merged. Obviously, I am looking for feedback from fs maintainers if the approach chosen for their fs is the right approach and feedback on the cleanup in general. Amir Goldstein (11): fs: common implementation of file type conversions ufs: use fs_umode_to_dtype() helper hfsplus: use fs_umode_to_dtype() helper ext2: use common file type conversion exofs: use common file type conversion ext4: use common file type conversion ocfs2: use common file type conversion f2fs: use common file type conversion nilfs2: use common file type conversion btrfs: use common file type conversion xfs: use common file type conversion fs/btrfs/btrfs_inode.h | 2 - fs/btrfs/delayed-inode.c | 2 +- fs/btrfs/inode.c | 19 +------ fs/exofs/common.h | 12 ----- fs/exofs/dir.c | 34 +----------- fs/ext2/dir.c | 35 +++--------- fs/ext2/ext2.h | 16 ------ fs/ext4/ext4.h | 38 +++++-------- fs/f2fs/dir.c | 27 +--------- fs/f2fs/inline.c | 2 +- fs/hfsplus/dir.c | 16 +----- fs/nilfs2/dir.c | 38 ++----------- fs/ocfs2/dir.c | 18 ++----- fs/ocfs2/ocfs2_fs.h | 32 ++++------- fs/ufs/util.h | 29 +--------- fs/xfs/libxfs/xfs_da_format.h | 5 +- fs/xfs/libxfs/xfs_dir2.c | 17 ------ fs/xfs/libxfs/xfs_dir2.h | 6 --- fs/xfs/xfs_iops.c | 2 +- include/linux/f2fs_fs.h | 8 +-- include/linux/file_type.h | 107 +++++++++++++++++++++++++++++++++++++ include/linux/fs.h | 17 +----- include/uapi/linux/btrfs_tree.h | 2 + include/uapi/linux/nilfs2_ondisk.h | 1 + 24 files changed, 167 insertions(+), 318 deletions(-) create mode 100644 include/linux/file_type.h -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-25 21:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-23 20:19 [RFC][PATCH 00/11] common implementation of dirent file types Phillip Potter 2018-10-23 21:17 ` [RFC][PATCH v2 10/10] btrfs: use common file type conversion Phillip Potter 2018-10-24 10:11 ` David Sterba 2018-10-24 13:28 ` Phillip Potter 2018-10-24 6:43 ` [RFC][PATCH 00/11] common implementation of dirent file types Amir Goldstein 2018-10-25 11:08 ` Jan Kara 2018-10-25 12:48 ` Phillip Potter -- strict thread matches above, loose matches on Subject: below -- 2016-12-19 20:10 Amir Goldstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).