linux-kernel - Re: [PATCH RFC] fs: reduce pointer chasing in is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241113143645.45652s6afeg3kdmt@quack3>
Date: Wed, 13 Nov 2024 15:36:45 +0100
From: Jan Kara <jack@...e.cz>
To: Jeff Layton <jlayton@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Josef Bacik <josef@...icpanda.com>,
	"Darrick J. Wong" <djwong@...nel.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] fs: reduce pointer chasing in is_mgtime() test

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@...nel.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@...e.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@...nel.org>
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR