[PATCH 00/11] nfs: improve use of wake_up_bit and wake_up_var

[PATCH 00/11] nfs: improve use of wake_up_bit and wake_up_var

[NeilBrown]

wake_up_bit and wake_up_var are fragile interfaces as they sometimes
require a barrier before them.  Recently some new interfaces were added
which avoid the need for explicit barriers.  If we can remove all
instances of those fragile interfaces, that would be ideal.

Unforunately there is one can in NFS that does not fit the new
interfaces.  However most do.  This series replaces most use of the old
interfaces with the new, and adds various related cleanups.

Thanks,
NeilBrown

[PATCH 01/11] sunrpc: remove explicit barrier from rpc_make_runnable()

[NeilBrown]

The wake_up_bit() interface is fragile as a barrier is often required
as is present here.  clear_and_wake_up_bit() is a more robust interface
as it includes the barrier and is appropriate here.

This patch rearranges the code slightly and makes use of
clear_and_wake_up_bit().  This removes some of the need to understand
barriers.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 include/linux/sunrpc/sched.h |  2 ++
 net/sunrpc/sched.c           | 13 +++++--------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index fec1e8a1570c..76e1c0194376 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -168,6 +168,8 @@ enum {
 #define RPC_IS_QUEUED(t)	test_bit(RPC_TASK_QUEUED, &(t)->tk_runstate)
 #define rpc_set_queued(t)	set_bit(RPC_TASK_QUEUED, &(t)->tk_runstate)
 #define rpc_clear_queued(t)	clear_bit(RPC_TASK_QUEUED, &(t)->tk_runstate)
+#define rpc_clear_and_wake_queued(t)	clear_and_wake_up_bit(RPC_TASK_QUEUED,	\
+							      &(t)->tk_runstate)
 
 #define RPC_IS_ACTIVATED(t)	test_bit(RPC_TASK_ACTIVE, &(t)->tk_runstate)
 
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index cef623ea1506..1b710ffc7ad6 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -361,17 +361,14 @@ EXPORT_SYMBOL_GPL(rpc_wait_for_completion_task);
 static void rpc_make_runnable(struct workqueue_struct *wq,
 		struct rpc_task *task)
 {
-	bool need_wakeup = !rpc_test_and_set_running(task);
-
-	rpc_clear_queued(task);
-	if (!need_wakeup)
-		return;
-	if (RPC_IS_ASYNC(task)) {
+	if (rpc_test_and_set_running(task))
+		rpc_clear_queued(task);
+	else if (RPC_IS_ASYNC(task)) {
+		rpc_clear_queued(task);
 		INIT_WORK(&task->u.tk_work, rpc_async_schedule);
 		queue_work(wq, &task->u.tk_work);
 	} else {
-		smp_mb__after_atomic();
-		wake_up_bit(&task->tk_runstate, RPC_TASK_QUEUED);
+		rpc_clear_and_wake_queued(task);
 	}
 }
 
-- 
2.47.0

[PATCH 02/11] sunrpc: use clear_and_wake_up_bit() for XPRT_LOCKED.

[NeilBrown]

wake_up_bit() requires a full memory barrier between the bit being
cleared and wake_up_bit() being called, else a race can result in wake
up not being sent despite another task preparing to wait.

Some paths between the clear_bit and the wake_up_bit do not have a full
barrier, such as when xprt_reserve_xprt() finds that XPRT_WRITE_SPACE is
set and clears XPRT_LOCKED before returning to xprt_release_write and
thence xprt_autoclose().

This doesn't appear to be a problem in practice as no failure reports
are known, but it seems prudent to send the wakeup immediately after the
bit is cleared and to use clear_and_wake_up_bit() which includes the
required barriers.

In most cases, if nothing is waiting for the bit the waitqueue_active()
test in wake_up_bit() will mean this does minimal extra work - though as
the waitqueue can be shared, this is not guaranteed.

Using clear_and_wake_up_bit() makes the code "obviously correct".

Signed-off-by: NeilBrown <neilb@suse.de>
---
 net/sunrpc/xprt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 09f245cda526..40385362e982 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -248,7 +248,7 @@ static void xprt_clear_locked(struct rpc_xprt *xprt)
 {
 	xprt->snd_task = NULL;
 	if (!test_bit(XPRT_CLOSE_WAIT, &xprt->state))
-		clear_bit_unlock(XPRT_LOCKED, &xprt->state);
+		clear_and_wake_up_bit(XPRT_LOCKED, &xprt->state);
 	else
 		queue_work(xprtiod_workqueue, &xprt->task_cleanup);
 }
@@ -744,7 +744,6 @@ static void xprt_autoclose(struct work_struct *work)
 	clear_bit(XPRT_CLOSE_WAIT, &xprt->state);
 	xprt->ops->close(xprt);
 	xprt_release_write(xprt, NULL);
-	wake_up_bit(&xprt->state, XPRT_LOCKED);
 	memalloc_nofs_restore(pflags);
 }
 
