[PATCH 0/2] cleanups in nfs4reovery.c

[PATCH 0/2] cleanups in nfs4reovery.c

[NeilBrown]

This first of these patchs is part of my work to change how directory
locking is managed.  That will involve moving the lock as close as possible
to the operation being locked, and using some standard interfaces 
which combine the lock and the lookup.  Then changing the mechanics of
taking a lock.

nfsd4_list_rec_dir() currenty locks a direct and performs a lookup
in a different function to where the lock and lookup results are needed,
and does it even when those are not needed at all.  So the first
patch moves the lock and lookup to where it is needed.

The second patch (arguably) improves the calling protocol for
nfs4_client_to_reclaim().  If people don't like this second patch I'm
happy for it to be dropped.  It is the first patch which is particularly
important to me.

Thanks,
NeilBrown


 [PATCH 1/2] nfsd: move name lookup out of nfsd4_list_rec_dir()
 [PATCH 2/2] nfsd: change nfs4_client_to_reclaim() to allocate data

[PATCH 1/2] nfsd: move name lookup out of nfsd4_list_rec_dir()

[NeilBrown]

From: NeilBrown <neil@brown.name>

nfsd4_list_rec_dir() is called with two different callbacks.
One of the callbacks uses vfs_rmdir() to remove the directory.
The other doesn't use the dentry at all, just the name.

As only one callback needs the dentry, this patch moves the lookup into
that callback.  This prepares of changes to how directory operations
are locked.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4recover.c | 50 +++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index e2b9472e5c78..990fe3d958ed 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -237,7 +237,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	nfs4_reset_creds(original_cred);
 }
 
-typedef int (recdir_func)(struct dentry *, struct dentry *, struct nfsd_net *);
+typedef int (recdir_func)(struct dentry *, char *, struct nfsd_net *);
 
 struct name_list {
 	char name[HEXDIR_LEN];
@@ -291,24 +291,14 @@ nfsd4_list_rec_dir(recdir_func *f, struct nfsd_net *nn)
 	}
 
 	status = iterate_dir(nn->rec_file, &ctx.ctx);
-	inode_lock_nested(d_inode(dir), I_MUTEX_PARENT);
 
 	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
-		if (!status) {
-			struct dentry *dentry;
-			dentry = lookup_one(&nop_mnt_idmap,
-					    &QSTR(entry->name), dir);
-			if (IS_ERR(dentry)) {
-				status = PTR_ERR(dentry);
-				break;
-			}
-			status = f(dir, dentry, nn);
-			dput(dentry);
-		}
+		if (!status)
+			status = f(dir, entry->name, nn);
+
 		list_del(&entry->list);
 		kfree(entry);
 	}
-	inode_unlock(d_inode(dir));
 	nfs4_reset_creds(original_cred);
 
 	list_for_each_entry_safe(entry, tmp, &ctx.names, list) {
@@ -406,18 +396,19 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 }
 
 static int
