public inbox for linux-nfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Mike Snitzer <snitzer@kernel.org>
To: linux-nfs@vger.kernel.org
Cc: Anna Schumaker <anna@kernel.org>,
	Trond Myklebust <trondmy@hammerspace.com>,
	Chuck Lever <chuck.lever@oracle.com>,
	Jeff Layton <jlayton@kernel.org>, NeilBrown <neilb@suse.de>
Subject: [for-6.13 PATCH 02/19] nfs_common: must not hold RCU while calling nfsd_file_put_local
Date: Fri,  8 Nov 2024 18:39:45 -0500	[thread overview]
Message-ID: <20241108234002.16392-3-snitzer@kernel.org> (raw)
In-Reply-To: <20241108234002.16392-1-snitzer@kernel.org>

Move holding the RCU from nfs_to_nfsd_file_put_local to
nfs_to_nfsd_net_put.  It is the call to nfs_to->nfsd_serv_put that
requires the RCU anyway (the puts for nfsd_file and netns were
combined to avoid an extra indirect reference but that
micro-optimization isn't possible now).

This fixes xfstests generic/013 and it triggering:

"Voluntary context switch within RCU read-side critical section!"

[  143.545738] Call Trace:
[  143.546206]  <TASK>
[  143.546625]  ? show_regs+0x6d/0x80
[  143.547267]  ? __warn+0x91/0x140
[  143.547951]  ? rcu_note_context_switch+0x496/0x5d0
[  143.548856]  ? report_bug+0x193/0x1a0
[  143.549557]  ? handle_bug+0x63/0xa0
[  143.550214]  ? exc_invalid_op+0x1d/0x80
[  143.550938]  ? asm_exc_invalid_op+0x1f/0x30
[  143.551736]  ? rcu_note_context_switch+0x496/0x5d0
[  143.552634]  ? wakeup_preempt+0x62/0x70
[  143.553358]  __schedule+0xaa/0x1380
[  143.554025]  ? _raw_spin_unlock_irqrestore+0x12/0x40
[  143.554958]  ? try_to_wake_up+0x1fe/0x6b0
[  143.555715]  ? wake_up_process+0x19/0x20
[  143.556452]  schedule+0x2e/0x120
[  143.557066]  schedule_preempt_disabled+0x19/0x30
[  143.557933]  rwsem_down_read_slowpath+0x24d/0x4a0
[  143.558818]  ? xfs_efi_item_format+0x50/0xc0 [xfs]
[  143.559894]  down_read+0x4e/0xb0
[  143.560519]  xlog_cil_commit+0x1b2/0xbc0 [xfs]
[  143.561460]  ? _raw_spin_unlock+0x12/0x30
[  143.562212]  ? xfs_inode_item_precommit+0xc7/0x220 [xfs]
[  143.563309]  ? xfs_trans_run_precommits+0x69/0xd0 [xfs]
[  143.564394]  __xfs_trans_commit+0xb5/0x330 [xfs]
[  143.565367]  xfs_trans_roll+0x48/0xc0 [xfs]
[  143.566262]  xfs_defer_trans_roll+0x57/0x100 [xfs]
[  143.567278]  xfs_defer_finish_noroll+0x27a/0x490 [xfs]
[  143.568342]  xfs_defer_finish+0x1a/0x80 [xfs]
[  143.569267]  xfs_bunmapi_range+0x4d/0xb0 [xfs]
[  143.570208]  xfs_itruncate_extents_flags+0x13d/0x230 [xfs]
[  143.571353]  xfs_free_eofblocks+0x12e/0x190 [xfs]
[  143.572359]  xfs_file_release+0x12d/0x140 [xfs]
[  143.573324]  __fput+0xe8/0x2d0
[  143.573922]  __fput_sync+0x1d/0x30
[  143.574574]  nfsd_filp_close+0x33/0x60 [nfsd]
[  143.575430]  nfsd_file_free+0x96/0x150 [nfsd]
[  143.576274]  nfsd_file_put+0xf7/0x1a0 [nfsd]
[  143.577104]  nfsd_file_put_local+0x18/0x30 [nfsd]
[  143.578070]  nfs_close_local_fh+0x101/0x110 [nfs_localio]
[  143.579079]  __put_nfs_open_context+0xc9/0x180 [nfs]
[  143.580031]  nfs_file_clear_open_context+0x4a/0x60 [nfs]
[  143.581038]  nfs_file_release+0x3e/0x60 [nfs]
[  143.581879]  __fput+0xe8/0x2d0
[  143.582464]  __fput_sync+0x1d/0x30
[  143.583108]  __x64_sys_close+0x41/0x80
[  143.583823]  x64_sys_call+0x189a/0x20d0
[  143.584552]  do_syscall_64+0x64/0x170
[  143.585240]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[  143.586185] RIP: 0033:0x7f3c5153efd7

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs_common/nfslocalio.c |  8 +++-----
 fs/nfsd/filecache.c        | 14 +++++++-------
 fs/nfsd/filecache.h        |  2 +-
 include/linux/nfslocalio.h | 18 +++++++++++++++---
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
index 09404d142d1a..a74ec08f6c96 100644
--- a/fs/nfs_common/nfslocalio.c
+++ b/fs/nfs_common/nfslocalio.c
@@ -155,11 +155,9 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *uuid,
 	/* We have an implied reference to net thanks to nfsd_serv_try_get */
 	localio = nfs_to->nfsd_open_local_fh(net, uuid->dom, rpc_clnt,
 					     cred, nfs_fh, fmode);
-	if (IS_ERR(localio)) {
-		rcu_read_lock();
-		nfs_to->nfsd_serv_put(net);
-		rcu_read_unlock();
-	}
+	if (IS_ERR(localio))
+		nfs_to_nfsd_net_put(net);
+
 	return localio;
 }
 EXPORT_SYMBOL_GPL(nfs_open_local_fh);
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index c16671135d17..9a62b4da89bb 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -391,19 +391,19 @@ nfsd_file_put(struct nfsd_file *nf)
 }
 
 /**
- * nfsd_file_put_local - put the reference to nfsd_file and local nfsd_serv
- * @nf: nfsd_file of which to put the references
+ * nfsd_file_put_local - put nfsd_file reference and arm nfsd_serv_put in caller
+ * @nf: nfsd_file of which to put the reference
  *
- * First put the reference of the nfsd_file and then put the
- * reference to the associated nn->nfsd_serv.
+ * First save the associated net to return to caller, then put
+ * the reference of the nfsd_file.
  */
