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

[Christian Schoenebeck]

On Sunday, 15 February 2026 13:36:02 CET Dominique Martinet wrote:
> Still haven't taken time to review, just replying since I'm looking at
> 9p mails tonight... (The IO accounting part was sent to Linus earlier,
> still intending to review and test soonish but feel free to send a v3
> with Christian comments for now)
> 
> Remi Pommarel wrote on Thu, Feb 12, 2026 at 10:42:50PM +0100:
> > > > +	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
> > 
> > I'm having trouble seeing how v9fs_issue_read() could be called for
> > S_ISLNK inodes under 9p2000 or 9p2000.u.
> > 
> > As I understand it, v9fs_issue_read() is only invoked through the page
> > cache operations via the inode’s a_ops. This seems to me that only
> > regular files and (now that I added a page_get_link() in
> > v9fs_symlink_inode_operations_dotl) symlinks using 9p2000.L can call
> > v9fs_issue_read(). But not symlinks using 9p2000 or 9p2000.u as I
> > haven't modified v9fs_symlink_inode_operations. But I may have missed
> > something here ?
> 
> I think that's correct, but since it's not obvious perhaps a comment
> just above the p9_client_readlink() call might be useful?

And maybe adding a BUG_ON(!p9_is_proto_dotl(client)) in p9_client_readlink()
[net/9p/client.c].

> Also nitpick: please add brackets to the else as well because the if
> part had some (checkpatch doesn't complain either way, but I don't like
>   if (foo) {
>     ...
>   } else
>     bar
> formatting)
> 
> > > > 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?
> > 
> > According to filesystems/porting.rst and commit 21fc61c73c39 ("don't put
> > symlink bodies in pagecache into highmem") all symlinks that need to use
> > page_follow_link_light() (which is now more or less page_get_link())
> > should not add highmem pages in pagecache or deadlocks could happen. The
> > inode_nohighmem() prevents that.
> > 
> > Also from __page_get_link()
> > 
> >   BUG_ON(mapping_gfp_mask(mapping) & __GFP_HIGHMEM);
> > 
> > A BUG_ON() is supposed to punish us if inode_nohighmem() is not used
> > here.
> > 
> > Of course this does not have any effect on 64bits platforms.
> 
> That's how it should be but it marks memory as GFP_USER, I'm curious if
> it really has no other effect... Anyway, from what I've read I think it
> is likely to go away (highmem on 32bit platforms) soon enough, so this
> probably won't matter in the long run.
> (if we really care we could flag this only for S_ISLNK(mode), but I
> don't think it's worth the check)

Good point. Most other fs only call inode_nohighmem() for symlinks. IIUIC it's
not a good idea to do this simply for all inodes, as it may lead to deadlocks
for instance. So I think it is worth to add that check.

/Christian