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

[Dominique Martinet <asmadeus@codewreck.org>]

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?

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)


-- 
Dominique Martinet | Asmadeus