Re: [PATCH v2 3/3] 9p: Enable symlink caching in page cache

[Christian Schoenebeck <linux_oss@crudebyte.com>]

On Wednesday, 21 January 2026 20:56:10 CET Remi Pommarel wrote:
> Currently, when cache=loose is enabled, file reads are cached in the
> page cache, but symlink reads are not. This patch allows the results
> of p9_client_readlink() to be stored in the page cache, eliminating
> the need for repeated 9P transactions on subsequent symlink accesses.
> 
> This change improves performance for workloads that involve frequent
> symlink resolution.
> 
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  fs/9p/vfs_addr.c       | 24 ++++++++++++--
>  fs/9p/vfs_inode.c      |  6 ++--
>  fs/9p/vfs_inode_dotl.c | 73 +++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 90 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/9p/vfs_addr.c b/fs/9p/vfs_addr.c
> index 862164181bac..ee672abbb02c 100644
> --- a/fs/9p/vfs_addr.c
> +++ b/fs/9p/vfs_addr.c
> @@ -70,10 +70,19 @@ static void v9fs_issue_read(struct netfs_io_subrequest
> *subreq) {
>  	struct netfs_io_request *rreq = subreq->rreq;
>  	struct p9_fid *fid = rreq->netfs_priv;
> +	char *target;
>  	unsigned long long pos = subreq->start + subreq->transferred;
> -	int total, err;
> -
> -	total = p9_client_read(fid, pos, &subreq->io_iter, &err);
> +	int total, err, len, n;
> +
> +	if (S_ISLNK(rreq->inode->i_mode)) {
> +		err = p9_client_readlink(fid, &target);

Treadlink request requires 9p2000.L. So this would break with legacy protocol
versions 9p2000 and 9p2000.u I guess:

https://wiki.qemu.org/Documentation/9p#9p_Protocol

> +		len = strnlen(target, PAGE_SIZE - 1);

Usually we are bound to PATH_MAX.

Target link path is coming from 9p server, which may run another OS and
therefore target might be longer than PATH_MAX, in which case it would always
yield in -ENAMETOOLONG when client requests that link target from cache.

But OTOH there is no real alternative for storing a link target in cache that
can never be delivered to user. So I guess it is OK as-is.

Simply using PATH_MAX would make it worse, as it would potentially silently
shorten the link from host.

> +		n = copy_to_iter(target, len, &subreq->io_iter);
> +		if (n != len)
> +			err = -EFAULT;
> +		total = i_size_read(rreq->inode);
> +	} else
> +		total = p9_client_read(fid, pos, &subreq->io_iter, &err);
> 
>  	/* if we just extended the file size, any portion not in
>  	 * cache won't be on server and is zeroes */
> @@ -99,6 +108,7 @@ static void v9fs_issue_read(struct netfs_io_subrequest
> *subreq) static int v9fs_init_request(struct netfs_io_request *rreq, struct
> file *file) {
>  	struct p9_fid *fid;
> +	struct dentry *dentry;
>  	bool writing = (rreq->origin == NETFS_READ_FOR_WRITE ||
>  			rreq->origin == NETFS_WRITETHROUGH ||
>  			rreq->origin == NETFS_UNBUFFERED_WRITE ||
> @@ -115,6 +125,14 @@ static int v9fs_init_request(struct netfs_io_request
> *rreq, struct file *file) if (!fid)
>  			goto no_fid;
>  		p9_fid_get(fid);
> +	} else if (S_ISLNK(rreq->inode->i_mode)) {
> +		dentry = d_find_alias(rreq->inode);
> +		if (!dentry)
> +			goto no_fid;
> +		fid = v9fs_fid_lookup(dentry);
> +		dput(dentry);
> +		if (IS_ERR(fid))
> +			goto no_fid;
>  	} else {
>  		fid = v9fs_fid_find_inode(rreq->inode, writing, INVALID_UID, true);
>  		if (!fid)
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index a82a71be309b..e1b762f3e081 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -302,10 +302,12 @@ int v9fs_init_inode(struct v9fs_session_info *v9ses,
>  			goto error;
>  		}
> 
> -		if (v9fs_proto_dotl(v9ses))
> +		if (v9fs_proto_dotl(v9ses)) {
>  			inode->i_op = &v9fs_symlink_inode_operations_dotl;
> -		else
> +			inode_nohighmem(inode);

What is that for?

> +		} else {
>  			inode->i_op = &v9fs_symlink_inode_operations;
> +		}
> 
>  		break;
>  	case S_IFDIR:
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index 6312b3590f74..486b11dbada3 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -686,9 +686,13 @@ v9fs_vfs_symlink_dotl(struct mnt_idmap *idmap, struct
> inode *dir, int err;
>  	kgid_t gid;
>  	const unsigned char *name;
> +	umode_t mode;
> +	struct v9fs_session_info *v9ses;
>  	struct p9_qid qid;
>  	struct p9_fid *dfid;
>  	struct p9_fid *fid = NULL;
> +	struct inode *inode;
> +	struct posix_acl *dacl = NULL, *pacl = NULL;
> 
>  	name = dentry->d_name.name;
>  	p9_debug(P9_DEBUG_VFS, "%lu,%s,%s\n", dir->i_ino, name, symname);
> @@ -702,6 +706,15 @@ v9fs_vfs_symlink_dotl(struct mnt_idmap *idmap, struct
> inode *dir,
> 
>  	gid = v9fs_get_fsgid_for_create(dir);
> 
> +	/* Update mode based on ACL value */
> +	err = v9fs_acl_mode(dir, &mode, &dacl, &pacl);
> +	if (err) {
> +		p9_debug(P9_DEBUG_VFS,
> +			 "Failed to get acl values in symlink %d\n",
> +			 err);
> +		goto error;
> +	}
> +
>  	/* Server doesn't alter fid on TSYMLINK. Hence no need to clone it. */
>  	err = p9_client_symlink(dfid, name, symname, gid, &qid);
> 
> @@ -712,8 +725,30 @@ v9fs_vfs_symlink_dotl(struct mnt_idmap *idmap, struct
> inode *dir,
> 
>  	v9fs_invalidate_inode_attr(dir);
> 
> +	/* instantiate inode and assign the unopened fid to the dentry */
> +	fid = p9_client_walk(dfid, 1, &name, 1);
> +	if (IS_ERR(fid)) {
> +		err = PTR_ERR(fid);
> +		p9_debug(P9_DEBUG_VFS, "p9_client_walk failed %d\n",
> +			 err);
> +		goto error;
> +	}
> +
> +	v9ses = v9fs_inode2v9ses(dir);
> +	inode = v9fs_get_new_inode_from_fid(v9ses, fid, dir->i_sb);
> +	if (IS_ERR(inode)) {
> +		err = PTR_ERR(inode);
> +		p9_debug(P9_DEBUG_VFS, "inode creation failed %d\n",
> +			 err);
> +		goto error;
> +	}
> +	v9fs_set_create_acl(inode, fid, dacl, pacl);
> +	v9fs_fid_add(dentry, &fid);
> +	d_instantiate(dentry, inode);
> +	err = 0;
>  error:
>  	p9_fid_put(fid);
> +	v9fs_put_acl(dacl, pacl);
>  	p9_fid_put(dfid);
>  	return err;
>  }
> @@ -853,24 +888,23 @@ v9fs_vfs_mknod_dotl(struct mnt_idmap *idmap, struct
> inode *dir, }
> 
>  /**
> - * v9fs_vfs_get_link_dotl - follow a symlink path
> + * v9fs_vfs_get_link_nocache_dotl - Resolve a symlink directly.
> + *
> + * To be used when symlink caching is not enabled.
> + *
>   * @dentry: dentry for symlink
>   * @inode: inode for symlink
>   * @done: destructor for return value
>   */
> -
>  static const char *
> -v9fs_vfs_get_link_dotl(struct dentry *dentry,
> -		       struct inode *inode,
> -		       struct delayed_call *done)
> +v9fs_vfs_get_link_nocache_dotl(struct dentry *dentry,
> +			       struct inode *inode,
> +			       struct delayed_call *done)
>  {
>  	struct p9_fid *fid;
>  	char *target;
>  	int retval;
> 
> -	if (!dentry)
> -		return ERR_PTR(-ECHILD);
> -
>  	p9_debug(P9_DEBUG_VFS, "%pd\n", dentry);
> 
>  	fid = v9fs_fid_lookup(dentry);
> @@ -884,6 +918,29 @@ v9fs_vfs_get_link_dotl(struct dentry *dentry,
>  	return target;
>  }
> 
> +/**
> + * v9fs_vfs_get_link_dotl - follow a symlink path
> + * @dentry: dentry for symlink
> + * @inode: inode for symlink
> + * @done: destructor for return value
> + */
> +static const char *
> +v9fs_vfs_get_link_dotl(struct dentry *dentry,
> +		       struct inode *inode,
> +		       struct delayed_call *done)
> +{
> +	struct v9fs_session_info *v9ses;
> +
> +	if (!dentry)
> +		return ERR_PTR(-ECHILD);
> +
> +	v9ses = v9fs_inode2v9ses(inode);
> +	if (v9ses->cache & (CACHE_META|CACHE_LOOSE))
> +		return page_get_link(dentry, inode, done);
> +
> +	return v9fs_vfs_get_link_nocache_dotl(dentry, inode, done);
> +}
> +
>  int v9fs_refresh_inode_dotl(struct p9_fid *fid, struct inode *inode)
>  {
>  	struct p9_stat_dotl *st;