| Message ID | 20241113-mgtime-v1-1-84e256980e11@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fs: reduce pointer chasing in is_mgtime() test | expand |
On Wed 13-11-24 09:17:51, Jeff Layton wrote: > The is_mgtime test checks whether the FS_MGTIME flag is set in the > fstype. To get there from the inode though, we have to dereference 3 > pointers. > > Add a new IOP_MGTIME flag, and have inode_init_always() set that flag > when the fstype flag is set. Then, make is_mgtime test for IOP_MGTIME > instead. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> I guess this makes sense. I'd say inode->i_sb is likely in cache anyway by the time we get to updating inode timestamps but the sb->s_type->fs_flags dereferences are likely cache cold. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > I had always had a little concern around the amount of pointer chasing > in this helper. Given the discussion around Josef's fsnotify patches, I > figured I'd draft up a patch to cut that down. > > Sending this as an RFC since we're getting close to the end of the merge > window and I haven't done any performance testing with this. I think > it's a reasonable thing to consider doing though, given how hot the > write() codepaths can be. > --- > fs/inode.c | 2 ++ > include/linux/fs.h | 3 ++- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 838be0b49a63bd8d5700db0e6103c47e251793c3..70a2f8c717e063752a0b87c6eb27cde7a18d6879 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -243,6 +243,8 @@ int inode_init_always_gfp(struct super_block *sb, struct inode *inode, gfp_t gfp > inode->i_opflags = 0; > if (sb->s_xattr) > inode->i_opflags |= IOP_XATTR; > + if (sb->s_type->fs_flags & FS_MGTIME) > + inode->i_opflags |= IOP_MGTIME; > i_uid_write(inode, 0); > i_gid_write(inode, 0); > atomic_set(&inode->i_writecount, 0); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index aa37083436096df9969d2f63f6ec4d1dc8b260d2..d32c6f6298b17c44ff22d922516028da31cec14d 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -623,6 +623,7 @@ is_uncached_acl(struct posix_acl *acl) > #define IOP_NOFOLLOW 0x0004 > #define IOP_XATTR 0x0008 > #define IOP_DEFAULT_READLINK 0x0010 > +#define IOP_MGTIME 0x0020 > > /* > * Keep mostly read-only and often accessed (especially for > @@ -2581,7 +2582,7 @@ struct file_system_type { > */ > static inline bool is_mgtime(const struct inode *inode) > { > - return inode->i_sb->s_type->fs_flags & FS_MGTIME; > + return inode->i_opflags & IOP_MGTIME; > } > > extern struct dentry *mount_bdev(struct file_system_type *fs_type, > > --- > base-commit: 80ce1b3dc72ceab16a967e2aa222c5cc06ad6042 > change-id: 20241113-mgtime-9aad7b90c64a > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> >
On Wed, 13 Nov 2024 09:17:51 -0500, Jeff Layton wrote: > The is_mgtime test checks whether the FS_MGTIME flag is set in the > fstype. To get there from the inode though, we have to dereference 3 > pointers. > > Add a new IOP_MGTIME flag, and have inode_init_always() set that flag > when the fstype flag is set. Then, make is_mgtime test for IOP_MGTIME > instead. > > [...] Applied to the vfs.mgtime branch of the vfs/vfs.git tree. Patches in the vfs.mgtime branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.mgtime [1/1] fs: reduce pointer chasing in is_mgtime() test https://git.kernel.org/vfs/vfs/c/9fed2c0f2f07
diff --git a/fs/inode.c b/fs/inode.c index 838be0b49a63bd8d5700db0e6103c47e251793c3..70a2f8c717e063752a0b87c6eb27cde7a18d6879 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -243,6 +243,8 @@ int inode_init_always_gfp(struct super_block *sb, struct inode *inode, gfp_t gfp inode->i_opflags = 0; if (sb->s_xattr) inode->i_opflags |= IOP_XATTR; + if (sb->s_type->fs_flags & FS_MGTIME) + inode->i_opflags |= IOP_MGTIME; i_uid_write(inode, 0); i_gid_write(inode, 0); atomic_set(&inode->i_writecount, 0); diff --git a/include/linux/fs.h b/include/linux/fs.h index aa37083436096df9969d2f63f6ec4d1dc8b260d2..d32c6f6298b17c44ff22d922516028da31cec14d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -623,6 +623,7 @@ is_uncached_acl(struct posix_acl *acl) #define IOP_NOFOLLOW 0x0004 #define IOP_XATTR 0x0008 #define IOP_DEFAULT_READLINK 0x0010 +#define IOP_MGTIME 0x0020 /* * Keep mostly read-only and often accessed (especially for @@ -2581,7 +2582,7 @@ struct file_system_type { */ static inline bool is_mgtime(const struct inode *inode) { - return inode->i_sb->s_type->fs_flags & FS_MGTIME; + return inode->i_opflags & IOP_MGTIME; } extern struct dentry *mount_bdev(struct file_system_type *fs_type,
The is_mgtime test checks whether the FS_MGTIME flag is set in the fstype. To get there from the inode though, we have to dereference 3 pointers. Add a new IOP_MGTIME flag, and have inode_init_always() set that flag when the fstype flag is set. Then, make is_mgtime test for IOP_MGTIME instead. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- I had always had a little concern around the amount of pointer chasing in this helper. Given the discussion around Josef's fsnotify patches, I figured I'd draft up a patch to cut that down. Sending this as an RFC since we're getting close to the end of the merge window and I haven't done any performance testing with this. I think it's a reasonable thing to consider doing though, given how hot the write() codepaths can be. --- fs/inode.c | 2 ++ include/linux/fs.h | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) --- base-commit: 80ce1b3dc72ceab16a967e2aa222c5cc06ad6042 change-id: 20241113-mgtime-9aad7b90c64a Best regards,