[PATCH 0/2] Fixes for the NFS automounter

[PATCH 0/2] Fixes for the NFS automounter

[Trond Myklebust]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

This series addresses 2 sets of issues with the NFS automount
functionality:

1. Capabilities are being inherited from the parent directory instead
   of assuming that the subdirectory may have different properties than
   the parent. This is particularly problematic when the parent is a
   referral.
2. We appear to be repeating some operations unnecessarily when
   traversing a submount point.

Trond Myklebust (2):
  NFS: Fix the setting of capabilities when automounting a new
    filesystem
  NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

 fs/nfs/client.c      | 44 +++++++++++++++++++++-
 fs/nfs/internal.h    |  2 +-
 fs/nfs/nfs4_fs.h     |  5 ++-
 fs/nfs/nfs4client.c  | 20 +---------
 fs/nfs/nfs4getroot.c | 14 +++----
 fs/nfs/nfs4proc.c    | 89 ++++++++++++++++++++------------------------
 6 files changed, 93 insertions(+), 81 deletions(-)

-- 
2.50.1

[PATCH 1/2] NFS: Fix the setting of capabilities when automounting a new filesystem

[Trond Myklebust]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

Capabilities cannot be inherited when we cross into a new filesystem.
They need to be reset to the minimal defaults, and then probed for
again.

Fixes: 54ceac451598 ("NFS: Share NFS superblocks per-protocol per-server per-FSID")
Cc: stable@vger.kernel.org
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/client.c     | 44 ++++++++++++++++++++++++++++++++++++++++++--
 fs/nfs/internal.h   |  2 +-
 fs/nfs/nfs4client.c | 20 +-------------------
 fs/nfs/nfs4proc.c   |  2 +-
 4 files changed, 45 insertions(+), 23 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index e13eb429b8b5..8fb4a950dd55 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -682,6 +682,44 @@ struct nfs_client *nfs_init_client(struct nfs_client *clp,
 }
 EXPORT_SYMBOL_GPL(nfs_init_client);
 