@@ -911,7 +910,6 @@ void xprt_unlock_connect(struct rpc_xprt *xprt, void *cookie)
 	xprt_schedule_autodisconnect(xprt);
 out:
 	spin_unlock(&xprt->transport_lock);
-	wake_up_bit(&xprt->state, XPRT_LOCKED);
 }
 EXPORT_SYMBOL_GPL(xprt_unlock_connect);
 
-- 
2.47.0

[PATCH 03/11] nfs: use clear_and_wake_up_bit().

[NeilBrown]

In three places nfs clears a bit and then sends a wake_up_bit().
Using clear_and_wake_up_bit() makes this code cleaner and avoids the
need for explicit barriers.

In 2 cases the wake_up is conditional on a "congested" flag.  For these
we use the flag to select between clear_bit and clear_and_wake_up_bit.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/inode.c    |  4 +---
 fs/nfs/pagelist.c | 14 ++++++--------
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 596f35170137..4c4c3ab57fcd 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1451,9 +1451,7 @@ int nfs_clear_invalid_mapping(struct address_space *mapping)
 	ret = nfs_invalidate_mapping(inode, mapping);
 	trace_nfs_invalidate_mapping_exit(inode, ret);
 
-	clear_bit_unlock(NFS_INO_INVALIDATING, bitlock);
-	smp_mb__after_atomic();
-	wake_up_bit(bitlock, NFS_INO_INVALIDATING);
+	clear_and_wake_up_bit(NFS_INO_INVALIDATING, bitlock);
 out:
 	return ret;
 }
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index e27c07bd8929..7f3914064cee 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -214,11 +214,10 @@ nfs_page_set_headlock(struct nfs_page *req)
 void
 nfs_page_clear_headlock(struct nfs_page *req)
 {
-	clear_bit_unlock(PG_HEADLOCK, &req->wb_flags);
-	smp_mb__after_atomic();
 	if (!test_bit(PG_CONTENDED1, &req->wb_flags))
-		return;
-	wake_up_bit(&req->wb_flags, PG_HEADLOCK);
+		clear_bit_unlock(PG_HEADLOCK, &req->wb_flags);
+	else
+		clear_and_wake_up_bit(PG_HEADLOCK, &req->wb_flags);
 }
 
 /*
@@ -519,11 +518,10 @@ nfs_create_subreq(struct nfs_page *req,
  */
 void nfs_unlock_request(struct nfs_page *req)
 {
-	clear_bit_unlock(PG_BUSY, &req->wb_flags);
-	smp_mb__after_atomic();
 	if (!test_bit(PG_CONTENDED2, &req->wb_flags))
-		return;
-	wake_up_bit(&req->wb_flags, PG_BUSY);
+		clear_bit_unlock(PG_BUSY, &req->wb_flags);
+	else
+		clear_and_wake_up_bit(PG_BUSY, &req->wb_flags);
 }
 
 /**
-- 
2.47.0

[PATCH 04/11] nfs: combine NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK

[NeilBrown]

The flags NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK are effectively
identical.
The only time either are cleared is in pnfs_clear_layoutreturn_waitbit(),
and there both are cleared.
The only time NFS_LAYOUT_RETURN is set is in pnfs_prepare_layoutreturn()
immediately after NFS_LAYOUT_RETURN_LOCK was set.
The only other time that NFS_LAYOUT_RETURN_LOCK is set is in
pnfs_mark_layout_stateid_invalid() if NFS_LAYOUT_RETURN was set but
NFS_LAYOUT_RETURN_LOCK was not set - but that is an impossible
combination given that else where the flags are set or cleared together.

So we only need one of these flags.  This patch discards
NFS_LAYOUT_RETURN_LOCK and does the test_and_set needed for exclusion with
NFS_LAYOUT_RETURN.

Also the wake_up_bit in pnfs_clear_layoutreturn_waitbit() is changed to
clear_and_wake_up_bit() which includes all needed barriers internally.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/pnfs.c | 15 ++++-----------
 fs/nfs/pnfs.h |  1 -
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 0d16b383a452..5963c0440e23 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -386,10 +386,7 @@ pnfs_clear_layoutreturn_info(struct pnfs_layout_hdr *lo)
 
 static void pnfs_clear_layoutreturn_waitbit(struct pnfs_layout_hdr *lo)
 {
-	clear_bit_unlock(NFS_LAYOUT_RETURN, &lo->plh_flags);
-	clear_bit(NFS_LAYOUT_RETURN_LOCK, &lo->plh_flags);
-	smp_mb__after_atomic();
-	wake_up_bit(&lo->plh_flags, NFS_LAYOUT_RETURN);
+	clear_and_wake_up_bit(NFS_LAYOUT_RETURN, &lo->plh_flags);
 	rpc_wake_up(&NFS_SERVER(lo->plh_inode)->roc_rpcwaitq);
 }
 
@@ -471,9 +468,6 @@ pnfs_mark_layout_stateid_invalid(struct pnfs_layout_hdr *lo,
 	pnfs_clear_layoutreturn_info(lo);
 	pnfs_free_returned_lsegs(lo, lseg_list, &range, 0);
 	set_bit(NFS_LAYOUT_DRAIN, &lo->plh_flags);
-	if (test_bit(NFS_LAYOUT_RETURN, &lo->plh_flags) &&
-	    !test_and_set_bit(NFS_LAYOUT_RETURN_LOCK, &lo->plh_flags))
-		pnfs_clear_layoutreturn_waitbit(lo);
 	return !list_empty(&lo->plh_segs);
 }
 
@@ -1310,9 +1304,8 @@ pnfs_prepare_layoutreturn(struct pnfs_layout_hdr *lo,
 	/* Serialise LAYOUTGET/LAYOUTRETURN */
 	if (atomic_read(&lo->plh_outstanding) != 0)
 		return false;
