On 4 Aug 2025, at 15:07, Benjamin Coddington wrote:
> 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.
I ran this through some basic testing, and I see now that _TYPE gets into
fattr->mode when we decode, which of course makes sense.
So, FWIW - this makes sense and looks good to me now.
Ben
