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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxx/
>
> 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
>
