On 4 Aug 2025, at 12:54, Trond Myklebust wrote:
> 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;
>>> }
.. now I think we won't have fattr->mode here, more below:
>>>
>>> - 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.
Ah, I see now... we're going to call ->fsinfo() again (as you stated in the
description).
But I don't see how _CHANGE or _SIZE are used.. and as I was hunting around
there's that check in nfs4_get_rootfh() (above) for S_ISDIR(fattr->mode), but this
dropped the fsinfo call out of nfs4_proc_get_rootfh(), so I think
fattr->mode will never be set.
> 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.
Gotcha, making sense. This is tricky for me to review, maybe I should just
test it.. :)
Ben