-	if (test_and_set_bit(NFS_LAYOUT_RETURN_LOCK, &lo->plh_flags))
+	if (test_and_set_bit(NFS_LAYOUT_RETURN, &lo->plh_flags))
 		return false;
-	set_bit(NFS_LAYOUT_RETURN, &lo->plh_flags);
 	pnfs_get_layout_hdr(lo);
 	nfs4_stateid_copy(stateid, &lo->plh_stateid);
 	*cred = get_cred(lo->plh_lc_cred);
@@ -1454,7 +1447,7 @@ _pnfs_return_layout(struct inode *ino)
 	/* Reference matched in nfs4_layoutreturn_release */
 	pnfs_get_layout_hdr(lo);
 	/* Is there an outstanding layoutreturn ? */
-	if (test_bit(NFS_LAYOUT_RETURN_LOCK, &lo->plh_flags)) {
+	if (test_bit(NFS_LAYOUT_RETURN, &lo->plh_flags)) {
 		spin_unlock(&ino->i_lock);
 		if (wait_on_bit(&lo->plh_flags, NFS_LAYOUT_RETURN,
 					TASK_UNINTERRUPTIBLE))
@@ -1564,7 +1557,7 @@ bool pnfs_roc(struct inode *ino,
 		goto out_noroc;
 	}
 	pnfs_get_layout_hdr(lo);
-	if (test_bit(NFS_LAYOUT_RETURN_LOCK, &lo->plh_flags)) {
+	if (test_bit(NFS_LAYOUT_RETURN, &lo->plh_flags)) {
 		spin_unlock(&ino->i_lock);
 		rcu_read_unlock();
 		wait_on_bit(&lo->plh_flags, NFS_LAYOUT_RETURN,
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 30d2613e912b..df914f17b927 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -100,7 +100,6 @@ enum {
 	NFS_LAYOUT_RW_FAILED,		/* get rw layout failed stop trying */
 	NFS_LAYOUT_BULK_RECALL,		/* bulk recall affecting layout */
 	NFS_LAYOUT_RETURN,		/* layoutreturn in progress */
-	NFS_LAYOUT_RETURN_LOCK,		/* Serialise layoutreturn */
 	NFS_LAYOUT_RETURN_REQUESTED,	/* Return this layout ASAP */
 	NFS_LAYOUT_INVALID_STID,	/* layout stateid id is invalid */
 	NFS_LAYOUT_FIRST_LAYOUTGET,	/* Serialize first layoutget */
-- 
2.47.0

[PATCH 05/11] nfs: use clear_and_wake_up_bit() in pnfs code

[NeilBrown]

The wake_up_bit() interface is fragile as it sometimes needs an explicit
barrier before it is called.  It is generally better to use the combined
interfaces which have all necessary barriers.

The usage of wake_up_bit() in NFS/pnfs IS safe as the required barriers
are included.  But it is ugly to need that explicit barrier.

This patch changes to use clear_and_wake_up_bit() which transparently
includes the required barrier.  Also use test_and_clear_wake_up_bit() in
one case.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/pnfs.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 5963c0440e23..445ba09ba324 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2034,9 +2034,8 @@ static void nfs_layoutget_begin(struct pnfs_layout_hdr *lo)
 
 static void nfs_layoutget_end(struct pnfs_layout_hdr *lo)
 {
-	if (atomic_dec_and_test(&lo->plh_outstanding) &&
-	    test_and_clear_bit(NFS_LAYOUT_DRAIN, &lo->plh_flags))
-		wake_up_bit(&lo->plh_flags, NFS_LAYOUT_DRAIN);
+	if (atomic_dec_and_test(&lo->plh_outstanding))
+		test_and_clear_wake_up_bit(NFS_LAYOUT_DRAIN, &lo->plh_flags);
 }
 
 static bool pnfs_is_first_layoutget(struct pnfs_layout_hdr *lo)
@@ -2048,9 +2047,7 @@ static void pnfs_clear_first_layoutget(struct pnfs_layout_hdr *lo)
 {
 	unsigned long *bitlock = &lo->plh_flags;
 
-	clear_bit_unlock(NFS_LAYOUT_FIRST_LAYOUTGET, bitlock);
-	smp_mb__after_atomic();
-	wake_up_bit(bitlock, NFS_LAYOUT_FIRST_LAYOUTGET);
+	clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET, bitlock);
 }
 
 static void _add_to_server_list(struct pnfs_layout_hdr *lo,
@@ -3221,9 +3218,7 @@ static void pnfs_clear_layoutcommitting(struct inode *inode)
 {
 	unsigned long *bitlock = &NFS_I(inode)->flags;
 
-	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
-	smp_mb__after_atomic();
-	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
+	clear_and_wake_up_bit(NFS_INO_LAYOUTCOMMITTING, bitlock);
 }
 
 /*
-- 
2.47.0

[PATCH 06/11] nfs: use store_release_wake_up() for clearing d_fsdata

[NeilBrown]

The barrier provided by smp_store_release() is before the store, while
wake_up_var() needs a full barrier *after* the store.

The new store_release_wake_up() interface encodes all the barriers
making this sort of bug harder to write.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/dir.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 492cffd9d3d8..ded86facef8f 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1837,9 +1837,7 @@ static void block_revalidate(struct dentry *dentry)
 
 static void unblock_revalidate(struct dentry *dentry)
 {
-	/* store_release ensures wait_var_event() sees the update */
-	smp_store_release(&dentry->d_fsdata, NULL);
-	wake_up_var(&dentry->d_fsdata);
+	store_release_wake_up(&dentry->d_fsdata, NULL);
 }
 
 /*
-- 
2.47.0

[PATCH 07/11] sunrpc: discard rpc_wait_bit_killable()

[NeilBrown]

rpc_wait_bit_kill() currently differs from bit_wait() in the it returns
-ERESTARTSYS rather then -EINTR.  The sunrpc and nfs code never really
care about the difference.  The error could get up to user-space but it
is only generated when a process is being killed, in which case there is
no user-space to see the difference.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 net/sunrpc/sched.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 1b710ffc7ad6..0618dc586009 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -274,14 +274,6 @@ void rpc_destroy_wait_queue(struct rpc_wait_queue *queue)
 }
 EXPORT_SYMBOL_GPL(rpc_destroy_wait_queue);
 
-static int rpc_wait_bit_killable(struct wait_bit_key *key, int mode)
-{
-	schedule();
-	if (signal_pending_state(mode, current))
-		return -ERESTARTSYS;
-	return 0;
-}
-
 #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) || IS_ENABLED(CONFIG_TRACEPOINTS)
 static void rpc_task_set_debuginfo(struct rpc_task *task)
 {
@@ -343,7 +335,7 @@ static int rpc_complete_task(struct rpc_task *task)
 int rpc_wait_for_completion_task(struct rpc_task *task)
 {
 	return out_of_line_wait_on_bit(&task->tk_runstate, RPC_TASK_ACTIVE,
-			rpc_wait_bit_killable, TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
+			bit_wait, TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 }
 EXPORT_SYMBOL_GPL(rpc_wait_for_completion_task);
 
@@ -982,12 +974,12 @@ static void __rpc_execute(struct rpc_task *task)
 		/* sync task: sleep here */
 		trace_rpc_task_sync_sleep(task, task->tk_action);
 		status = out_of_line_wait_on_bit(&task->tk_runstate,
-				RPC_TASK_QUEUED, rpc_wait_bit_killable,
+				RPC_TASK_QUEUED, bit_wait,
 				TASK_KILLABLE|TASK_FREEZABLE);
 		if (status < 0) {
 			/*
 			 * When a sync task receives a signal, it exits with
-			 * -ERESTARTSYS. In order to catch any callbacks that
+			 * -EINTR. In order to catch any callbacks that
 			 * clean up after sleeping on some queue, we don't
 			 * break the loop here, but go around once more.
 			 */
-- 
2.47.0

[PATCH 08/11] nfs: discard nfs_wait_bit_killable()

[NeilBrown]

This patch changes NFS to use wait_on_bit() instead of
wait_on_bit_action()
nfs_wait_bit_killable() is identical to bit_wait() except that it
returns -ERESTARTSYS instead of -EINTR.  NFS doesn't care about this
distinction.  The status will often get back to user-space but it will
only be sent when the process is being killed in which case there is no
user-space to care.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/file.c      |  5 ++---
 fs/nfs/inode.c     | 14 ++------------
 fs/nfs/internal.h  |  1 -
 fs/nfs/nfs4state.c |  5 ++---
 fs/nfs/pnfs.c      |  9 +++------
 5 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 1bb646752e46..0fafdfec5886 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -607,9 +607,8 @@ static vm_fault_t nfs_vm_page_mkwrite(struct vm_fault *vmf)
 		goto out;
 	}
 
-	wait_on_bit_action(&NFS_I(inode)->flags, NFS_INO_INVALIDATING,
-			   nfs_wait_bit_killable,
-			   TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
+	wait_on_bit(&NFS_I(inode)->flags, NFS_INO_INVALIDATING,
+		    TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 
 	folio_lock(folio);
 	mapping = folio->mapping;
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 4c4c3ab57fcd..2f1b4f11a056 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -72,15 +72,6 @@ nfs_fattr_to_ino_t(struct nfs_fattr *fattr)
 	return nfs_fileid_to_ino_t(fattr->fileid);
 }
 
-int nfs_wait_bit_killable(struct wait_bit_key *key, int mode)
-{
-	schedule();
-	if (signal_pending_state(mode, current))
-		return -ERESTARTSYS;
-	return 0;
-}
-EXPORT_SYMBOL_GPL(nfs_wait_bit_killable);
-
 /**
  * nfs_compat_user_ino64 - returns the user-visible inode number
  * @fileid: 64-bit fileid
@@ -1419,9 +1410,8 @@ int nfs_clear_invalid_mapping(struct address_space *mapping)
 	 * the bit lock here if it looks like we're going to be doing that.
 	 */
 	for (;;) {
-		ret = wait_on_bit_action(bitlock, NFS_INO_INVALIDATING,
-					 nfs_wait_bit_killable,
-					 TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
+		ret = wait_on_bit(bitlock, NFS_INO_INVALIDATING,
+				  TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 		if (ret)
 			goto out;
 		smp_rmb(); /* pairs with smp_wmb() below */
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index e564bd11ba60..1ec10fa50830 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -451,7 +451,6 @@ extern void nfs_evict_inode(struct inode *);
 extern void nfs_zap_acl_cache(struct inode *inode);
 extern void nfs_set_cache_invalid(struct inode *inode, unsigned long flags);
 extern bool nfs_check_cache_invalid(struct inode *, unsigned long);
-extern int nfs_wait_bit_killable(struct wait_bit_key *key, int mode);
 
 #if IS_ENABLED(CONFIG_NFS_LOCALIO)
 /* localio.c */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 9a9f60a2291b..556b521f17eb 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1313,9 +1313,8 @@ int nfs4_wait_clnt_recover(struct nfs_client *clp)
 	might_sleep();
 
 	refcount_inc(&clp->cl_count);
-	res = wait_on_bit_action(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,
-				 nfs_wait_bit_killable,
-				 TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
+	res = wait_on_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,
+			  TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 	if (res)
 		goto out;
 	if (clp->cl_cons_state < 0)
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 445ba09ba324..400f409f45fa 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2022,9 +2022,8 @@ static int pnfs_prepare_to_retry_layoutget(struct pnfs_layout_hdr *lo)
 	 * reference
 	 */
 	pnfs_layoutcommit_inode(lo->plh_inode, false);
-	return wait_on_bit_action(&lo->plh_flags, NFS_LAYOUT_RETURN,
-				   nfs_wait_bit_killable,
-				   TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
+	return wait_on_bit(&lo->plh_flags, NFS_LAYOUT_RETURN,
+			   TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 }
 
 static void nfs_layoutget_begin(struct pnfs_layout_hdr *lo)
@@ -3319,9 +3318,8 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 	if (test_and_set_bit(NFS_INO_LAYOUTCOMMITTING, &nfsi->flags)) {
 		if (!sync)
 			goto out;
-		status = wait_on_bit_lock_action(&nfsi->flags,
+		status = wait_on_bit_lock(&nfsi->flags,
 				NFS_INO_LAYOUTCOMMITTING,
-				nfs_wait_bit_killable,
 				TASK_KILLABLE|TASK_FREEZABLE_UNSAFE);
 		if (status)
 			goto out;
@@ -3369,7 +3367,6 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
 		}
 	}
 
-
 	status = nfs4_proc_layoutcommit(data, sync);
 out:
 	if (status)
-- 
2.47.0

[PATCH 09/11] nfs: add memory barrier before calling wake_up_var on cl_state

[NeilBrown]

After setting NFS4CLNT_RUN_MANAGER we need a full memory barrier before
it is safe to call wake_up_var().  As that setting is "atomic" it is
sufficient to use smp_mb__after_atomic().

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/nfs4proc.c  | 1 +
 fs/nfs/nfs4state.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 405f17e6e0b4..37c8aa1f3e1b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -10847,6 +10847,7 @@ static void nfs4_disable_swap(struct inode *inode)
 
 	set_bit(NFS4CLNT_RUN_MANAGER, &clp->cl_state);
 	clear_bit(NFS4CLNT_MANAGER_AVAILABLE, &clp->cl_state);
+	smp_mb__after_atomic();
 	wake_up_var(&clp->cl_state);
 }
 
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 556b521f17eb..189d7b57cb74 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1207,6 +1207,7 @@ void nfs4_schedule_state_manager(struct nfs_client *clp)
 		swapon = !test_and_set_bit(NFS4CLNT_MANAGER_AVAILABLE,
 					   &clp->cl_state);
 		if (!swapon) {
+			smp_mb__after_atomic();
 			wake_up_var(&clp->cl_state);
 			return;
 		}
-- 
2.47.0

[PATCH 10/11] nfs: use atomic_dec_and_wake_up()

[NeilBrown]

In two places nfs uses wake_up_var() after atomic_dec_and_test() on the
same var.  This is correct as no extra barriers are needed in this case.
This can be made more clear by using the atomic_dec_and_wake_up()
interface.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/pagelist.c | 3 +--
 fs/nfs/write.c    | 6 +-----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 7f3914064cee..a1b4c77cbc68 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -557,8 +557,7 @@ static void nfs_clear_request(struct nfs_page *req)
 		req->wb_page = NULL;
 	}
 	if (l_ctx != NULL) {
-		if (atomic_dec_and_test(&l_ctx->io_count)) {
-			wake_up_var(&l_ctx->io_count);
+		if (atomic_dec_and_wake_up(&l_ctx->io_count)) {
 			ctx = l_ctx->open_context;
 			if (test_bit(NFS_CONTEXT_UNLOCK, &ctx->flags))
 				rpc_wake_up(&NFS_SERVER(d_inode(ctx->dentry))->uoc_rpcwaitq);
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 50fa539611f5..3b709cfff0da 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1658,11 +1658,7 @@ void nfs_commit_begin(struct nfs_mds_commit_info *cinfo)
 
 bool nfs_commit_end(struct nfs_mds_commit_info *cinfo)
 {
-	if (atomic_dec_and_test(&cinfo->rpcs_out)) {
-		wake_up_var(&cinfo->rpcs_out);
-		return true;
-	}
-	return false;
+	return atomic_dec_and_wake_up(&cinfo->rpcs_out);
 }
 
 void nfs_commitdata_release(struct nfs_commit_data *data)
-- 
2.47.0

[PATCH 11/11] nfs: use wait_var_event_spinlock() to wait for nfsi->layout to change.

[NeilBrown]

The recently added interface wait_var_event_spinlock() is designed for
waiting for events that can only be checked under a spinlock.
This matches the requirements for waiting for nfsi->layout so we can
change to using wait_var_event_spinlock().

This avoids a use of plain wake_up_var() which always needs to be
checked for correct barrier usage.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfs/pnfs.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 400f409f45fa..04be1165a1c5 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -306,7 +306,6 @@ void
 pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 {
 	struct inode *inode;
-	unsigned long i_state;
 
 	if (!lo)
 		return;
@@ -317,12 +316,11 @@ pnfs_put_layout_hdr(struct pnfs_layout_hdr *lo)
 		if (!list_empty(&lo->plh_segs))
 			WARN_ONCE(1, "NFS: BUG unfreed layout segments.\n");
 		pnfs_detach_layout_hdr(lo);
-		i_state = inode->i_state;
+		/* Notify pnfs_destroy_layout_final() that we're done */
+		if (inode->i_state & (I_FREEING | I_CLEAR))
+			wake_up_var_locked(lo, &inode->i_lock);
 		spin_unlock(&inode->i_lock);
 		pnfs_free_layout_hdr(lo);
-		/* Notify pnfs_destroy_layout_final() that we're done */
-		if (i_state & (I_FREEING | I_CLEAR))
-			wake_up_var(lo);
 	}
 }
 
@@ -795,23 +793,16 @@ void pnfs_destroy_layout(struct nfs_inode *nfsi)
 }
 EXPORT_SYMBOL_GPL(pnfs_destroy_layout);
 
-static bool pnfs_layout_removed(struct nfs_inode *nfsi,
-				struct pnfs_layout_hdr *lo)
-{
-	bool ret;
-
-	spin_lock(&nfsi->vfs_inode.i_lock);
-	ret = nfsi->layout != lo;
-	spin_unlock(&nfsi->vfs_inode.i_lock);
-	return ret;
-}
-
 void pnfs_destroy_layout_final(struct nfs_inode *nfsi)
 {
 	struct pnfs_layout_hdr *lo = __pnfs_destroy_layout(nfsi);
 
-	if (lo)
-		wait_var_event(lo, pnfs_layout_removed(nfsi, lo));
+	if (lo) {
+		spin_lock(&nfsi->vfs_inode.i_lock);
+		wait_var_event_spinlock(lo, nfsi->layout != lo,
+					&nfsi->vfs_inode.i_lock);
+		spin_unlock(&nfsi->vfs_inode.i_lock);
+	}
 }
 
 static bool
-- 
2.47.0

Re: [PATCH 04/11] nfs: combine NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK

[Mike Snitzer]

On Fri, Dec 06, 2024 at 01:15:30PM +1100, NeilBrown wrote:
> The flags NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK are effectively
> identical.
> The only time either are cleared is in pnfs_clear_layoutreturn_waitbit(),
> and there both are cleared.
> The only time NFS_LAYOUT_RETURN is set is in pnfs_prepare_layoutreturn()
> immediately after NFS_LAYOUT_RETURN_LOCK was set.
> The only other time that NFS_LAYOUT_RETURN_LOCK is set is in
> pnfs_mark_layout_stateid_invalid() if NFS_LAYOUT_RETURN was set but
> NFS_LAYOUT_RETURN_LOCK was not set - but that is an impossible
> combination given that else where the flags are set or cleared together.
> 
> So we only need one of these flags.  This patch discards
> NFS_LAYOUT_RETURN_LOCK and does the test_and_set needed for exclusion with
> NFS_LAYOUT_RETURN.
> 
> Also the wake_up_bit in pnfs_clear_layoutreturn_waitbit() is changed to
> clear_and_wake_up_bit() which includes all needed barriers internally.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>

I appreciate that you've done a general audit of the NFS code and
looked to improve / optimize the wake_up_bit() callers, etc.  But how
did you test this specific patch's changes to the pnfs code?

Reason I ask is if you look at the commit that introduced
NFS_LAYOUT_RETURN_LOCK way back when:
6604b203fb63 pNFS: On error, do not send LAYOUTGET until the LAYOUTRETURN has completed

You'll see that, with your patch, you've seem to have now reverted the
code back to before stable@ commit 6604b203fb63 was applied.

Now there may be merit to doing that due to other changes in the pnfs
code that didn't exist back then but... your changes look suspicious
given the evolution of this code.

Mike

Re: [PATCH 04/11] nfs: combine NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK

[NeilBrown]

On Thu, 23 Jan 2025, Mike Snitzer wrote:
> On Fri, Dec 06, 2024 at 01:15:30PM +1100, NeilBrown wrote:
> > The flags NFS_LAYOUT_RETURN and NFS_LAYOUT_RETURN_LOCK are effectively
> > identical.
> > The only time either are cleared is in pnfs_clear_layoutreturn_waitbit(),
> > and there both are cleared.
> > The only time NFS_LAYOUT_RETURN is set is in pnfs_prepare_layoutreturn()
> > immediately after NFS_LAYOUT_RETURN_LOCK was set.
> > The only other time that NFS_LAYOUT_RETURN_LOCK is set is in
> > pnfs_mark_layout_stateid_invalid() if NFS_LAYOUT_RETURN was set but
> > NFS_LAYOUT_RETURN_LOCK was not set - but that is an impossible
> > combination given that else where the flags are set or cleared together.
> > 
> > So we only need one of these flags.  This patch discards
> > NFS_LAYOUT_RETURN_LOCK and does the test_and_set needed for exclusion with
> > NFS_LAYOUT_RETURN.
> > 
> > Also the wake_up_bit in pnfs_clear_layoutreturn_waitbit() is changed to
> > clear_and_wake_up_bit() which includes all needed barriers internally.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> 
> I appreciate that you've done a general audit of the NFS code and
> looked to improve / optimize the wake_up_bit() callers, etc.  But how
> did you test this specific patch's changes to the pnfs code?

mumble mumble mumble

> 
> Reason I ask is if you look at the commit that introduced
> NFS_LAYOUT_RETURN_LOCK way back when:
> 6604b203fb63 pNFS: On error, do not send LAYOUTGET until the LAYOUTRETURN has completed

That commit was effectively reverted by 
Commit: 61f454e30c18 ("pNFS: Fix a deadlock when coalescing writes and returning the layout")

I don't pretend to understand exactly what is going on with these flags 
but I'm fairly confident that I didn't change behaviour that I didn't
intent to change.
And now that I found that commit I'm even more confident that
NFS_LAYOUT_RETURN_LOCK isn't needed.

Thanks for reviewing my code !!

NeilBrown

> 
> You'll see that, with your patch, you've seem to have now reverted the
> code back to before stable@ commit 6604b203fb63 was applied.
> 
> Now there may be merit to doing that due to other changes in the pnfs
> code that didn't exist back then but... your changes look suspicious
> given the evolution of this code.
> 
> Mike
> 
>