[PATCH v2 0/3] NFS: don't release an open context from writeback

[PATCH v2 0/3] NFS: don't release an open context from writeback

[Mike Snitzer]

v1 deferred the final deactivate_super() in nfs_sb_deactive() to nfsiod:
https://lore.kernel.org/linux-nfs/20260911184239.90154-1-snitzer@kernel.org/

Feedback on v1 was that it worked around the problem rather than fixing
it. That is fair: v1 made the last step of releasing an open context
safe to run from writeback, when the problem is that writeback releases
the open context at all. Testing bears that out, see below: with v1
applied the flusher still deadlocks, one step earlier, on I_SYNC.

Releasing an open context closes NFSv4 state and drops what may be the
last reference to the dentry, the inode and the superblock. The flusher
holds sb->s_umount and has set I_SYNC on the inode, so it can end up
waiting for itself on either. Writes normally avoid this because their
requests are released by the RPC release callback, on nfsiod. This
series fixes the three ways writeback does not:

Patch 1: a write that fails before it is sent is completed by
nfs_write_error() in the submitter. This is the deadlock that was hit
in the field, on a client whose server began returning AUTH_TOOWEAK.

Patch 2: nfs_initiate_pgio() and nfs_initiate_commit() drop their task
reference with rpc_put_task(), which runs the release callback in the
submitter if the task has already completed. This is the deadlock that
NeilBrown reported in 2014:
https://lore.kernel.org/linux-nfs/20140407135001.56ef9f36@notabene.brown/

Patch 3: the asynchronous LAYOUTCOMMIT that ->write_inode() sends has
the same problem as patch 2, with an inode and superblock reference of
its own in place of an open context, and no workqueue at all.

All three move the release to nfsiod, where every other write already
does it. None changes how or when a filesystem is shut down, and
nfs_sb_deactive() is left as it is.

Testing: NFSv3 over loopback, with a test-only knob that makes
nfs_do_writepage() see -EACCES in the pageio descriptor. Dirty a file
through a mapping after closing it, unmount, and let the periodic
flusher write it back. A plain umount is enough to leave the write
requests as the only thing keeping the superblock alive.

                  plain file           sillyrenamed file
  v1              shuts down, from     flusher hangs in evict(),
                  v1's work item       waiting for I_SYNC

  this series     shuts down, from     shuts down, from the release
                  the context's        of the sillyrename REMOVE
                  work item

Patches 2 and 3 close races and were not exercised by this test.

Changes since v1:
- Dropped "NFS: defer the final superblock deactivation".
- Fix the three places that release such references from writeback
  instead.
- Patch 1 carries a Fixes: tag; v1 had none.

Mike Snitzer (3):
  NFS: don't release the open context of a failed write from writeback
  NFS: don't run the release of a WRITE or COMMIT in the submitter
  NFSv4/pnfs: don't run the release of a LAYOUTCOMMIT in the submitter

 fs/nfs/inode.c         | 37 ++++++++++++++++++++++++++++++++++---
 fs/nfs/internal.h      |  1 +
 fs/nfs/nfs4proc.c      |  7 ++++++-
 fs/nfs/pagelist.c      |  3 ++-
 fs/nfs/write.c         |  8 +++++++-
 include/linux/nfs_fs.h |  2 ++
 6 files changed, 52 insertions(+), 6 deletions(-)


base-commit: 82e951e8628cdc0a5434c6f71ff1566b20e9912c
-- 
2.52.0

[PATCH v2 1/3] NFS: don't release the open context of a failed write from writeback

[Mike Snitzer]

When a write fails with an error that nfs_error_is_fatal_on_server()
accepts, nfs_do_writepage() and nfs_async_write_error() complete the
request in the submitting thread: nfs_write_error() removes the request
from the inode and drops the last reference to it. The request owns a
reference to its open context, so if the file has already been closed
this is also where the open context is released.

Releasing an open context closes NFSv4 state and drops what may be the
last reference to the dentry, the inode and the superblock. The
submitting thread is usually the flusher, which may do none of that:

 - It holds sb->s_umount shared, or works on behalf of a sync(2) caller
   that does. An open context does not keep a filesystem mounted, so
   once it has been unmounted the open context can hold the last active
   reference to the superblock, and deactivate_super() then waits for
   s_umount forever:

     wb_workfn
       __writeback_inodes_wb
         super_trylock_shared(sb)          <- s_umount held shared
         writeback_sb_inodes
           nfs_writepages
             nfs_do_writepage              <- fatal error: out_launder
               nfs_write_error
                 nfs_release_request
                   nfs_put_lock_context
                     __put_nfs_open_context
                       nfs_sb_deactive
                         deactivate_super
                           down_write(&sb->s_umount)   <- deadlock

 - Since commit 169ebd90131b ("writeback: Avoid iput() from flusher
   thread") it pins the inode with I_SYNC instead of a reference, so
   that it never does the final iput(). evict() waits for I_SYNC, so if
   dput() drops the last reference to a stale or sillyrenamed inode,
   the flusher waits for itself:

     wb_workfn
       __writeback_inodes_wb
         writeback_sb_inodes               <- sets I_SYNC
           nfs_writepages
             nfs_do_writepage
               nfs_write_error
                 nfs_release_request
                   nfs_put_lock_context
                     __put_nfs_open_context
                       dput
                         nfs_dentry_iput
                           iput
                             evict
                               inode_wait_for_writeback   <- deadlock

The first of these was hit on a 6.12-based client whose server began
rejecting requests with AUTH_TOOWEAK (-EACCES). The flush worker hung in
deactivate_super(), and every later automount of the same filesystem
blocked in grab_super() behind it. The second was reproduced by failing
the writeback of a sillyrenamed file that had been closed.

A write that was sent is normally released by the RPC release callback,
which runs on nfsiod; the next patch closes the case where it is not.
Do the same for a write that fails before it is sent. Have
nfs_write_error() hold the open context across the release of the
request, and drop that reference with put_nfs_open_context_async(),
which hands the release of the context to nfsiod if the reference is
the last one.

This does not change when or how a filesystem is shut down. Since
commit 3d4ff43d895c ("nfs_open_context doesn't need struct path
either") an open context pins the superblock instead of a mount, so an
open context that outlives the last mount shuts the filesystem down
when it is released. The completion of a write already does that from
nfsiod.

Fixes: a6598813a4c5 ("NFS: Don't write back further requests if there is a pending write error")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/inode.c         | 37 ++++++++++++++++++++++++++++++++++---
 fs/nfs/internal.h      |  1 +
 fs/nfs/write.c         |  5 +++++
 include/linux/nfs_fs.h |  2 ++
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 3022454f7698..c91334dd7e3e 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1231,13 +1231,11 @@ struct nfs_open_context *get_nfs_open_context(struct nfs_open_context *ctx)
 }
 EXPORT_SYMBOL_GPL(get_nfs_open_context);
 
-static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync)
+static void nfs_free_open_context(struct nfs_open_context *ctx, int is_sync)
 {
 	struct inode *inode = d_inode(ctx->dentry);
 	struct super_block *sb = ctx->dentry->d_sb;
 
-	if (!refcount_dec_and_test(&ctx->lock_context.count))
-		return;
 	if (!list_empty(&ctx->list)) {
 		spin_lock(&inode->i_lock);
 		list_del_rcu(&ctx->list);
@@ -1254,12 +1252,45 @@ static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync)
 	kfree_rcu(ctx, rcu_head);
 }
 
+static void __put_nfs_open_context(struct nfs_open_context *ctx, int is_sync)
+{
+	if (refcount_dec_and_test(&ctx->lock_context.count))
+		nfs_free_open_context(ctx, is_sync);
+}
+
 void put_nfs_open_context(struct nfs_open_context *ctx)
 {
 	__put_nfs_open_context(ctx, 0);
 }
 EXPORT_SYMBOL_GPL(put_nfs_open_context);
 
