Re: [PATCH] xfs: use inode_set_cached_link()

["Darrick J. Wong" <djwong@kernel.org>]

On Sat, Nov 23, 2024 at 08:51:05AM +0100, Mateusz Guzik wrote:
> For cases where caching is applicable this dodges inode locking, memory
> allocation and memcpy + strlen.
> 
> Throughput of readlink on Saphire Rappids (ops/s):
> before:	3641273
> after:	4009524 (+10%)
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
> 
> First a minor note that in the stock case strlen is called on the buffer
> and I verified that i_disk_size is the value which is computed.
> 
> The important note is that I'm assuming the pointed to area is stable
> for the duration of the inode's lifetime -- that is if the read off
> symlink is fine *or* it was just created and is eligible caching, it
> wont get invalidated as long as the inode is in memory. If this does not
> hold then this submission is wrong and it would be nice(tm) to remedy
> it.

It is not stable for the lifetime of the inode.  See commit
7b7820b83f2300 ("xfs: don't expose internal symlink metadata buffers to
the vfs").  With parent pointers' ability to expand the symlink xattr
fork area sufficiently to bump the symlink target into a remote block
and online repair's ability to mess with the inode, direct vfs access of
if_data has only become more difficult.

--D

> This depends on stuff which landed in vfs-6.14.misc, but is not in next
> nor fs-next yet.
> 
> For benchmark code see bottom of https://lore.kernel.org/linux-fsdevel/20241120112037.822078-1-mjguzik@gmail.com/
> 
>  fs/xfs/xfs_iops.c    |  1 +
>  fs/xfs/xfs_symlink.c | 24 ++++++++++++++++++++++++
>  fs/xfs/xfs_symlink.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 207e0dadffc3..1d0a3797f876 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1394,6 +1394,7 @@ xfs_setup_iops(
>  		break;
>  	case S_IFLNK:
>  		inode->i_op = &xfs_symlink_inode_operations;
> +		xfs_setup_cached_symlink(ip);
>  		break;
>  	default:
>  		inode->i_op = &xfs_inode_operations;
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index 4252b07cd251..59bf1b9ccb20 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -28,6 +28,30 @@
>  #include "xfs_parent.h"
>  #include "xfs_defer.h"
>  
> +void
> +xfs_setup_cached_symlink(
> +	struct xfs_inode	*ip)
> +{
> +	struct inode		*inode = &ip->i_vnode;
> +	xfs_fsize_t		pathlen;
> +
> +	/*
> +	 * If we have the symlink readily accessible let the VFS know where to
> +	 * find it. This avoids calls to xfs_readlink().
> +	 */
> +	pathlen = ip->i_disk_size;
> +	if (pathlen <= 0 || pathlen > XFS_SYMLINK_MAXLEN)
> +		return;
> +
> +	if (ip->i_df.if_format != XFS_DINODE_FMT_LOCAL)
> +		return;
> +
> +	if (XFS_IS_CORRUPT(ip->i_mount, !ip->i_df.if_data))
> +		return;
> +
> +	inode_set_cached_link(inode, ip->i_df.if_data, pathlen);
> +}
> +
>  int
>  xfs_readlink(
>  	struct xfs_inode	*ip,
> diff --git a/fs/xfs/xfs_symlink.h b/fs/xfs/xfs_symlink.h
> index 0d29a50e66fd..0e45a8a33829 100644
> --- a/fs/xfs/xfs_symlink.h
> +++ b/fs/xfs/xfs_symlink.h
> @@ -12,5 +12,6 @@ int xfs_symlink(struct mnt_idmap *idmap, struct xfs_inode *dp,
>  		umode_t mode, struct xfs_inode **ipp);
>  int xfs_readlink(struct xfs_inode *ip, char *link);
>  int xfs_inactive_symlink(struct xfs_inode *ip);
> +void xfs_setup_cached_symlink(struct xfs_inode *ip);
>  
>  #endif /* __XFS_SYMLINK_H */
> -- 
> 2.43.0
>