| Message ID | 56697064041956d0bc084593a287136b799b97b7.1525873677.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9.05.2018 16:54, David Sterba wrote: > The btrfs inode flag flavour is now simply called 'inode flags' and the > vfs inode are i_flags. Also rename the internal variable to something > less confusing than 'ip'. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/ctree.h | 2 +- > fs/btrfs/inode.c | 4 ++-- > fs/btrfs/ioctl.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 2771cc56a622..d738eaa1d6e1 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -3260,7 +3260,7 @@ void btrfs_test_inode_set_ops(struct inode *inode); > long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg); > long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg); > int btrfs_ioctl_get_supported_features(void __user *arg); > -void btrfs_update_iflags(struct inode *inode); > +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); > int btrfs_is_empty_uuid(u8 *uuid); > int btrfs_defrag_file(struct inode *inode, struct file *file, > struct btrfs_ioctl_defrag_range_args *range, > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index d241285a0d2a..c30afabcb6b0 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3924,7 +3924,7 @@ static int btrfs_read_locked_inode(struct inode *inode) > break; > } > > - btrfs_update_iflags(inode); > + btrfs_sync_inode_flags_to_i_flags(inode); > return 0; > > make_bad: > @@ -6263,7 +6263,7 @@ static void btrfs_inherit_iflags(struct inode *inode, struct inode *dir) > BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM; > } > > - btrfs_update_iflags(inode); > + btrfs_sync_inode_flags_to_i_flags(inode); > } > > static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans, > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 632e26d6f7ce..11edbdf3df7d 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags) > /* > * Update inode->i_flags based on the btrfs internal flags. > */ > -void btrfs_update_iflags(struct inode *inode) > +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode) > { nit: imho that name is too verbose, I think btrfs_sync_inode_flags should suffice alongside the one line comment at the top of the function. As a matter of fact what necessitates duplicating those flags in both the btrfs-specific data structure and the generic vfs inode? So looking at how btrfs_update_iflags is used, we have: btrfs_ioctl_setflags - we gain nothing by having those flags duplicated since in case of failure the revert procedure is the same - we have to set i_oldflags to both btrfs_inode and vfs_inode btrfs_read_locked_inode - so here we read the flags from the on-disk inode and then use btrfs_update_iflags to set the generic vfs flags to the vfs_inode btrfs_inherit_iflags - the call to btrfs_update_iflags is not even necessary since none of the flags which is being inherited: BTRFS_INODE_NOCOMPRESS/BTRFS_INODE_COMPRESS/BTRFS_INODE_NODATACOW is supported by btrfs_update_iflags. Given this I'd be more inclined to rename the function to something like: btrfs_init_i_flags/btrfs_set_iflags or some such. And to go a bit further imho those flags which are generic should always be set in the vfs_inode and when we are about to persist an inode item on-disk then we should get the necessary flags from the vfs_inode and put in the inode item. > struct btrfs_inode *ip = BTRFS_I(inode); > unsigned int new_fl = 0; > @@ -317,7 +317,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) > goto out_drop; > } > > - btrfs_update_iflags(inode); > + btrfs_sync_inode_flags_to_i_flags(inode); > inode_inc_iversion(inode); > inode->i_ctime = current_time(inode); > ret = btrfs_update_inode(trans, root, inode); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 10, 2018 at 09:34:21AM +0300, Nikolay Borisov wrote: > > @@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags) > > /* > > * Update inode->i_flags based on the btrfs internal flags. > > */ > > -void btrfs_update_iflags(struct inode *inode) > > +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode) > > { > > nit: imho that name is too verbose, That's intentional to be descriptive enough so reading the comment at the functions is not necessary. > I think btrfs_sync_inode_flags > should suffice alongside the one line comment at the top of the > function. There are too many flavors of inodes, which direction would 'btrfs_sync_inode_flags' sync? > As a matter of fact what necessitates duplicating those flags > in both the btrfs-specific data structure and the generic vfs inode? Because btrfs inode flags are part of the on-disk format while the VFS inode flags are in-memory and can change any time. > So looking at how btrfs_update_iflags is used, we have: > > btrfs_ioctl_setflags - we gain nothing by having those flags duplicated > since in case of failure the revert procedure is the same - we have to > set i_oldflags to both btrfs_inode and vfs_inode > > btrfs_read_locked_inode - so here we read the flags from the on-disk > inode and then use btrfs_update_iflags to set the generic vfs flags to > the vfs_inode > > btrfs_inherit_iflags - the call to btrfs_update_iflags is not even > necessary since none of the flags which is being inherited: > BTRFS_INODE_NOCOMPRESS/BTRFS_INODE_COMPRESS/BTRFS_INODE_NODATACOW > is supported by btrfs_update_iflags. > > Given this I'd be more inclined to rename the function to something > like: btrfs_init_i_flags/btrfs_set_iflags or some such. > > And to go a bit further imho those flags which are generic should always > be set in the vfs_inode and when we are about to persist an inode item > on-disk then we should get the necessary flags from the vfs_inode and > put in the inode item. Not all btrfs flags have the counterpart in vfs inode flags and vice versa, they're different namespace and for that we need the conversion helpers. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 2771cc56a622..d738eaa1d6e1 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3260,7 +3260,7 @@ void btrfs_test_inode_set_ops(struct inode *inode); long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg); long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg); int btrfs_ioctl_get_supported_features(void __user *arg); -void btrfs_update_iflags(struct inode *inode); +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode); int btrfs_is_empty_uuid(u8 *uuid); int btrfs_defrag_file(struct inode *inode, struct file *file, struct btrfs_ioctl_defrag_range_args *range, diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index d241285a0d2a..c30afabcb6b0 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3924,7 +3924,7 @@ static int btrfs_read_locked_inode(struct inode *inode) break; } - btrfs_update_iflags(inode); + btrfs_sync_inode_flags_to_i_flags(inode); return 0; make_bad: @@ -6263,7 +6263,7 @@ static void btrfs_inherit_iflags(struct inode *inode, struct inode *dir) BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM; } - btrfs_update_iflags(inode); + btrfs_sync_inode_flags_to_i_flags(inode); } static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans, diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 632e26d6f7ce..11edbdf3df7d 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags) /* * Update inode->i_flags based on the btrfs internal flags. */ -void btrfs_update_iflags(struct inode *inode) +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode) { struct btrfs_inode *ip = BTRFS_I(inode); unsigned int new_fl = 0; @@ -317,7 +317,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg) goto out_drop; } - btrfs_update_iflags(inode); + btrfs_sync_inode_flags_to_i_flags(inode); inode_inc_iversion(inode); inode->i_ctime = current_time(inode); ret = btrfs_update_inode(trans, root, inode);
The btrfs inode flag flavour is now simply called 'inode flags' and the vfs inode are i_flags. Also rename the internal variable to something less confusing than 'ip'. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/ctree.h | 2 +- fs/btrfs/inode.c | 4 ++-- fs/btrfs/ioctl.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)