+static void nfs_free_open_context_work(struct work_struct *work)
+{
+	struct nfs_open_context *ctx =
+		container_of(work, struct nfs_open_context, free_work);
+
+	nfs_free_open_context(ctx, 0);
+}
+
+/**
+ * put_nfs_open_context_async - drop a reference from a caller that must not
+ *				release the open context itself
+ * @ctx: open context to drop
+ *
+ * Releasing an open context closes NFSv4 state and drops what may be the
+ * last reference to the dentry, the inode and the superblock. None of that
+ * is allowed in writeback, which holds sb->s_umount and has set I_SYNC on
+ * the inode. If this is the last reference, release the context from nfsiod,
+ * which is where the completion of a write releases it.
+ */
+void put_nfs_open_context_async(struct nfs_open_context *ctx)
+{
+	if (!refcount_dec_and_test(&ctx->lock_context.count))
+		return;
+	INIT_WORK(&ctx->free_work, nfs_free_open_context_work);
+	queue_work(nfsiod_workqueue, &ctx->free_work);
+}
+
 static void put_nfs_open_context_sync(struct nfs_open_context *ctx)
 {
 	__put_nfs_open_context(ctx, 1);
diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index 48f7c0e25da1..94d61a42c7ee 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -525,6 +525,7 @@ extern int __init register_nfs_fs(void);
 extern void __exit unregister_nfs_fs(void);
 extern bool nfs_sb_active(struct super_block *sb);
 extern void nfs_sb_deactive(struct super_block *sb);
+extern void put_nfs_open_context_async(struct nfs_open_context *ctx);
 extern int nfs_client_for_each_server(struct nfs_client *clp,
 				      int (*fn)(struct nfs_server *, void *),
 				      void *data);
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 7f173dbbeb9d..f38bbebffe4d 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -568,11 +568,16 @@ static struct nfs_page *nfs_lock_and_join_requests(struct folio *folio)
 
 static void nfs_write_error(struct nfs_page *req, int error)
 {
+	struct nfs_open_context *ctx;
+
+	/* The caller may be writeback, which must not release the context */
+	ctx = get_nfs_open_context(nfs_req_openctx(req));
 	trace_nfs_write_error(nfs_page_to_inode(req), req, error);
 	nfs_mapping_set_error(nfs_page_to_folio(req), error);
 	nfs_inode_remove_request(req);
 	nfs_page_end_writeback(req);
 	nfs_release_request(req);
+	put_nfs_open_context_async(ctx);
 }
 
 /*
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index b85a73ae7919..32a46de106cd 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -26,6 +26,7 @@
 #include <linux/refcount.h>
 #include <linux/rwsem.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 
 #include <linux/sunrpc/debug.h>
 #include <linux/sunrpc/auth.h>
@@ -115,6 +116,7 @@ struct nfs_open_context {
 	struct nfs4_threshold	*mdsthreshold;
 	struct list_head list;
 	struct rcu_head	rcu_head;
+	struct work_struct free_work;
 	struct nfs_file_localio nfl;
 };
 
-- 
2.52.0

[PATCH v2 2/3] NFS: don't run the release of a WRITE or COMMIT in the submitter

[Mike Snitzer]

nfs_initiate_pgio() and nfs_initiate_commit() start an asynchronous RPC
task and then drop their reference to it with rpc_put_task(). If the
task has already completed, that reference is the last one and
rpc_put_task() runs the rpc_release() callback in the submitter rather
than on nfsiod, where task_setup_data.workqueue asks for it to run.

The release of a WRITE or COMMIT releases write requests and, with
them, what may be the last reference to an open context. The submitter
is usually the flusher, which holds sb->s_umount and has set I_SYNC on
the inode, so it can release neither the superblock nor the inode:

  wb_workfn
    wb_writeback
      writeback_sb_inodes
        __writeback_single_inode
          nfs_write_inode
            __nfs_commit_inode
              nfs_generic_commit_list
                nfs_commit_list
                  nfs_initiate_commit
                    rpc_put_task
                      rpc_free_task
                        nfs_commit_release
                          nfs_commitdata_release
                            put_nfs_open_context
                              nfs_sb_deactive
                                deactivate_super   <- blocks on s_umount

This is the deadlock that was reported in 2014, see the link below.

Before commit bf294b41cefc ("SUNRPC: Close a race in
__rpc_wait_for_completion_task()") the final rpc_put_task() always
handed the release to the workqueue of the task. That commit made
rpc_put_task() run the release in the caller, and added
rpc_put_task_async() for callers that must not. Use it.

nfs_initiate_pgio() also starts READs, whose release now always runs
on nfsiod as well.

Nothing depends on the release of a COMMIT having run by the time
nfs_initiate_commit() returns: no caller has passed it FLUSH_SYNC since
commit 64a93dbf25d3 ("NFS: Fix deadlocks in nfs_scan_commit_list()").
Before that commit nfs_write_inode() did, from the flusher, and the
wait then made the reference of the submitter the last one every time.

Link: https://lore.kernel.org/linux-nfs/20140407135001.56ef9f36@notabene.brown/
Fixes: bf294b41cefc ("SUNRPC: Close a race in __rpc_wait_for_completion_task()")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/pagelist.c | 3 ++-
 fs/nfs/write.c    | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 71f0ce2bc4ea..e87ad5651f4d 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -816,7 +816,8 @@ int nfs_initiate_pgio(struct rpc_clnt *clnt, struct nfs_pgio_header *hdr,
 	task = rpc_run_task(&task_setup_data);
 	if (IS_ERR(task))
 		return PTR_ERR(task);
-	rpc_put_task(task);
+	/* The caller may be writeback: don't run rpc_release() here */
+	rpc_put_task_async(task);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nfs_initiate_pgio);
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index f38bbebffe4d..766481c5870e 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1677,7 +1677,8 @@ int nfs_initiate_commit(struct rpc_clnt *clnt, struct nfs_commit_data *data,
 		return PTR_ERR(task);
 	if (how & FLUSH_SYNC)
 		rpc_wait_for_completion_task(task);
-	rpc_put_task(task);
+	/* The caller may be writeback: don't run rpc_release() here */
+	rpc_put_task_async(task);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nfs_initiate_commit);
-- 
2.52.0

[PATCH v2 3/3] NFSv4/pnfs: don't run the release of a LAYOUTCOMMIT in the submitter

[Mike Snitzer]

Since commit 472e25944981 ("NFSv4.1: Pin the inode and super block in
asynchronous layoutcommit") nfs4_proc_layoutcommit() pins the inode and
the superblock for an asynchronous LAYOUTCOMMIT with
nfs_igrab_and_active(), and nfs4_layoutcommit_release() drops both with
nfs_iput_and_deactive().

The task is put with rpc_put_task(), so if it has already completed,
the release runs in the submitter. nfs4_write_inode() sends an
asynchronous LAYOUTCOMMIT from writeback, where the submitter is the
flusher. It holds sb->s_umount and has set I_SYNC on the inode, so it
can do neither the final iput() nor the final deactivate_super(). If
the task has not completed, the release runs on rpciod, because the
task has no workqueue.

Give the asynchronous task nfsiod as its workqueue and put it with
rpc_put_task_async(), as is done for WRITE and COMMIT. None of the
callers that send an asynchronous LAYOUTCOMMIT waits for its release.

A synchronous LAYOUTCOMMIT is left as it is: it pins nothing, so its
release has nothing of the kind to drop.

Fixes: 472e25944981 ("NFSv4.1: Pin the inode and super block in asynchronous layoutcommit")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/nfs4proc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 518348e87dd8..133d01ce9f08 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -10147,6 +10147,7 @@ nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data, bool sync)
 			return -EAGAIN;
 		}
 		task_setup_data.flags = RPC_TASK_ASYNC;
+		task_setup_data.workqueue = nfsiod_workqueue;
 	}
 	nfs4_init_sequence(NFS_SERVER(data->args.inode)->nfs_client,
 			   &data->args.seq_args, &data->res.seq_res, 1, 0);
@@ -10157,7 +10158,11 @@ nfs4_proc_layoutcommit(struct nfs4_layoutcommit_data *data, bool sync)
 		status = task->tk_status;
 	trace_nfs4_layoutcommit(data->args.inode, &data->args.stateid, status);
 	dprintk("%s: status %d\n", __func__, status);
-	rpc_put_task(task);
+	/* An asynchronous caller may be writeback: don't run rpc_release() here */
+	if (sync)
+		rpc_put_task(task);
+	else
+		rpc_put_task_async(task);
 	return status;
 }
 
-- 
2.52.0