+static void nfs4_server_set_init_caps(struct nfs_server *server)
+{
+#if IS_ENABLED(CONFIG_NFS_V4)
+	/* Set the basic capabilities */
+	server->caps = server->nfs_client->cl_mvops->init_caps;
+	if (server->flags & NFS_MOUNT_NORDIRPLUS)
+		server->caps &= ~NFS_CAP_READDIRPLUS;
+	if (server->nfs_client->cl_proto == XPRT_TRANSPORT_RDMA)
+		server->caps &= ~NFS_CAP_READ_PLUS;
+
+	/*
+	 * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower
+	 * authentication.
+	 */
+	if (nfs4_disable_idmapping &&
+	    server->client->cl_auth->au_flavor == RPC_AUTH_UNIX)
+		server->caps |= NFS_CAP_UIDGID_NOMAP;
+#endif
+}
+
+void nfs_server_set_init_caps(struct nfs_server *server)
+{
+	switch (server->nfs_client->rpc_ops->version) {
+	case 2:
+		server->caps = NFS_CAP_HARDLINKS | NFS_CAP_SYMLINKS;
+		break;
+	case 3:
+		server->caps = NFS_CAP_HARDLINKS | NFS_CAP_SYMLINKS;
+		if (!(server->flags & NFS_MOUNT_NORDIRPLUS))
+			server->caps |= NFS_CAP_READDIRPLUS;
+		break;
+	default:
+		nfs4_server_set_init_caps(server);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(nfs_server_set_init_caps);
+
 /*
  * Create a version 2 or 3 client
  */
@@ -726,7 +764,6 @@ static int nfs_init_server(struct nfs_server *server,
 	/* Initialise the client representation from the mount data */
 	server->flags = ctx->flags;
 	server->options = ctx->options;
-	server->caps |= NFS_CAP_HARDLINKS | NFS_CAP_SYMLINKS;
 
 	switch (clp->rpc_ops->version) {
 	case 2:
@@ -762,6 +799,8 @@ static int nfs_init_server(struct nfs_server *server,
 	if (error < 0)
 		goto error;
 
+	nfs_server_set_init_caps(server);
+
 	/* Preserve the values of mount_server-related mount options */
 	if (ctx->mount_server.addrlen) {
 		memcpy(&server->mountd_address, &ctx->mount_server.address,
@@ -934,7 +973,6 @@ void nfs_server_copy_userdata(struct nfs_server *target, struct nfs_server *sour
 	target->acregmax = source->acregmax;
 	target->acdirmin = source->acdirmin;
 	target->acdirmax = source->acdirmax;
-	target->caps = source->caps;
 	target->options = source->options;
 	target->auth_info = source->auth_info;
 	target->port = source->port;
@@ -1169,6 +1207,8 @@ struct nfs_server *nfs_clone_server(struct nfs_server *source,
 	if (error < 0)
 		goto out_free_server;
 
+	nfs_server_set_init_caps(server);
+
 	/* probe the filesystem info for this server filesystem */
 	error = nfs_probe_server(server, fh);
 	if (error < 0)
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 0143e0794d32..1a18d8d9be25 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -231,7 +231,7 @@ extern struct nfs_client *
 nfs4_find_client_sessionid(struct net *, const struct sockaddr *,
 				struct nfs4_sessionid *, u32);
 extern struct nfs_server *nfs_create_server(struct fs_context *);
-extern void nfs4_server_set_init_caps(struct nfs_server *);
+extern void nfs_server_set_init_caps(struct nfs_server *);
 extern struct nfs_server *nfs4_create_server(struct fs_context *);
 extern struct nfs_server *nfs4_create_referral_server(struct fs_context *);
 extern int nfs4_update_server(struct nfs_server *server, const char *hostname,
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 2ea98f1f116f..6fddf43d729c 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1074,24 +1074,6 @@ static void nfs4_session_limit_xasize(struct nfs_server *server)
 #endif
 }
 
-void nfs4_server_set_init_caps(struct nfs_server *server)
-{
-	/* Set the basic capabilities */
-	server->caps |= server->nfs_client->cl_mvops->init_caps;
-	if (server->flags & NFS_MOUNT_NORDIRPLUS)
-			server->caps &= ~NFS_CAP_READDIRPLUS;
-	if (server->nfs_client->cl_proto == XPRT_TRANSPORT_RDMA)
-		server->caps &= ~NFS_CAP_READ_PLUS;
-
-	/*
-	 * Don't use NFS uid/gid mapping if we're using AUTH_SYS or lower
-	 * authentication.
-	 */
-	if (nfs4_disable_idmapping &&
-			server->client->cl_auth->au_flavor == RPC_AUTH_UNIX)
-		server->caps |= NFS_CAP_UIDGID_NOMAP;
-}
-
 static int nfs4_server_common_setup(struct nfs_server *server,
 		struct nfs_fh *mntfh, bool auth_probe)
 {
@@ -1110,7 +1092,7 @@ static int nfs4_server_common_setup(struct nfs_server *server,
 	if (error < 0)
 		return error;
 
-	nfs4_server_set_init_caps(server);
+	nfs_server_set_init_caps(server);
 
 	/* Probe the root fh to retrieve its FSID and filehandle */
 	error = nfs4_get_rootfh(server, mntfh, auth_probe);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d7dc669d84c5..c7c7ec22f21d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4092,7 +4092,7 @@ int nfs4_server_capabilities(struct nfs_server *server, struct nfs_fh *fhandle)
 	};
 	int err;
 
-	nfs4_server_set_init_caps(server);
+	nfs_server_set_init_caps(server);
 	do {
 		err = nfs4_handle_exception(server,
 				_nfs4_server_capabilities(server, fhandle),
-- 
2.50.1

Re: [PATCH 1/2] NFS: Fix the setting of capabilities when automounting a new filesystem

[Benjamin Coddington]

On 3 Aug 2025, at 21:29, Trond Myklebust wrote:

> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> Capabilities cannot be inherited when we cross into a new filesystem.
> They need to be reset to the minimal defaults, and then probed for
> again.
>
> Fixes: 54ceac451598 ("NFS: Share NFS superblocks per-protocol per-server per-FSID")
> Cc: stable@vger.kernel.org
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

[PATCH 2/2] NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

[Trond Myklebust]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

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@hammerspace.com>
---
 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,
+	};
 	struct nfs4_lookup_root_arg args = {
 		.bitmask = bitmask,
 	};
 	struct nfs4_lookup_res res = {
 		.server = server,
-		.fattr = info->fattr,
+		.fattr = fattr,
 		.fh = fhandle,
 	};
 	struct rpc_message msg = {
@@ -4257,27 +4260,20 @@ static int _nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
 		.rpc_resp = &res,
 	};
 
-	bitmask[0] = nfs4_fattr_bitmap[0];
-	bitmask[1] = nfs4_fattr_bitmap[1];
-	/*
-	 * Process the label in the upcoming getfattr
-	 */
-	bitmask[2] = nfs4_fattr_bitmap[2] & ~FATTR4_WORD2_SECURITY_LABEL;
-
-	nfs_fattr_init(info->fattr);
+	nfs_fattr_init(fattr);
 	return nfs4_call_sync(server->client, server, &msg, &args.seq_args, &res.seq_res, 0);
 }
 
 static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
-		struct nfs_fsinfo *info)
+			    struct nfs_fattr *fattr)
 {
 	struct nfs4_exception exception = {
 		.interruptible = true,
 	};
 	int err;
 	do {
-		err = _nfs4_lookup_root(server, fhandle, info);
-		trace_nfs4_lookup_root(server, fhandle, info->fattr, err);
+		err = _nfs4_lookup_root(server, fhandle, fattr);
+		trace_nfs4_lookup_root(server, fhandle, fattr, err);
 		switch (err) {
 		case 0:
 		case -NFS4ERR_WRONGSEC:
@@ -4290,8 +4286,9 @@ static int nfs4_lookup_root(struct nfs_server *server, struct nfs_fh *fhandle,
 	return err;
 }
 
-static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
-				struct nfs_fsinfo *info, rpc_authflavor_t flavor)
+static int nfs4_lookup_root_sec(struct nfs_server *server,
+				struct nfs_fh *fhandle, struct nfs_fattr *fattr,
+				rpc_authflavor_t flavor)
 {
 	struct rpc_auth_create_args auth_args = {
 		.pseudoflavor = flavor,
@@ -4301,7 +4298,7 @@ static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandl
 	auth = rpcauth_create(&auth_args, server->client);
 	if (IS_ERR(auth))
 		return -EACCES;
-	return nfs4_lookup_root(server, fhandle, info);
+	return nfs4_lookup_root(server, fhandle, fattr);
 }
 
 /*
@@ -4314,7 +4311,7 @@ static int nfs4_lookup_root_sec(struct nfs_server *server, struct nfs_fh *fhandl
  * negative errno value.
  */
 static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
-			      struct nfs_fsinfo *info)
+			      struct nfs_fattr *fattr)
 {
 	/* Per 3530bis 15.33.5 */
 	static const rpc_authflavor_t flav_array[] = {
@@ -4330,8 +4327,9 @@ static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
 	if (server->auth_info.flavor_len > 0) {
 		/* try each flavor specified by user */
 		for (i = 0; i < server->auth_info.flavor_len; i++) {
-			status = nfs4_lookup_root_sec(server, fhandle, info,
-						server->auth_info.flavors[i]);
+			status = nfs4_lookup_root_sec(
+				server, fhandle, fattr,
+				server->auth_info.flavors[i]);
 			if (status == -NFS4ERR_WRONGSEC || status == -EACCES)
 				continue;
 			break;
@@ -4339,7 +4337,7 @@ static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
 	} else {
 		/* no flavors specified by user, try default list */
 		for (i = 0; i < ARRAY_SIZE(flav_array); i++) {
-			status = nfs4_lookup_root_sec(server, fhandle, info,
+			status = nfs4_lookup_root_sec(server, fhandle, fattr,
 						      flav_array[i]);
 			if (status == -NFS4ERR_WRONGSEC || status == -EACCES)
 				continue;
@@ -4363,28 +4361,22 @@ static int nfs4_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
  * nfs4_proc_get_rootfh - get file handle for server's pseudoroot
  * @server: initialized nfs_server handle
  * @fhandle: we fill in the pseudo-fs root file handle
- * @info: we fill in an FSINFO struct
+ * @fattr: we fill in a bare bones struct fattr
  * @auth_probe: probe the auth flavours
  *
  * Returns zero on success, or a negative errno.
  */
 int nfs4_proc_get_rootfh(struct nfs_server *server, struct nfs_fh *fhandle,
-			 struct nfs_fsinfo *info,
-			 bool auth_probe)
+			 struct nfs_fattr *fattr, bool auth_probe)
 {
 	int status = 0;
 
 	if (!auth_probe)
-		status = nfs4_lookup_root(server, fhandle, info);
+		status = nfs4_lookup_root(server, fhandle, fattr);
 
 	if (auth_probe || status == NFS4ERR_WRONGSEC)
-		status = server->nfs_client->cl_mvops->find_root_sec(server,
-				fhandle, info);
-
-	if (status == 0)
-		status = nfs4_server_capabilities(server, fhandle);
-	if (status == 0)
-		status = nfs4_do_fsinfo(server, fhandle, info);
+		status = server->nfs_client->cl_mvops->find_root_sec(
+			server, fhandle, fattr);
 
 	return nfs4_map_errors(status);
 }
@@ -10351,10 +10343,10 @@ nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data, bool sync)
  * Use the state managment nfs_client cl_rpcclient, which uses krb5i (if
  * possible) as per RFC3530bis and RFC5661 Security Considerations sections
  */
-static int
-_nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
-		    struct nfs_fsinfo *info,
-		    struct nfs4_secinfo_flavors *flavors, bool use_integrity)
+static int _nfs41_proc_secinfo_no_name(struct nfs_server *server,
+				       struct nfs_fh *fhandle,
+				       struct nfs4_secinfo_flavors *flavors,
+				       bool use_integrity)
 {
 	struct nfs41_secinfo_no_name_args args = {
 		.style = SECINFO_STYLE_CURRENT_FH,
@@ -10398,9 +10390,9 @@ _nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 	return status;
 }
 
-static int
-nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
-			   struct nfs_fsinfo *info, struct nfs4_secinfo_flavors *flavors)
+static int nfs41_proc_secinfo_no_name(struct nfs_server *server,
+				      struct nfs_fh *fhandle,
+				      struct nfs4_secinfo_flavors *flavors)
 {
 	struct nfs4_exception exception = {
 		.interruptible = true,
@@ -10412,7 +10404,7 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 
 		/* try to use integrity protection with machine cred */
 		if (_nfs4_is_integrity_protected(server->nfs_client))
-			err = _nfs41_proc_secinfo_no_name(server, fhandle, info,
+			err = _nfs41_proc_secinfo_no_name(server, fhandle,
 							  flavors, true);
 
 		/*
@@ -10422,7 +10414,7 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 		 * the current filesystem's rpc_client and the user cred.
 		 */
 		if (err == -NFS4ERR_WRONGSEC)
-			err = _nfs41_proc_secinfo_no_name(server, fhandle, info,
+			err = _nfs41_proc_secinfo_no_name(server, fhandle,
 							  flavors, false);
 
 		switch (err) {
@@ -10438,9 +10430,8 @@ nfs41_proc_secinfo_no_name(struct nfs_server *server, struct nfs_fh *fhandle,
 	return err;
 }
 
-static int
-nfs41_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
-		    struct nfs_fsinfo *info)
+static int nfs41_find_root_sec(struct nfs_server *server,
+			       struct nfs_fh *fhandle, struct nfs_fattr *fattr)
 {
 	int err;
 	struct page *page;
@@ -10456,14 +10447,14 @@ nfs41_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
 	}
 
 	flavors = page_address(page);
-	err = nfs41_proc_secinfo_no_name(server, fhandle, info, flavors);
+	err = nfs41_proc_secinfo_no_name(server, fhandle, flavors);
 
 	/*
 	 * Fall back on "guess and check" method if
 	 * the server doesn't support SECINFO_NO_NAME
 	 */
 	if (err == -NFS4ERR_WRONGSEC || err == -ENOTSUPP) {
-		err = nfs4_find_root_sec(server, fhandle, info);
+		err = nfs4_find_root_sec(server, fhandle, fattr);
 		goto out_freepage;
 	}
 	if (err)
@@ -10488,8 +10479,8 @@ nfs41_find_root_sec(struct nfs_server *server, struct nfs_fh *fhandle,
 			flavor = RPC_AUTH_MAXFLAVOR;
 
 		if (flavor != RPC_AUTH_MAXFLAVOR) {
-			err = nfs4_lookup_root_sec(server, fhandle,
-						   info, flavor);
+			err = nfs4_lookup_root_sec(server, fhandle, fattr,
+						   flavor);
 			if (!err)
 				break;
 		}
-- 
2.50.1

Re: [PATCH 2/2] NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

[Benjamin Coddington]

On 3 Aug 2025, at 21:29, Trond Myklebust wrote:

> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>
> 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@hammerspace.com>
> ---
>  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?

Reviewed-by: Benjamin Coddington <bcodding@redhat.com>

Ben

Re: [PATCH 2/2] NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

[Trond Myklebust]

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@hammerspace.com>
> > 
> > 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@hammerspace.com>
> > ---
> >  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@redhat.com>
> 
> Ben
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trondmy@kernel.org, trond.myklebust@hammerspace.com

Re: [PATCH 2/2] NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

[Benjamin Coddington]

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@hammerspace.com>
>>>
>>> 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@hammerspace.com>
>>> ---
>>>  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

Re: [PATCH 2/2] NFSv4: Remove duplicate lookups, capability probes and fsinfo calls

[Benjamin Coddington]

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@hammerspace.com>
>>>>
>>>> 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@hammerspace.com>
>>>> ---
>>>>  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

Re: [PATCH 0/2] Fixes for the NFS automounter

[Scott Haiden]

Hello,

I think this patch 
(https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=b01f21cacde9f2878492cf318fee61bf4ccad323) 
might have broken the getxattr system call over NFS.

On v6.16.1, I was able to call `getxattr` on files in an NFS 4.2 mount, 
but with this patch, running the same command I get:

     $ cd /path/to/nfs4.2/mount
     $ getfattr -n user.hash.sha512 'S01E01 - Kassa.mkv'
     S01E01 - Kassa.mkv: user.hash.sha512: Operation not supported

Without this patch, it just returns the xattr with no problem.

Am I missing a mount option? Or, is it possible this commit introduced a 
bug?

I ran a git bisect between v6.16.1 and v6.16.2 and it pointed to this 
patch's backport to the stable tree.

Thanks,
--Scott