-void
-nfsd_file_put_local(struct nfsd_file *nf) __must_hold(rcu)
+struct net *
+nfsd_file_put_local(struct nfsd_file *nf)
 {
 	struct net *net = nf->nf_net;
 
 	nfsd_file_put(nf);
-	nfsd_serv_put(net);
+	return net;
 }
 
 /**
diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index cadf3c2689c4..d5db6b34ba30 100644
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h
@@ -55,7 +55,7 @@ void nfsd_file_cache_shutdown(void);
 int nfsd_file_cache_start_net(struct net *net);
 void nfsd_file_cache_shutdown_net(struct net *net);
 void nfsd_file_put(struct nfsd_file *nf);
-void nfsd_file_put_local(struct nfsd_file *nf);
+struct net *nfsd_file_put_local(struct nfsd_file *nf);
 struct nfsd_file *nfsd_file_get(struct nfsd_file *nf);
 struct file *nfsd_file_file(struct nfsd_file *nf);
 void nfsd_file_close_inode_sync(struct inode *inode);
diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
index 3982fea79919..9202f4b24343 100644
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -55,7 +55,7 @@ struct nfsd_localio_operations {
 						const struct cred *,
 						const struct nfs_fh *,
 						const fmode_t);
-	void (*nfsd_file_put_local)(struct nfsd_file *);
+	struct net *(*nfsd_file_put_local)(struct nfsd_file *);
 	struct file *(*nfsd_file_file)(struct nfsd_file *);
 } ____cacheline_aligned;
 
@@ -66,7 +66,7 @@ struct nfsd_file *nfs_open_local_fh(nfs_uuid_t *,
 		   struct rpc_clnt *, const struct cred *,
 		   const struct nfs_fh *, const fmode_t);
 
-static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
+static inline void nfs_to_nfsd_net_put(struct net *net)
 {
 	/*
 	 * Once reference to nfsd_serv is dropped, NFSD could be
@@ -74,10 +74,22 @@ static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
 	 * by always taking RCU.
 	 */
 	rcu_read_lock();
-	nfs_to->nfsd_file_put_local(localio);
+	nfs_to->nfsd_serv_put(net);
 	rcu_read_unlock();
 }
 