-purge_old(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
+purge_old(struct dentry *parent, char *cname, struct nfsd_net *nn)
 {
 	int status;
+	struct dentry *child;
 	struct xdr_netobj name;
 
-	if (child->d_name.len != HEXDIR_LEN - 1) {
+	if (strlen(cname) != HEXDIR_LEN - 1) {
 		printk("%s: illegal name %pd in recovery directory\n",
 				__func__, child);
 		/* Keep trying; maybe the others are OK: */
 		return 0;
 	}
-	name.data = kmemdup_nul(child->d_name.name, child->d_name.len, GFP_KERNEL);
+	name.data = kstrdup(cname, GFP_KERNEL);
 	if (!name.data) {
 		dprintk("%s: failed to allocate memory for name.data!\n",
 			__func__);
@@ -427,10 +418,17 @@ purge_old(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
 	if (nfs4_has_reclaimed_state(name, nn))
 		goto out_free;
 
-	status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child);
-	if (status)
-		printk("failed to remove client recovery directory %pd\n",
-				child);
+	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
+	child = lookup_one(&nop_mnt_idmap, &QSTR(cname), parent);
+	if (!IS_ERR(child)) {
+		status = vfs_rmdir(&nop_mnt_idmap, d_inode(parent), child);
+		if (status)
+			printk("failed to remove client recovery directory %pd\n",
+			       child);
+		dput(child);
+	}
+	inode_unlock(d_inode(parent));
+
 out_free:
 	kfree(name.data);
 out:
@@ -461,18 +459,18 @@ nfsd4_recdir_purge_old(struct nfsd_net *nn)
 }
 
 static int
-load_recdir(struct dentry *parent, struct dentry *child, struct nfsd_net *nn)
+load_recdir(struct dentry *parent, char *cname, struct nfsd_net *nn)
 {
 	struct xdr_netobj name;
 	struct xdr_netobj princhash = { .len = 0, .data = NULL };
 
-	if (child->d_name.len != HEXDIR_LEN - 1) {
-		printk("%s: illegal name %pd in recovery directory\n",
-				__func__, child);
+	if (strlen(cname) != HEXDIR_LEN - 1) {
+		printk("%s: illegal name %s in recovery directory\n",
+				__func__, cname);
 		/* Keep trying; maybe the others are OK: */
 		return 0;
 	}
-	name.data = kmemdup_nul(child->d_name.name, child->d_name.len, GFP_KERNEL);
+	name.data = kstrdup(cname, GFP_KERNEL);
 	if (!name.data) {
 		dprintk("%s: failed to allocate memory for name.data!\n",
 			__func__);
-- 
2.50.0.107.gf914562f5916.dirty

[PATCH 2/2] nfsd: change nfs4_client_to_reclaim() to allocate data

[NeilBrown]

From: NeilBrown <neil@brown.name>

The calling convention for nfs4_client_to_reclaim() is clumsy in that
the caller need to free memory if the function fails.  It is much
cleaner if the function frees its own memory.

This patch changes nfs4_client_to_reclaim() to re-allocate the .data
fields to be stored in the newly allocated struct nfs4_client_reclaim,
and to free everything on failure.

__cld_pipe_inprogress_downcall() needs to allocate the data anyway to
copy it from user-space, so now that data is allocated twice.  I think
that is a small price to pay for a cleaner interface.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/nfsd/nfs4recover.c | 67 +++++++++++++++----------------------------
 fs/nfsd/nfs4state.c   | 22 ++++++++++++--
 2 files changed, 42 insertions(+), 47 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 990fe3d958ed..1605538ecb80 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -147,24 +147,13 @@ legacy_recdir_name_error(struct nfs4_client *clp, int error)
 
 static void
 __nfsd4_create_reclaim_record_grace(struct nfs4_client *clp,
-		const char *dname, int len, struct nfsd_net *nn)
+				    char *dname, struct nfsd_net *nn)
 {
-	struct xdr_netobj name;
+	struct xdr_netobj name = { .len = strlen(dname), .data = dname };
 	struct xdr_netobj princhash = { .len = 0, .data = NULL };
 	struct nfs4_client_reclaim *crp;
 
-	name.data = kmemdup(dname, len, GFP_KERNEL);
-	if (!name.data) {
-		dprintk("%s: failed to allocate memory for name.data!\n",
-			__func__);
-		return;
-	}
-	name.len = len;
 	crp = nfs4_client_to_reclaim(name, princhash, nn);
-	if (!crp) {
-		kfree(name.data);
-		return;
-	}
 	crp->cr_clp = clp;
 }
 
@@ -223,8 +212,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	inode_unlock(d_inode(dir));
 	if (status == 0) {
 		if (nn->in_grace)
-			__nfsd4_create_reclaim_record_grace(clp, dname,
-					HEXDIR_LEN, nn);
+			__nfsd4_create_reclaim_record_grace(clp, dname, nn);
 		vfs_fsync(nn->rec_file, 0);
 	} else {
 		printk(KERN_ERR "NFSD: failed to write recovery record"
@@ -461,7 +449,7 @@ nfsd4_recdir_purge_old(struct nfsd_net *nn)
 static int
 load_recdir(struct dentry *parent, char *cname, struct nfsd_net *nn)
 {
-	struct xdr_netobj name;
+	struct xdr_netobj name = { .len = HEXDIR_LEN, .data = cname };
 	struct xdr_netobj princhash = { .len = 0, .data = NULL };
 
 	if (strlen(cname) != HEXDIR_LEN - 1) {
@@ -470,16 +458,7 @@ load_recdir(struct dentry *parent, char *cname, struct nfsd_net *nn)
 		/* Keep trying; maybe the others are OK: */
 		return 0;
 	}
-	name.data = kstrdup(cname, GFP_KERNEL);
-	if (!name.data) {
-		dprintk("%s: failed to allocate memory for name.data!\n",
-			__func__);
-		goto out;
-	}
-	name.len = HEXDIR_LEN;
-	if (!nfs4_client_to_reclaim(name, princhash, nn))
-		kfree(name.data);
-out:
+	nfs4_client_to_reclaim(name, princhash, nn);
 	return 0;
 }
 
@@ -777,6 +756,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
 {
 	uint8_t cmd, princhashlen;
 	struct xdr_netobj name, princhash = { .len = 0, .data = NULL };
+	char *namecopy __free(kfree) = NULL;
+	char *princhashcopy __free(kfree) = NULL;
 	uint16_t namelen;
 
 	if (get_user(cmd, &cmsg->cm_cmd)) {
@@ -794,19 +775,19 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
 				dprintk("%s: invalid namelen (%u)", __func__, namelen);
 				return -EINVAL;
 			}
-			name.data = memdup_user(&ci->cc_name.cn_id, namelen);
-			if (IS_ERR(name.data))
-				return PTR_ERR(name.data);
+			namecopy = memdup_user(&ci->cc_name.cn_id, namelen);
+			if (IS_ERR(namecopy))
+				return PTR_ERR(namecopy);
+			name.data = namecopy;
 			name.len = namelen;
 			get_user(princhashlen, &ci->cc_princhash.cp_len);
 			if (princhashlen > 0) {
-				princhash.data = memdup_user(
-						&ci->cc_princhash.cp_data,
-						princhashlen);
-				if (IS_ERR(princhash.data)) {
-					kfree(name.data);
-					return PTR_ERR(princhash.data);
-				}
+				princhashcopy = memdup_user(
+					&ci->cc_princhash.cp_data,
+					princhashlen);
+				if (IS_ERR(princhashcopy))
+					return PTR_ERR(princhashcopy);
+				princhash.data = princhashcopy;
 				princhash.len = princhashlen;
 			} else
 				princhash.len = 0;
@@ -820,9 +801,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
 				dprintk("%s: invalid namelen (%u)", __func__, namelen);
 				return -EINVAL;
 			}
-			name.data = memdup_user(&cnm->cn_id, namelen);
-			if (IS_ERR(name.data))
-				return PTR_ERR(name.data);
+			namecopy = memdup_user(&cnm->cn_id, namelen);
+			if (IS_ERR(namecopy))
+				return PTR_ERR(namecopy);
+			name.data = namecopy;
 			name.len = namelen;
 		}
 #ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
@@ -830,15 +812,12 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg,
 			struct cld_net *cn = nn->cld_net;
 
 			name.len = name.len - 5;
-			memmove(name.data, name.data + 5, name.len);
+			name.data = name.data + 5;
 			cn->cn_has_legacy = true;
 		}
 #endif
-		if (!nfs4_client_to_reclaim(name, princhash, nn)) {
-			kfree(name.data);
-			kfree(princhash.data);
+		if (!nfs4_client_to_reclaim(name, princhash, nn))
 			return -EFAULT;
-		}
 		return nn->client_tracking_ops->msglen;
 	}
 	return -EFAULT;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e45f67924bd0..fa073353f30b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8801,9 +8801,6 @@ nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn)
 
 /*
  * failure => all reset bets are off, nfserr_no_grace...
- *
- * The caller is responsible for freeing name.data if NULL is returned (it
- * will be freed in nfs4_remove_reclaim_record in the normal case).
  */
 struct nfs4_client_reclaim *
 nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
@@ -8812,6 +8809,22 @@ nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
 	unsigned int strhashval;
 	struct nfs4_client_reclaim *crp;
 
+	name.data = kmemdup(name.data, name.len, GFP_KERNEL);
+	if (!name.data) {
+		dprintk("%s: failed to allocate memory for name.data!\n",
+			__func__);
+		return NULL;
+	}
+	if (princhash.len) {
+		princhash.data = kmemdup(princhash.data, princhash.len, GFP_KERNEL);
+		if (!princhash.data) {
+			dprintk("%s: failed to allocate memory for princhash.data!\n",
+				__func__);
+			kfree(name.data);
+			return NULL;
+		}
+	} else
+		princhash.data = NULL;
 	crp = alloc_reclaim();
 	if (crp) {
 		strhashval = clientstr_hashval(name);
@@ -8823,6 +8836,9 @@ nfs4_client_to_reclaim(struct xdr_netobj name, struct xdr_netobj princhash,
 		crp->cr_princhash.len = princhash.len;
 		crp->cr_clp = NULL;
 		nn->reclaim_str_hashtbl_size++;
+	} else {
+		kfree(name.data);
+		kfree(princhash.data);
 	}
 	return crp;
 }
-- 
2.50.0.107.gf914562f5916.dirty

Re: [PATCH 0/2] cleanups in nfs4reovery.c

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

On Mon, 08 Sep 2025 11:38:31 +1000, NeilBrown wrote:
> This first of these patchs is part of my work to change how directory
> locking is managed.  That will involve moving the lock as close as possible
> to the operation being locked, and using some standard interfaces
> which combine the lock and the lookup.  Then changing the mechanics of
> taking a lock.
> 
> nfsd4_list_rec_dir() currenty locks a direct and performs a lookup
> in a different function to where the lock and lookup results are needed,
> and does it even when those are not needed at all.  So the first
> patch moves the lock and lookup to where it is needed.
> 
> [...]

Applied to nfsd-testing, thanks!

[1/2] nfsd: move name lookup out of nfsd4_list_rec_dir()
      commit: 80803596e1719e88e81401e67e06245cd1133a20
[2/2] nfsd: change nfs4_client_to_reclaim() to allocate data
      commit: 1a65d347afe90f3ed49e23b7421821a3729f8bfe

--
Chuck Lever

Re: [PATCH 0/2] cleanups in nfs4reovery.c

[Jeff Layton]

On Mon, 2025-09-08 at 11:38 +1000, NeilBrown wrote:
> This first of these patchs is part of my work to change how directory
> locking is managed.  That will involve moving the lock as close as possible
> to the operation being locked, and using some standard interfaces 
> which combine the lock and the lookup.  Then changing the mechanics of
> taking a lock.
> 
> nfsd4_list_rec_dir() currenty locks a direct and performs a lookup
> in a different function to where the lock and lookup results are needed,
> and does it even when those are not needed at all.  So the first
> patch moves the lock and lookup to where it is needed.
> 
> The second patch (arguably) improves the calling protocol for
> nfs4_client_to_reclaim().  If people don't like this second patch I'm
> happy for it to be dropped.  It is the first patch which is particularly
> important to me.
> 
> Thanks,
> NeilBrown
> 
> 
>  [PATCH 1/2] nfsd: move name lookup out of nfsd4_list_rec_dir()
>  [PATCH 2/2] nfsd: change nfs4_client_to_reclaim() to allocate data

I'm fine with both of these, so:

Reviewed-by: Jeff Layton <jlayton@kernel.org>

...this does remind me though:

Is it time to switch the default for CONFIG_NFSD_LEGACY_CLIENT_TRACKING
to N? It has been a little over a year since we added the Kconfig
option (and had it default to Y).

Re: [PATCH 0/2] cleanups in nfs4reovery.c

[Chuck Lever]

On 9/8/25 3:40 PM, Jeff Layton wrote:
> On Mon, 2025-09-08 at 11:38 +1000, NeilBrown wrote:
>> This first of these patchs is part of my work to change how directory
>> locking is managed.  That will involve moving the lock as close as possible
>> to the operation being locked, and using some standard interfaces 
>> which combine the lock and the lookup.  Then changing the mechanics of
>> taking a lock.
>>
>> nfsd4_list_rec_dir() currenty locks a direct and performs a lookup
>> in a different function to where the lock and lookup results are needed,
>> and does it even when those are not needed at all.  So the first
>> patch moves the lock and lookup to where it is needed.
>>
>> The second patch (arguably) improves the calling protocol for
>> nfs4_client_to_reclaim().  If people don't like this second patch I'm
>> happy for it to be dropped.  It is the first patch which is particularly
>> important to me.
>>
>> Thanks,
>> NeilBrown
>>
>>
>>  [PATCH 1/2] nfsd: move name lookup out of nfsd4_list_rec_dir()
>>  [PATCH 2/2] nfsd: change nfs4_client_to_reclaim() to allocate data
> 
> I'm fine with both of these, so:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 
> ...this does remind me though:
> 
> Is it time to switch the default for CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> to N? It has been a little over a year since we added the Kconfig
> option (and had it default to Y).

<shrug> Send a patch? I'm not opposed.


-- 
Chuck Lever