On Mon, 2025-08-04 at 12:43 -0400, Benjamin Coddington wrote:
> On 3 Aug 2025, at 21:29, Trond Myklebust wrote:
>
> > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> >
> > When crossing into a new filesystem, the NFSv4 client will look up
> > the
> > new directory, and then call nfs4_server_capabilities() as well as
> > nfs4_do_fsinfo() at least twice.
> >
> > This patch removes the duplicate calls, and reduces the initial
> > lookup
> > to retrieve just a minimal set of attributes.
> >
> > Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> > ---
> > fs/nfs/nfs4_fs.h | 5 ++-
> > fs/nfs/nfs4getroot.c | 14 +++----
> > fs/nfs/nfs4proc.c | 87 ++++++++++++++++++++--------------------
> > ----
> > 3 files changed, 48 insertions(+), 58 deletions(-)
> >
> > diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> > index d3ca91f60fc1..c34c89af9c7d 100644
> > --- a/fs/nfs/nfs4_fs.h
> > +++ b/fs/nfs/nfs4_fs.h
> > @@ -63,7 +63,7 @@ struct nfs4_minor_version_ops {
> > bool (*match_stateid)(const nfs4_stateid *,
> > const nfs4_stateid *);
> > int (*find_root_sec)(struct nfs_server *, struct
> > nfs_fh *,
> > - struct nfs_fsinfo *);
> > + struct nfs_fattr *);
> > void (*free_lock_state)(struct nfs_server *,
> > struct nfs4_lock_state *);
> > int (*test_and_free_expired)(struct nfs_server *,
> > @@ -296,7 +296,8 @@ extern int nfs4_call_sync(struct rpc_clnt *,
> > struct nfs_server *,
> > extern void nfs4_init_sequence(struct nfs4_sequence_args *, struct
> > nfs4_sequence_res *, int, int);
> > extern int nfs4_proc_setclientid(struct nfs_client *, u32,
> > unsigned short, const struct cred *, struct nfs4_setclientid_res
> > *);
> > extern int nfs4_proc_setclientid_confirm(struct nfs_client *,
> > struct nfs4_setclientid_res *arg, const struct cred *);
> > -extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh
> > *, struct nfs_fsinfo *, bool);
> > +extern int nfs4_proc_get_rootfh(struct nfs_server *, struct nfs_fh
> > *,
> > + struct nfs_fattr *, bool);
> > extern int nfs4_proc_bind_conn_to_session(struct nfs_client *,
> > const struct cred *cred);
> > extern int nfs4_proc_exchange_id(struct nfs_client *clp, const
> > struct cred *cred);
> > extern int nfs4_destroy_clientid(struct nfs_client *clp);
> > diff --git a/fs/nfs/nfs4getroot.c b/fs/nfs/nfs4getroot.c
> > index 1a69479a3a59..e67ea345de69 100644
> > --- a/fs/nfs/nfs4getroot.c
> > +++ b/fs/nfs/nfs4getroot.c
> > @@ -12,30 +12,28 @@
> >
> > int nfs4_get_rootfh(struct nfs_server *server, struct nfs_fh
> > *mntfh, bool auth_probe)
> > {
> > - struct nfs_fsinfo fsinfo;
> > + struct nfs_fattr *fattr = nfs_alloc_fattr();
> > int ret = -ENOMEM;
> >
> > - fsinfo.fattr = nfs_alloc_fattr();
> > - if (fsinfo.fattr == NULL)
> > + if (fattr == NULL)
> > goto out;
> >
> > /* Start by getting the root filehandle from the server */
> > - ret = nfs4_proc_get_rootfh(server, mntfh, &fsinfo,
> > auth_probe);
> > + ret = nfs4_proc_get_rootfh(server, mntfh, fattr,
> > auth_probe);
> > if (ret < 0) {
> > dprintk("nfs4_get_rootfh: getroot error = %d\n", -
> > ret);
> > goto out;
> > }
> >
> > - if (!(fsinfo.fattr->valid & NFS_ATTR_FATTR_TYPE)
> > - || !S_ISDIR(fsinfo.fattr->mode)) {
> > + if (!(fattr->valid & NFS_ATTR_FATTR_TYPE) ||
> > !S_ISDIR(fattr->mode)) {
> > printk(KERN_ERR "nfs4_get_rootfh:"
> > " getroot encountered non-directory\n");
> > ret = -ENOTDIR;
> > goto out;
> > }
> >
> > - memcpy(&server->fsid, &fsinfo.fattr->fsid, sizeof(server-
> > >fsid));
> > + memcpy(&server->fsid, &fattr->fsid, sizeof(server->fsid));
> > out:
> > - nfs_free_fattr(fsinfo.fattr);
> > + nfs_free_fattr(fattr);
> > return ret;
> > }
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index c7c7ec22f21d..7d2b67e06cc3 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -4240,15 +4240,18 @@ static int nfs4_discover_trunking(struct
> > nfs_server *server,
> > }
> >
> > static int _nfs4_lookup_root(struct nfs_server *server, struct
> > nfs_fh *fhandle,
> > - struct nfs_fsinfo *info)
> > + struct nfs_fattr *fattr)
> > {
> > - u32 bitmask[3];
> > + u32 bitmask[3] = {
> > + [0] = FATTR4_WORD0_TYPE | FATTR4_WORD0_CHANGE |
> > + FATTR4_WORD0_SIZE | FATTR4_WORD0_FSID,
>
> Just a thought when noticing this change --
>
> Don't we want at least FATTR4_WORD0_FILEID and
> FATTR4_WORD1_MOUNTED_ON_FILEID as well here?
Only FATTR4_WORD0_TYPE and FATTR4_WORD0_FSID are used by the caller, so
we don't actually need FATTR4_WORD0_CHANGE or FATTR4_WORD0_SIZE either.
I put them in so that the test for the auth flavour would have more
chances of catching a problem.
Note that neither FATTR4_WORD0_FILEID or FATTR4_WORD1_MOUNTED_ON_FILEID
are mandatory attributes, so I also chose to avoid them + all the
timestamps for that reason.
>
> Reviewed-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
>
> Ben
>
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@xxxxxxxxxx, trond.myklebust@xxxxxxxxxxxxxxx