+static inline void nfs_to_nfsd_file_put_local(struct nfsd_file *localio)
+{
+	/*
+	 * Must not hold RCU otherwise nfsd_file_put() can easily trigger:
+	 * "Voluntary context switch within RCU read-side critical section!"
+	 * by scheduling deep in underlying filesystem (e.g. XFS).
+	 */
+	struct net *net = nfs_to->nfsd_file_put_local(localio);
+
+	nfs_to_nfsd_net_put(net);
+}
+
 #else   /* CONFIG_NFS_LOCALIO */
 static inline void nfsd_localio_ops_init(void)
 {
-- 
2.44.0


  parent reply	other threads:[~2024-11-08 23:40 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-08 23:39 [for-6.13 PATCH 00/19] nfs/nfsd: fixes and improvements for LOCALIO Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 01/19] nfs/localio: must clear res.replen in nfs_local_read_done Mike Snitzer
2024-11-11  0:36   ` NeilBrown
2024-11-08 23:39 ` Mike Snitzer [this message]
2024-11-11  1:01   ` [for-6.13 PATCH 02/19] nfs_common: must not hold RCU while calling nfsd_file_put_local NeilBrown
2024-11-13 14:58   ` Jeff Layton
2024-11-13 16:51     ` Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 03/19] nfs/localio: remove redundant suid/sgid handling Mike Snitzer
2024-11-11  1:09   ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 04/19] nfs/localio: eliminate unnecessary kref in nfs_local_fsync_ctx Mike Snitzer
2024-11-11  1:15   ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 05/19] nfs/localio: remove extra indirect nfs_to call to check {read,write}_iter Mike Snitzer
2024-11-11  1:20   ` NeilBrown
2024-11-11 15:09     ` Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 06/19] nfs/localio: eliminate need for nfs_local_fsync_work forward declaration Mike Snitzer
2024-11-11  1:21   ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 07/19] nfs/localio: add direct IO enablement with sync and async IO support Mike Snitzer
2024-11-11  1:31   ` NeilBrown
2024-11-12 14:31   ` Chuck Lever
2024-11-08 23:39 ` [for-6.13 PATCH 08/19] nfsd: add nfsd_file_{get,put} to 'nfs_to' nfsd_localio_operations Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 09/19] nfs_common: rename functions that invalidate LOCALIO nfs_clients Mike Snitzer
2024-11-11  1:32   ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 10/19] nfs_common: move localio_lock to new lock member of nfs_uuid_t Mike Snitzer
2024-11-11  1:55   ` NeilBrown
2024-11-11 15:33     ` Mike Snitzer
2024-11-11 20:35       ` NeilBrown
2024-11-11 22:27         ` Mike Snitzer
2024-11-11 23:23           ` NeilBrown
2024-11-12  0:16             ` Mike Snitzer
2024-11-12  0:49               ` NeilBrown
2024-11-12 14:36                 ` Chuck Lever
2024-11-12 23:13                   ` NeilBrown
2024-11-13  0:07                     ` Chuck Lever III
2024-11-13  0:32                       ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 11/19] nfs: cache all open LOCALIO nfsd_file(s) in client Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 12/19] nfsd: update percpu_ref to manage references on nfsd_net Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 13/19] nfsd: rename nfsd_serv_ prefixed methods and variables with nfsd_net_ Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 14/19] nfsd: nfsd_file_acquire_local no longer returns GC'd nfsd_file Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 15/19] nfs_common: rename nfslocalio nfs_uuid_lock to nfs_uuids_lock Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 16/19] nfs_common: track all open nfsd_files per LOCALIO nfs_client Mike Snitzer
2024-11-08 23:40 ` [for-6.13 PATCH 17/19] nfs_common: add nfs_localio trace events Mike Snitzer
2024-11-08 23:40 ` [for-6.13 PATCH 18/19] nfs: probe for LOCALIO when v4 client reconnects to server Mike Snitzer
2024-11-08 23:40 ` [for-6.13 PATCH 19/19] nfs: probe for LOCALIO when v3 " Mike Snitzer
2024-11-11  3:06   ` NeilBrown
2024-11-10 15:49 ` [for-6.13 PATCH 00/19] nfs/nfsd: fixes and improvements for LOCALIO Chuck Lever III

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20241108234002.16392-3-snitzer@kernel.org \
    --to=snitzer@kernel.org \
    --cc=anna@kernel.org \
    --cc=chuck.lever@oracle.com \
    --cc=jlayton@kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=trondmy@hammerspace.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox