[PATCH v2 0/5] ceph: reduce mdsc->mutex contention in the cephfs kclient

[PATCH v2 0/5] ceph: reduce mdsc->mutex contention in the cephfs kclient

[Xiubo Li via B4 Relay]

This series reduces mdsc->mutex hold times from hundreds of
microseconds to tens of microseconds on the hot request-submit and
reply-handling paths.

The approach is incremental:
  1. Convert oldest_tid to atomic64_t so that __prepare_send_request()
     and __send_request() no longer need the mutex.
  2. Replace the request_tree rbtree with an xarray for O(1) lookups
     and internally-locked iteration.
  3. Add a dedicated wait_list_lock spinlock so wait-list operations
     no longer depend on the global mutex.
  4. Move mdsc->mutex acquisition inside __do_request(), then release
     it during the send phase (message construction and path walking),
     leaving only the brief setup/teardown under the lock.
  5. Narrow the mutex scope in replay_unsafe_requests() similarly.

Tested with concurrent readdir + stat on a 5000-file directory
(32 threads).  bpftrace measurements show:

                 before         after
  __do_request   354-2327 us    10-68 us    (34x)
  handle_reply   51-416 us     10-32 us    (13x)
  submit_request 89-211 us     10-41 us    (5x)

Benchmark results from test_i_caps (aggregate cache test):

  | Test                | Before   | After    | Improvement |
  |---------------------|----------|----------|-------------|
  | stat storm ST       | 692k/s   | 1,420k/s | +105% (2.1x)|
  | stat storm MT       | 722k/s   | 1,373k/s | +90%  (1.9x)|
  | open/close          | 403k/s   | 524k/s   | +30%        |
  | stat hot (cache)    | 353k/s   | 540k/s   | +53%        |
  | readdir             | 877/s    | 832/s    | ~0%         |
  | Total time          | 195s     | 121s     | -38%  (1.6x)|

The 2x stat throughput gain comes from __do_request() no longer
holding mdsc->mutex during the __send_request() phase, so dentry
path walking and message encoding in create_request_message()
run outside the lock.

No functional changes intended.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
Changes in v2:
- Add xa_store() error handling and bail out on failure in the submit path
- Guard xarray conversion with BITS_PER_LONG==64, fall back to rbtree on 32-bit
- Fix missing mutex_unlock on early-return path in __do_request()
- Move mutex_unlock before __wake_requests() and kick_requests() calls to
  avoid recursive lock acquisition
- Pin requests with ceph_mdsc_get_request() across lockless __send_request()
  in replay_unsafe_requests() to prevent use-after-free
- Keep mdsc->mutex held on 32-bit for rb_first()/rb_next() iteration in
  replay_unsafe_requests()
- Drop stale "called under mdsc->mutex" comment on __wake_requests()
- Add benchmark results from test_i_caps (2.1x stat throughput improvement)
- Link to v1: https://patch.msgid.link/20260713-ceph-mdsc-mutex-optimization-v1-0-9ae5ac135c34@clyso.com

---
Xiubo Li (5):
      ceph: convert oldest_tid to atomic64_t
      ceph: replace the request_tree rbtree with an xarray keyed by r_tid.
      ceph: add wait_list_lock for wait-list serialization
      ceph: move mdsc->mutex into __do_request()
      ceph: narrow mdsc->mutex scope in replay_unsafe_requests

 fs/ceph/debugfs.c    |   9 ++
 fs/ceph/mds_client.c | 292 +++++++++++++++++++++++++++++++++++++++++++--------
 fs/ceph/mds_client.h |  15 ++-
 3 files changed, 268 insertions(+), 48 deletions(-)
---
base-commit: 7e1f9e2cd2d0e780c394a4402c40e125109fec72
change-id: 20260713-ceph-mdsc-mutex-optimization-7e74ab6bbc8b

Best regards,
--  
Xiubo Li <xiubo.li@clyso.com>

[PATCH v2 1/5] ceph: convert oldest_tid to atomic64_t

[Xiubo Li via B4 Relay]

From: Xiubo Li <xiubo.li@clyso.com>

The oldest_client_tid sent in the MDS request header is advisory:
a stale value is harmless -- at worst the MDS may resend a reply
we already have, or skip one we still need (which will just be
retried).  With the plain u64 read, however, the compiler is free
to split or cache the load, which is undefined behaviour when the
write side runs under mdsc->mutex on a different CPU.

Convert mdsc->oldest_tid from u64 to atomic64_t so that reads
are guaranteed to be single-copy atomic on all architectures.
This removes the one remaining reason __prepare_send_request()
and __send_request() needed to be called under mdsc->mutex, so
drop those comments as well.

All write sites (__register_request, __unregister_request) still
run under mdsc->mutex, so use atomic64_set() for clarity.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/mds_client.c | 21 ++++++++-------------
 fs/ceph/mds_client.h |  2 +-
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 9f84ef2ac6e4..98d0a5baff70 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1235,8 +1235,9 @@ static void __register_request(struct ceph_mds_client *mdsc,
 	if (!req->r_mnt_idmap)
 		req->r_mnt_idmap = &nop_mnt_idmap;
 
-	if (mdsc->oldest_tid == 0 && req->r_op != CEPH_MDS_OP_SETFILELOCK)
-		mdsc->oldest_tid = req->r_tid;
+	if (atomic64_read(&mdsc->oldest_tid) == 0 &&
+	    req->r_op != CEPH_MDS_OP_SETFILELOCK)
+		atomic64_set(&mdsc->oldest_tid, req->r_tid);
 
 	if (dir) {
 		struct ceph_inode_info *ci = ceph_inode(dir);
@@ -1257,14 +1258,14 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
 	/* Never leave an unregistered request on an unsafe list! */
 	list_del_init(&req->r_unsafe_item);
 
-	if (req->r_tid == mdsc->oldest_tid) {
+	if (req->r_tid == atomic64_read(&mdsc->oldest_tid)) {
 		struct rb_node *p = rb_next(&req->r_node);
-		mdsc->oldest_tid = 0;
+		atomic64_set(&mdsc->oldest_tid, 0);
 		while (p) {
 			struct ceph_mds_request *next_req =
 				rb_entry(p, struct ceph_mds_request, r_node);
 			if (next_req->r_op != CEPH_MDS_OP_SETFILELOCK) {
-				mdsc->oldest_tid = next_req->r_tid;
+				atomic64_set(&mdsc->oldest_tid, next_req->r_tid);
 				break;
 			}
 			p = rb_next(p);
@@ -1693,7 +1694,7 @@ create_session_full_msg(struct ceph_mds_client *mdsc, int op, u64 seq)
 	ceph_encode_32(&p, 0);
 
 	/* version == 7, oldest_client_tid */
-	ceph_encode_64(&p, mdsc->oldest_tid);
+	ceph_encode_64(&p, atomic64_read(&mdsc->oldest_tid));
 
 	msg->front.iov_len = p - msg->front.iov_base;
 	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
@@ -2759,7 +2760,7 @@ static struct ceph_mds_request *__get_oldest_req(struct ceph_mds_client *mdsc)
 
 static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
 {
-	return mdsc->oldest_tid;
+	return atomic64_read(&mdsc->oldest_tid);
 }
 
 #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
@@ -3438,9 +3439,6 @@ static void complete_request(struct ceph_mds_client *mdsc,
 	complete_all(&req->r_completion);
 }
 
-/*
- * called under mdsc->mutex
- */
 static int __prepare_send_request(struct ceph_mds_session *session,
 				  struct ceph_mds_request *req,
 				  bool drop_cap_releases)
@@ -3555,9 +3553,6 @@ static int __prepare_send_request(struct ceph_mds_session *session,
 	return 0;
 }
 
-/*
- * called under mdsc->mutex
- */
 static int __send_request(struct ceph_mds_session *session,
 			  struct ceph_mds_request *req,
 			  bool drop_cap_releases)
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 731d6ad04956..3b614b5df18c 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -532,7 +532,7 @@ struct ceph_mds_client {
 	spinlock_t              snap_empty_lock;  /* protect snap_empty */
 
 	u64                    last_tid;      /* most recent mds request */
-	u64                    oldest_tid;    /* oldest incomplete mds request,
+	atomic64_t             oldest_tid;    /* oldest incomplete mds request,
 						 excluding setfilelock requests */
 	struct rb_root         request_tree;  /* pending mds requests */
 	struct delayed_work    delayed_work;  /* delayed work */

-- 
2.53.0

[PATCH v2 2/5] ceph: replace the request_tree rbtree with an xarray keyed by r_tid.

[Xiubo Li via B4 Relay]

From: Xiubo Li <xiubo.li@clyso.com>

The xarray gives O(1) lookups by tid (vs O(log N) rbtree) and
uses internal RCU-based locking, eliminating mdsc->mutex as a
precondition for tree access and paving the way for lockless
xa_load() in the future. In practise this yields a 13x reduction
of handle_reply mutex hold time, from 51-416us down to 10-32us,
due to parse_reply_info() no longer being protected by the global
mutex.

Check xa_store() for allocation failure, which would otherwise be
silent, and skip __do_request() in the submit path if registration
failed.

On 32-bit architectures xarray silently truncates u64 keys because
its index type is unsigned long.  Guard the entire conversion by
BITS_PER_LONG==64 and fall back to the original rbtree on 32-bit
so there is no regression from the current code.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/debugfs.c    |   9 +++
 fs/ceph/mds_client.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++--
 fs/ceph/mds_client.h |  10 +++-
 3 files changed, 173 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 18eb5da03411..f0d0b9b04b79 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -87,12 +87,21 @@ static int mdsc_show(struct seq_file *s, void *p)
 	struct ceph_fs_client *fsc = s->private;
 	struct ceph_mds_client *mdsc = fsc->mdsc;
 	struct ceph_mds_request *req;
+#if BITS_PER_LONG == 64
+	unsigned long idx;
+#else
 	struct rb_node *rp;
+#endif
 	char *path;
 
 	mutex_lock(&mdsc->mutex);
+#if BITS_PER_LONG == 64
+	idx = 0;
+	xa_for_each(&mdsc->request_tree, idx, req) {
+#else
 	for (rp = rb_first(&mdsc->request_tree); rp; rp = rb_next(rp)) {
 		req = rb_entry(rp, struct ceph_mds_request, r_node);
+#endif
 
 		if (req->r_request && req->r_session)
 			seq_printf(s, "%lld\tmds%d\t", req->r_tid,
diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 98d0a5baff70..239ed34886f9 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1183,7 +1183,9 @@ void ceph_mdsc_release_request(struct kref *kref)
 	kmem_cache_free(ceph_mds_request_cachep, req);
 }
 
+#if BITS_PER_LONG != 64
 DEFINE_RB_FUNCS(request, struct ceph_mds_request, r_tid, r_node)
+#endif
 
 /*
  * lookup session, bump ref if found.
@@ -1195,7 +1197,11 @@ lookup_get_request(struct ceph_mds_client *mdsc, u64 tid)
 {
 	struct ceph_mds_request *req;
 
+#if BITS_PER_LONG == 64
+	req = xa_load(&mdsc->request_tree, tid);
+#else
 	req = lookup_request(&mdsc->request_tree, tid);
+#endif
 	if (req)
 		ceph_mdsc_get_request(req);
 
@@ -1229,7 +1235,17 @@ static void __register_request(struct ceph_mds_client *mdsc,
 	}
 	doutc(cl, "%p tid %lld\n", req, req->r_tid);
 	ceph_mdsc_get_request(req);
+#if BITS_PER_LONG == 64
+	if (xa_is_err(xa_store(&mdsc->request_tree, req->r_tid, req,
+			       GFP_NOFS))) {
+		pr_err_client(cl, "%p tid %lld: xa_store failed\n",
+			      req, req->r_tid);
+		req->r_err = -ENOMEM;
+		return;
+	}
+#else
 	insert_request(&mdsc->request_tree, req);
+#endif
 
 	req->r_cred = get_current_cred();
 	if (!req->r_mnt_idmap)
@@ -1259,6 +1275,19 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
 	list_del_init(&req->r_unsafe_item);
 
 	if (req->r_tid == atomic64_read(&mdsc->oldest_tid)) {
+#if BITS_PER_LONG == 64
+		unsigned long tidx = req->r_tid + 1;
+		struct ceph_mds_request *next_req;
+
+		atomic64_set(&mdsc->oldest_tid, 0);
+		xa_for_each_start(&mdsc->request_tree, tidx, next_req, tidx) {
+			if (next_req->r_op != CEPH_MDS_OP_SETFILELOCK) {
+				atomic64_set(&mdsc->oldest_tid,
+					     next_req->r_tid);
+				break;
+			}
+		}
+#else
 		struct rb_node *p = rb_next(&req->r_node);
 		atomic64_set(&mdsc->oldest_tid, 0);
 		while (p) {
@@ -1270,9 +1299,14 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
 			}
 			p = rb_next(p);
 		}
+#endif
 	}
 
+#if BITS_PER_LONG == 64
+	xa_erase(&mdsc->request_tree, req->r_tid);
+#else
 	erase_request(&mdsc->request_tree, req);
+#endif
 
 	if (req->r_unsafe_dir) {
 		struct ceph_inode_info *ci = ceph_inode(req->r_unsafe_dir);
@@ -1829,7 +1863,11 @@ static void cleanup_session_requests(struct ceph_mds_client *mdsc,
 {
 	struct ceph_client *cl = mdsc->fsc->client;
 	struct ceph_mds_request *req;
+#if BITS_PER_LONG == 64
+	unsigned long idx;
+#else
 	struct rb_node *p;
+#endif
 
 	doutc(cl, "mds%d\n", session->s_mds);
 	mutex_lock(&mdsc->mutex);
@@ -1845,6 +1883,14 @@ static void cleanup_session_requests(struct ceph_mds_client *mdsc,
 		__unregister_request(mdsc, req);
 	}
 	/* zero r_attempts, so kick_requests() will re-send requests */
+#if BITS_PER_LONG == 64
+	idx = 0;
+	xa_for_each(&mdsc->request_tree, idx, req) {
+		if (req->r_session &&
+		    req->r_session->s_mds == session->s_mds)
+			req->r_attempts = 0;
+	}
+#else
 	p = rb_first(&mdsc->request_tree);
 	while (p) {
 		req = rb_entry(p, struct ceph_mds_request, r_node);
@@ -1853,6 +1899,7 @@ static void cleanup_session_requests(struct ceph_mds_client *mdsc,
 		    req->r_session->s_mds == session->s_mds)
 			req->r_attempts = 0;
 	}
+#endif
 	mutex_unlock(&mdsc->mutex);
 }
 
@@ -2732,7 +2779,9 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)
 	req->r_fmode = -1;
 	req->r_feature_needed = -1;
 	kref_init(&req->r_kref);
+#if BITS_PER_LONG != 64
 	RB_CLEAR_NODE(&req->r_node);
+#endif
 	INIT_LIST_HEAD(&req->r_wait);
 	init_completion(&req->r_completion);
 	init_completion(&req->r_safe_completion);
@@ -2752,10 +2801,16 @@ ceph_mdsc_create_request(struct ceph_mds_client *mdsc, int op, int mode)
  */
 static struct ceph_mds_request *__get_oldest_req(struct ceph_mds_client *mdsc)
 {
+#if BITS_PER_LONG == 64
+	unsigned long idx = 0;
+
+	return xa_find(&mdsc->request_tree, &idx, ULONG_MAX, XA_PRESENT);
+#else
 	if (RB_EMPTY_ROOT(&mdsc->request_tree))
 		return NULL;
 	return rb_entry(rb_first(&mdsc->request_tree),
 			struct ceph_mds_request, r_node);
+#endif
 }
 
 static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
@@ -3816,12 +3871,20 @@ static void kick_requests(struct ceph_mds_client *mdsc, int mds)
 {
 	struct ceph_client *cl = mdsc->fsc->client;
 	struct ceph_mds_request *req;
-	struct rb_node *p = rb_first(&mdsc->request_tree);
+#if BITS_PER_LONG == 64
+	unsigned long idx;
+#else
+	struct rb_node *p;
+#endif
 
 	doutc(cl, "kick_requests mds%d\n", mds);
-	while (p) {
+#if BITS_PER_LONG == 64
+	idx = 0;
+	xa_for_each(&mdsc->request_tree, idx, req) {
+#else
+	for (p = rb_first(&mdsc->request_tree); p; p = rb_next(p)) {
 		req = rb_entry(p, struct ceph_mds_request, r_node);
-		p = rb_next(p);
+#endif
 		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags))
 			continue;
 		if (req->r_attempts > 0)
@@ -3894,7 +3957,8 @@ int ceph_mdsc_submit_request(struct ceph_mds_client *mdsc, struct inode *dir,
 	mutex_lock(&mdsc->mutex);
 	__register_request(mdsc, req, dir);
 	trace_ceph_mdsc_submit_request(mdsc, req);
-	__do_request(mdsc, req);
+	if (!req->r_err)
+		__do_request(mdsc, req);
 	err = req->r_err;
 	mutex_unlock(&mdsc->mutex);
 	return err;
@@ -4654,7 +4718,11 @@ static void replay_unsafe_requests(struct ceph_mds_client *mdsc,
 				   struct ceph_mds_session *session)
 {
 	struct ceph_mds_request *req, *nreq;
+#if BITS_PER_LONG == 64
+	unsigned long idx;
+#else
 	struct rb_node *p;
+#endif
 
 	doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
 
@@ -4666,10 +4734,15 @@ static void replay_unsafe_requests(struct ceph_mds_client *mdsc,
 	 * also re-send old requests when MDS enters reconnect stage. So that MDS
 	 * can process completed request in clientreplay stage.
 	 */
+#if BITS_PER_LONG == 64
+	idx = 0;
+	xa_for_each(&mdsc->request_tree, idx, req) {
+#else
 	p = rb_first(&mdsc->request_tree);
 	while (p) {
 		req = rb_entry(p, struct ceph_mds_request, r_node);
 		p = rb_next(p);
+#endif
 		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags))
 			continue;
 		if (req->r_attempts == 0)
@@ -5514,7 +5587,11 @@ static void ceph_mdsc_reset_workfn(struct work_struct *work)
 	 */
 	{
 		struct ceph_mds_request *req;
+#if BITS_PER_LONG == 64
+		unsigned long idx;
+#else
 		struct rb_node *rn;
+#endif
 		u64 last_tid;
 
 		mutex_lock(&mdsc->mutex);
@@ -5522,14 +5599,24 @@ static void ceph_mdsc_reset_workfn(struct work_struct *work)
 		mutex_unlock(&mdsc->mutex);
 
 		mutex_lock(&mdsc->mutex);
+#if BITS_PER_LONG == 64
+		idx = 0;
+		while ((req = xa_find(&mdsc->request_tree, &idx, last_tid,
+				      XA_PRESENT))) {
+#else
 		rn = rb_first(&mdsc->request_tree);
 		while (rn) {
 			req = rb_entry(rn, struct ceph_mds_request, r_node);
 			if (req->r_tid > last_tid)
 				break;
+#endif
 			if (req->r_op == CEPH_MDS_OP_SETFILELOCK ||
 			    !(req->r_op & CEPH_MDS_OP_WRITE)) {
+#if BITS_PER_LONG == 64
+				idx++;
+#else
 				rn = rb_next(rn);
+#endif
 				continue;
 			}
 			ceph_mdsc_get_request(req);
@@ -5542,7 +5629,11 @@ static void ceph_mdsc_reset_workfn(struct work_struct *work)
 			ceph_mdsc_put_request(req);
 			if (time_after(jiffies, drain_deadline))
 				break;
+#if BITS_PER_LONG == 64
+			idx = 0;  /* restart: tree may have changed */
+#else
 			rn = rb_first(&mdsc->request_tree);
+#endif
 		}
 		mutex_unlock(&mdsc->mutex);
 
@@ -6268,7 +6359,11 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
 	mdsc->snap_realms = RB_ROOT;
 	INIT_LIST_HEAD(&mdsc->snap_empty);
 	spin_lock_init(&mdsc->snap_empty_lock);
+#if BITS_PER_LONG == 64
+	xa_init(&mdsc->request_tree);
+#else
 	mdsc->request_tree = RB_ROOT;
+#endif
 	INIT_DELAYED_WORK(&mdsc->delayed_work, delayed_work);
 	mdsc->last_renew_caps = jiffies;
 	INIT_LIST_HEAD(&mdsc->cap_delay_list);
@@ -6590,8 +6685,59 @@ static void flush_mdlog_and_wait_mdsc_unsafe_requests(struct ceph_mds_client *md
 						 u64 want_tid)
 {
 	struct ceph_client *cl = mdsc->fsc->client;
-	struct ceph_mds_request *req = NULL, *nextreq;
 	struct ceph_mds_session *last_session = NULL;
+#if BITS_PER_LONG == 64
+	struct ceph_mds_request *req;
+	unsigned long idx;
+
+	mutex_lock(&mdsc->mutex);
+	doutc(cl, "want %lld\n", want_tid);
+	idx = 0;
+	while ((req = xa_find(&mdsc->request_tree, &idx, want_tid,
+			      XA_PRESENT))) {
+		u64 next_tid = req->r_tid + 1;
+
+		if (req->r_op == CEPH_MDS_OP_SETFILELOCK ||
+		    !(req->r_op & CEPH_MDS_OP_WRITE)) {
+			idx = next_tid;
+			continue;
+		}
+
+		{
+			struct ceph_mds_session *s = req->r_session;
+
+			if (!s) {
+				idx = next_tid;
+				continue;
+			}
+
+			/* write op */
+			ceph_mdsc_get_request(req);
+			s = ceph_get_mds_session(s);
+			mutex_unlock(&mdsc->mutex);
+
+			/* send flush mdlog request to MDS */
+			if (last_session != s) {
+				send_flush_mdlog(s);
+				ceph_put_mds_session(last_session);
+				last_session = s;
+			} else {
+				ceph_put_mds_session(s);
+			}
+			doutc(cl, "wait on %llu (want %llu)\n",
+			      req->r_tid, want_tid);
+			wait_for_completion(&req->r_safe_completion);
+
+			mutex_lock(&mdsc->mutex);
+			ceph_mdsc_put_request(req);
+			idx = next_tid;
+		}
+	}
+	mutex_unlock(&mdsc->mutex);
+	ceph_put_mds_session(last_session);
+	doutc(cl, "done\n");
+#else /* BITS_PER_LONG != 64 — rbtree fallback */
+	struct ceph_mds_request *req = NULL, *nextreq;
 	struct rb_node *n;
 
 	mutex_lock(&mdsc->mutex);
@@ -6649,6 +6795,7 @@ static void flush_mdlog_and_wait_mdsc_unsafe_requests(struct ceph_mds_client *md
 	mutex_unlock(&mdsc->mutex);
 	ceph_put_mds_session(last_session);
 	doutc(cl, "done\n");
+#endif
 }
 
 void ceph_mdsc_sync(struct ceph_mds_client *mdsc)
@@ -6803,6 +6950,9 @@ static void ceph_mdsc_stop(struct ceph_mds_client *mdsc)
 	if (mdsc->mdsmap)
 		ceph_mdsmap_destroy(mdsc->mdsmap);
 	kfree(mdsc->sessions);
+#if BITS_PER_LONG == 64
+	xa_destroy(&mdsc->request_tree);
+#endif
 	ceph_caps_finalize(mdsc);
 
 	if (mdsc->s_cap_auths) {
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index 3b614b5df18c..c2de551607e3 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -329,7 +329,11 @@ typedef int (*ceph_mds_request_wait_callback_t) (struct ceph_mds_client *mdsc,
  */
 struct ceph_mds_request {
 	u64 r_tid;                   /* transaction id */
+#if BITS_PER_LONG == 64
+	/* embedded in mdsc->request_tree xarray */
+#else
 	struct rb_node r_node;
+#endif
 	struct ceph_mds_client *r_mdsc;
 
 	struct kref       r_kref;
@@ -534,7 +538,11 @@ struct ceph_mds_client {
 	u64                    last_tid;      /* most recent mds request */
 	atomic64_t             oldest_tid;    /* oldest incomplete mds request,
 						 excluding setfilelock requests */
-	struct rb_root         request_tree;  /* pending mds requests */
+#if BITS_PER_LONG == 64
+	struct xarray           request_tree;  /* pending mds requests */
+#else
+	struct rb_root          request_tree;  /* pending mds requests */
+#endif
 	struct delayed_work    delayed_work;  /* delayed work */
 	unsigned long    last_renew_caps;  /* last time we renewed our caps */
 	struct list_head cap_delay_list;   /* caps with delayed release */

-- 
2.53.0

Re: [PATCH v2 2/5] ceph: replace the request_tree rbtree with an xarray keyed by r_tid.

[Viacheslav Dubeyko]

On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@clyso.com>
> 
> The xarray gives O(1) lookups by tid (vs O(log N) rbtree) and
> uses internal RCU-based locking, eliminating mdsc->mutex as a
> precondition for tree access and paving the way for lockless
> xa_load() in the future. In practise this yields a 13x reduction
> of handle_reply mutex hold time, from 51-416us down to 10-32us,
> due to parse_reply_info() no longer being protected by the global
> mutex.
> 
> Check xa_store() for allocation failure, which would otherwise be
> silent, and skip __do_request() in the submit path if registration
> failed.
> 
> On 32-bit architectures xarray silently truncates u64 keys because
> its index type is unsigned long.  Guard the entire conversion by
> BITS_PER_LONG==64 and fall back to the original rbtree on 32-bit
> so there is no regression from the current code.
> 
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  fs/ceph/debugfs.c    |   9 +++
>  fs/ceph/mds_client.c | 160
> +++++++++++++++++++++++++++++++++++++++++++++++++--
>  fs/ceph/mds_client.h |  10 +++-
>  3 files changed, 173 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 18eb5da03411..f0d0b9b04b79 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -87,12 +87,21 @@ static int mdsc_show(struct seq_file *s, void *p)
>  	struct ceph_fs_client *fsc = s->private;
>  	struct ceph_mds_client *mdsc = fsc->mdsc;
>  	struct ceph_mds_request *req;
> +#if BITS_PER_LONG == 64
> +	unsigned long idx;
> +#else
>  	struct rb_node *rp;
> +#endif
>  	char *path;
>  
>  	mutex_lock(&mdsc->mutex);
> +#if BITS_PER_LONG == 64
> +	idx = 0;
> +	xa_for_each(&mdsc->request_tree, idx, req) {
> +#else
>  	for (rp = rb_first(&mdsc->request_tree); rp; rp =
> rb_next(rp)) {
>  		req = rb_entry(rp, struct ceph_mds_request, r_node);
> +#endif
>  
>  		if (req->r_request && req->r_session)
>  			seq_printf(s, "%lld\tmds%d\t", req->r_tid,
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 98d0a5baff70..239ed34886f9 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1183,7 +1183,9 @@ void ceph_mdsc_release_request(struct kref
> *kref)
>  	kmem_cache_free(ceph_mds_request_cachep, req);
>  }
>  
> +#if BITS_PER_LONG != 64
>  DEFINE_RB_FUNCS(request, struct ceph_mds_request, r_tid, r_node)
> +#endif
>  
>  /*
>   * lookup session, bump ref if found.
> @@ -1195,7 +1197,11 @@ lookup_get_request(struct ceph_mds_client
> *mdsc, u64 tid)
>  {
>  	struct ceph_mds_request *req;
>  
> +#if BITS_PER_LONG == 64
> +	req = xa_load(&mdsc->request_tree, tid);
> +#else
>  	req = lookup_request(&mdsc->request_tree, tid);
> +#endif
>  	if (req)
>  		ceph_mdsc_get_request(req);
>  
> @@ -1229,7 +1235,17 @@ static void __register_request(struct
> ceph_mds_client *mdsc,
>  	}
>  	doutc(cl, "%p tid %lld\n", req, req->r_tid);
>  	ceph_mdsc_get_request(req);

It looks like we get request before failure check. And the failure path
returns without undoing it. I believe we have issue here.

Thanks,
Slava.

> +#if BITS_PER_LONG == 64
> +	if (xa_is_err(xa_store(&mdsc->request_tree, req->r_tid, req,
> +			       GFP_NOFS))) {
> +		pr_err_client(cl, "%p tid %lld: xa_store failed\n",
> +			      req, req->r_tid);
> +		req->r_err = -ENOMEM;
> +		return;
> +	}
> +#else
>  	insert_request(&mdsc->request_tree, req);
> +#endif
>  
>  	req->r_cred = get_current_cred();
>  	if (!req->r_mnt_idmap)
> @@ -1259,6 +1275,19 @@ static void __unregister_request(struct
> ceph_mds_client *mdsc,
>  	list_del_init(&req->r_unsafe_item);
>  
>  	if (req->r_tid == atomic64_read(&mdsc->oldest_tid)) {
> +#if BITS_PER_LONG == 64
> +		unsigned long tidx = req->r_tid + 1;
> +		struct ceph_mds_request *next_req;
> +
> +		atomic64_set(&mdsc->oldest_tid, 0);
> +		xa_for_each_start(&mdsc->request_tree, tidx,
> next_req, tidx) {
> +			if (next_req->r_op !=
> CEPH_MDS_OP_SETFILELOCK) {
> +				atomic64_set(&mdsc->oldest_tid,
> +					     next_req->r_tid);
> +				break;
> +			}
> +		}
> +#else
>  		struct rb_node *p = rb_next(&req->r_node);
>  		atomic64_set(&mdsc->oldest_tid, 0);
>  		while (p) {
> @@ -1270,9 +1299,14 @@ static void __unregister_request(struct
> ceph_mds_client *mdsc,
>  			}
>  			p = rb_next(p);
>  		}
> +#endif
>  	}
>  
> +#if BITS_PER_LONG == 64
> +	xa_erase(&mdsc->request_tree, req->r_tid);
> +#else
>  	erase_request(&mdsc->request_tree, req);
> +#endif
>  
>  	if (req->r_unsafe_dir) {
>  		struct ceph_inode_info *ci = ceph_inode(req-
> >r_unsafe_dir);
> @@ -1829,7 +1863,11 @@ static void cleanup_session_requests(struct
> ceph_mds_client *mdsc,
>  {
>  	struct ceph_client *cl = mdsc->fsc->client;
>  	struct ceph_mds_request *req;
> +#if BITS_PER_LONG == 64
> +	unsigned long idx;
> +#else
>  	struct rb_node *p;
> +#endif
>  
>  	doutc(cl, "mds%d\n", session->s_mds);
>  	mutex_lock(&mdsc->mutex);
> @@ -1845,6 +1883,14 @@ static void cleanup_session_requests(struct
> ceph_mds_client *mdsc,
>  		__unregister_request(mdsc, req);
>  	}
>  	/* zero r_attempts, so kick_requests() will re-send requests
> */
> +#if BITS_PER_LONG == 64
> +	idx = 0;
> +	xa_for_each(&mdsc->request_tree, idx, req) {
> +		if (req->r_session &&
> +		    req->r_session->s_mds == session->s_mds)
> +			req->r_attempts = 0;
> +	}
> +#else
>  	p = rb_first(&mdsc->request_tree);
>  	while (p) {
>  		req = rb_entry(p, struct ceph_mds_request, r_node);
> @@ -1853,6 +1899,7 @@ static void cleanup_session_requests(struct
> ceph_mds_client *mdsc,
>  		    req->r_session->s_mds == session->s_mds)
>  			req->r_attempts = 0;
>  	}
> +#endif
>  	mutex_unlock(&mdsc->mutex);
>  }
>  
> @@ -2732,7 +2779,9 @@ ceph_mdsc_create_request(struct ceph_mds_client
> *mdsc, int op, int mode)
>  	req->r_fmode = -1;
>  	req->r_feature_needed = -1;
>  	kref_init(&req->r_kref);
> +#if BITS_PER_LONG != 64
>  	RB_CLEAR_NODE(&req->r_node);
> +#endif
>  	INIT_LIST_HEAD(&req->r_wait);
>  	init_completion(&req->r_completion);
>  	init_completion(&req->r_safe_completion);
> @@ -2752,10 +2801,16 @@ ceph_mdsc_create_request(struct
> ceph_mds_client *mdsc, int op, int mode)
>   */
>  static struct ceph_mds_request *__get_oldest_req(struct
> ceph_mds_client *mdsc)
>  {
> +#if BITS_PER_LONG == 64
> +	unsigned long idx = 0;
> +
> +	return xa_find(&mdsc->request_tree, &idx, ULONG_MAX,
> XA_PRESENT);
> +#else
>  	if (RB_EMPTY_ROOT(&mdsc->request_tree))
>  		return NULL;
>  	return rb_entry(rb_first(&mdsc->request_tree),
>  			struct ceph_mds_request, r_node);
> +#endif
>  }
>  
>  static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
> @@ -3816,12 +3871,20 @@ static void kick_requests(struct
> ceph_mds_client *mdsc, int mds)
>  {
>  	struct ceph_client *cl = mdsc->fsc->client;
>  	struct ceph_mds_request *req;
> -	struct rb_node *p = rb_first(&mdsc->request_tree);
> +#if BITS_PER_LONG == 64
> +	unsigned long idx;
> +#else
> +	struct rb_node *p;
> +#endif
>  
>  	doutc(cl, "kick_requests mds%d\n", mds);
> -	while (p) {
> +#if BITS_PER_LONG == 64
> +	idx = 0;
> +	xa_for_each(&mdsc->request_tree, idx, req) {
> +#else
> +	for (p = rb_first(&mdsc->request_tree); p; p = rb_next(p)) {
>  		req = rb_entry(p, struct ceph_mds_request, r_node);
> -		p = rb_next(p);
> +#endif
>  		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> >r_req_flags))
>  			continue;
>  		if (req->r_attempts > 0)
> @@ -3894,7 +3957,8 @@ int ceph_mdsc_submit_request(struct
> ceph_mds_client *mdsc, struct inode *dir,
>  	mutex_lock(&mdsc->mutex);
>  	__register_request(mdsc, req, dir);
>  	trace_ceph_mdsc_submit_request(mdsc, req);
> -	__do_request(mdsc, req);
> +	if (!req->r_err)
> +		__do_request(mdsc, req);
>  	err = req->r_err;
>  	mutex_unlock(&mdsc->mutex);
>  	return err;
> @@ -4654,7 +4718,11 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  				   struct ceph_mds_session *session)
>  {
>  	struct ceph_mds_request *req, *nreq;
> +#if BITS_PER_LONG == 64
> +	unsigned long idx;
> +#else
>  	struct rb_node *p;
> +#endif
>  
>  	doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
>  
> @@ -4666,10 +4734,15 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  	 * also re-send old requests when MDS enters reconnect
> stage. So that MDS
>  	 * can process completed request in clientreplay stage.
>  	 */
> +#if BITS_PER_LONG == 64
> +	idx = 0;
> +	xa_for_each(&mdsc->request_tree, idx, req) {
> +#else
>  	p = rb_first(&mdsc->request_tree);
>  	while (p) {
>  		req = rb_entry(p, struct ceph_mds_request, r_node);
>  		p = rb_next(p);
> +#endif
>  		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> >r_req_flags))
>  			continue;
>  		if (req->r_attempts == 0)
> @@ -5514,7 +5587,11 @@ static void ceph_mdsc_reset_workfn(struct
> work_struct *work)
>  	 */
>  	{
>  		struct ceph_mds_request *req;
> +#if BITS_PER_LONG == 64
> +		unsigned long idx;
> +#else
>  		struct rb_node *rn;
> +#endif
>  		u64 last_tid;
>  
>  		mutex_lock(&mdsc->mutex);
> @@ -5522,14 +5599,24 @@ static void ceph_mdsc_reset_workfn(struct
> work_struct *work)
>  		mutex_unlock(&mdsc->mutex);
>  
>  		mutex_lock(&mdsc->mutex);
> +#if BITS_PER_LONG == 64
> +		idx = 0;
> +		while ((req = xa_find(&mdsc->request_tree, &idx,
> last_tid,
> +				      XA_PRESENT))) {
> +#else
>  		rn = rb_first(&mdsc->request_tree);
>  		while (rn) {
>  			req = rb_entry(rn, struct ceph_mds_request,
> r_node);
>  			if (req->r_tid > last_tid)
>  				break;
> +#endif
>  			if (req->r_op == CEPH_MDS_OP_SETFILELOCK ||
>  			    !(req->r_op & CEPH_MDS_OP_WRITE)) {
> +#if BITS_PER_LONG == 64
> +				idx++;
> +#else
>  				rn = rb_next(rn);
> +#endif
>  				continue;
>  			}
>  			ceph_mdsc_get_request(req);
> @@ -5542,7 +5629,11 @@ static void ceph_mdsc_reset_workfn(struct
> work_struct *work)
>  			ceph_mdsc_put_request(req);
>  			if (time_after(jiffies, drain_deadline))
>  				break;
> +#if BITS_PER_LONG == 64
> +			idx = 0;  /* restart: tree may have changed
> */
> +#else
>  			rn = rb_first(&mdsc->request_tree);
> +#endif
>  		}
>  		mutex_unlock(&mdsc->mutex);
>  
> @@ -6268,7 +6359,11 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
>  	mdsc->snap_realms = RB_ROOT;
>  	INIT_LIST_HEAD(&mdsc->snap_empty);
>  	spin_lock_init(&mdsc->snap_empty_lock);
> +#if BITS_PER_LONG == 64
> +	xa_init(&mdsc->request_tree);
> +#else
>  	mdsc->request_tree = RB_ROOT;
> +#endif
>  	INIT_DELAYED_WORK(&mdsc->delayed_work, delayed_work);
>  	mdsc->last_renew_caps = jiffies;
>  	INIT_LIST_HEAD(&mdsc->cap_delay_list);
> @@ -6590,8 +6685,59 @@ static void
> flush_mdlog_and_wait_mdsc_unsafe_requests(struct ceph_mds_client *md
>  						 u64 want_tid)
>  {
>  	struct ceph_client *cl = mdsc->fsc->client;
> -	struct ceph_mds_request *req = NULL, *nextreq;
>  	struct ceph_mds_session *last_session = NULL;
> +#if BITS_PER_LONG == 64
> +	struct ceph_mds_request *req;
> +	unsigned long idx;
> +
> +	mutex_lock(&mdsc->mutex);
> +	doutc(cl, "want %lld\n", want_tid);
> +	idx = 0;
> +	while ((req = xa_find(&mdsc->request_tree, &idx, want_tid,
> +			      XA_PRESENT))) {
> +		u64 next_tid = req->r_tid + 1;
> +
> +		if (req->r_op == CEPH_MDS_OP_SETFILELOCK ||
> +		    !(req->r_op & CEPH_MDS_OP_WRITE)) {
> +			idx = next_tid;
> +			continue;
> +		}
> +
> +		{
> +			struct ceph_mds_session *s = req->r_session;
> +
> +			if (!s) {
> +				idx = next_tid;
> +				continue;
> +			}
> +
> +			/* write op */
> +			ceph_mdsc_get_request(req);
> +			s = ceph_get_mds_session(s);
> +			mutex_unlock(&mdsc->mutex);
> +
> +			/* send flush mdlog request to MDS */
> +			if (last_session != s) {
> +				send_flush_mdlog(s);
> +				ceph_put_mds_session(last_session);
> +				last_session = s;
> +			} else {
> +				ceph_put_mds_session(s);
> +			}
> +			doutc(cl, "wait on %llu (want %llu)\n",
> +			      req->r_tid, want_tid);
> +			wait_for_completion(&req-
> >r_safe_completion);
> +
> +			mutex_lock(&mdsc->mutex);
> +			ceph_mdsc_put_request(req);
> +			idx = next_tid;
> +		}
> +	}
> +	mutex_unlock(&mdsc->mutex);
> +	ceph_put_mds_session(last_session);
> +	doutc(cl, "done\n");
> +#else /* BITS_PER_LONG != 64 — rbtree fallback */
> +	struct ceph_mds_request *req = NULL, *nextreq;
>  	struct rb_node *n;
>  
>  	mutex_lock(&mdsc->mutex);
> @@ -6649,6 +6795,7 @@ static void
> flush_mdlog_and_wait_mdsc_unsafe_requests(struct ceph_mds_client *md
>  	mutex_unlock(&mdsc->mutex);
>  	ceph_put_mds_session(last_session);
>  	doutc(cl, "done\n");
> +#endif
>  }
>  
>  void ceph_mdsc_sync(struct ceph_mds_client *mdsc)
> @@ -6803,6 +6950,9 @@ static void ceph_mdsc_stop(struct
> ceph_mds_client *mdsc)
>  	if (mdsc->mdsmap)
>  		ceph_mdsmap_destroy(mdsc->mdsmap);
>  	kfree(mdsc->sessions);
> +#if BITS_PER_LONG == 64
> +	xa_destroy(&mdsc->request_tree);
> +#endif
>  	ceph_caps_finalize(mdsc);
>  
>  	if (mdsc->s_cap_auths) {
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index 3b614b5df18c..c2de551607e3 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -329,7 +329,11 @@ typedef int (*ceph_mds_request_wait_callback_t)
> (struct ceph_mds_client *mdsc,
>   */
>  struct ceph_mds_request {
>  	u64 r_tid;                   /* transaction id */
> +#if BITS_PER_LONG == 64
> +	/* embedded in mdsc->request_tree xarray */
> +#else
>  	struct rb_node r_node;
> +#endif
>  	struct ceph_mds_client *r_mdsc;
>  
>  	struct kref       r_kref;
> @@ -534,7 +538,11 @@ struct ceph_mds_client {
>  	u64                    last_tid;      /* most recent mds
> request */
>  	atomic64_t             oldest_tid;    /* oldest incomplete
> mds request,
>  						 excluding
> setfilelock requests */
> -	struct rb_root         request_tree;  /* pending mds
> requests */
> +#if BITS_PER_LONG == 64
> +	struct xarray           request_tree;  /* pending mds
> requests */
> +#else
> +	struct rb_root          request_tree;  /* pending mds
> requests */
> +#endif
>  	struct delayed_work    delayed_work;  /* delayed work */
>  	unsigned long    last_renew_caps;  /* last time we renewed
> our caps */
>  	struct list_head cap_delay_list;   /* caps with delayed
> release */

Re: [PATCH v2 2/5] ceph: replace the request_tree rbtree with an xarray keyed by r_tid.

[Xiubo Li]

On Thu, 16 Jul 2026 at 02:25, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> > From: Xiubo Li <xiubo.li@clyso.com>
> >
> > The xarray gives O(1) lookups by tid (vs O(log N) rbtree) and
> > uses internal RCU-based locking, eliminating mdsc->mutex as a
> > precondition for tree access and paving the way for lockless
> > xa_load() in the future. In practise this yields a 13x reduction
> > of handle_reply mutex hold time, from 51-416us down to 10-32us,
> > due to parse_reply_info() no longer being protected by the global
> > mutex.
> >
> > Check xa_store() for allocation failure, which would otherwise be
> > silent, and skip __do_request() in the submit path if registration
> > failed.
> >
> > On 32-bit architectures xarray silently truncates u64 keys because
> > its index type is unsigned long.  Guard the entire conversion by
> > BITS_PER_LONG==64 and fall back to the original rbtree on 32-bit
> > so there is no regression from the current code.
[......]
> > @@ -1195,7 +1197,11 @@ lookup_get_request(struct ceph_mds_client
> > *mdsc, u64 tid)
> >  {
> >       struct ceph_mds_request *req;
> >
> > +#if BITS_PER_LONG == 64
> > +     req = xa_load(&mdsc->request_tree, tid);
> > +#else
> >       req = lookup_request(&mdsc->request_tree, tid);
> > +#endif
> >       if (req)
> >               ceph_mdsc_get_request(req);
> >
> > @@ -1229,7 +1235,17 @@ static void __register_request(struct
> > ceph_mds_client *mdsc,
> >       }
> >       doutc(cl, "%p tid %lld\n", req, req->r_tid);
> >       ceph_mdsc_get_request(req);
>
> It looks like we get request before failure check. And the failure path
> returns without undoing it. I believe we have issue here.
>
Good catch. Let me fix it.

Thanks
Xiubo

> Thanks,
> Slava.
[......]

[PATCH v2 3/5] ceph: add wait_list_lock for wait-list serialization

[Xiubo Li via B4 Relay]

From: Xiubo Li <xiubo.li@clyso.com>

Add a dedicated spinlock to protect the waiting_for_map and
s_waiting lists, which are currently serialized by mdsc->mutex.
This is a preparatory step for removing mdsc->mutex from the
kick_requests() path: xa_for_each() is internally locked and the
per-request filter checks are lockless, so only list_del_init()
needs external protection.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/mds_client.c | 18 +++++++++++++++++-
 fs/ceph/mds_client.h |  3 +++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 239ed34886f9..a250938f32d3 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3669,7 +3669,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
 			doutc(cl, "no mdsmap, waiting for map\n");
 			trace_ceph_mdsc_suspend_request(mdsc, session, req,
 							ceph_mdsc_suspend_reason_no_mdsmap);
+			spin_lock(&mdsc->wait_list_lock);
 			list_add(&req->r_wait, &mdsc->waiting_for_map);
+			spin_unlock(&mdsc->wait_list_lock);
 			return;
 		}
 		if (!(mdsc->fsc->mount_options->flags &
@@ -3692,7 +3694,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
 		doutc(cl, "no mds or not active, waiting for map\n");
 		trace_ceph_mdsc_suspend_request(mdsc, session, req,
 						ceph_mdsc_suspend_reason_no_active_mds);
+		spin_lock(&mdsc->wait_list_lock);
 		list_add(&req->r_wait, &mdsc->waiting_for_map);
+		spin_unlock(&mdsc->wait_list_lock);
 		return;
 	}
 
@@ -3740,9 +3744,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
 			if (ceph_test_mount_opt(mdsc->fsc, CLEANRECOVER)) {
 				trace_ceph_mdsc_suspend_request(mdsc, session, req,
 								ceph_mdsc_suspend_reason_rejected);
+				spin_lock(&mdsc->wait_list_lock);
 				list_add(&req->r_wait, &mdsc->waiting_for_map);
-			} else
+				spin_unlock(&mdsc->wait_list_lock);
+			} else {
 				err = -EACCES;
+			}
 			goto out_session;
 		}
 
@@ -3757,7 +3764,9 @@ static void __do_request(struct ceph_mds_client *mdsc,
 		}
 		trace_ceph_mdsc_suspend_request(mdsc, session, req,
 						ceph_mdsc_suspend_reason_session);
+		spin_lock(&mdsc->wait_list_lock);
 		list_add(&req->r_wait, &session->s_waiting);
+		spin_unlock(&mdsc->wait_list_lock);
 		goto out_session;
 	}
 
@@ -3850,7 +3859,9 @@ static void __wake_requests(struct ceph_mds_client *mdsc,
 	struct ceph_mds_request *req;
 	LIST_HEAD(tmp_list);
 
+	spin_lock(&mdsc->wait_list_lock);
 	list_splice_init(head, &tmp_list);
+	spin_unlock(&mdsc->wait_list_lock);
 
 	while (!list_empty(&tmp_list)) {
 		req = list_entry(tmp_list.next,
@@ -3892,7 +3903,9 @@ static void kick_requests(struct ceph_mds_client *mdsc, int mds)
 		if (req->r_session &&
 		    req->r_session->s_mds == mds) {
 			doutc(cl, " kicking tid %llu\n", req->r_tid);
+			spin_lock(&mdsc->wait_list_lock);
 			list_del_init(&req->r_wait);
+			spin_unlock(&mdsc->wait_list_lock);
 			trace_ceph_mdsc_resume_request(mdsc, req);
 			__do_request(mdsc, req);
 		}
@@ -6359,6 +6372,7 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
 	mdsc->snap_realms = RB_ROOT;
 	INIT_LIST_HEAD(&mdsc->snap_empty);
 	spin_lock_init(&mdsc->snap_empty_lock);
+	spin_lock_init(&mdsc->wait_list_lock);
 #if BITS_PER_LONG == 64
 	xa_init(&mdsc->request_tree);
 #else
@@ -6445,7 +6459,9 @@ static void wait_requests(struct ceph_mds_client *mdsc)
 		mutex_lock(&mdsc->mutex);
 		while ((req = __get_oldest_req(mdsc))) {
 			doutc(cl, "timed out on tid %llu\n", req->r_tid);
+			spin_lock(&mdsc->wait_list_lock);
 			list_del_init(&req->r_wait);
+			spin_unlock(&mdsc->wait_list_lock);
 			__unregister_request(mdsc, req);
 		}
 	}
diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
index c2de551607e3..0a9f89498604 100644
--- a/fs/ceph/mds_client.h
+++ b/fs/ceph/mds_client.h
@@ -534,6 +534,9 @@ struct ceph_mds_client {
 	struct list_head        snap_empty;
 	int			num_snap_realms;
 	spinlock_t              snap_empty_lock;  /* protect snap_empty */
+	spinlock_t              wait_list_lock;   /* protect waiting_for_map
+						   * and s_waiting lists
+						   */
 
 	u64                    last_tid;      /* most recent mds request */
 	atomic64_t             oldest_tid;    /* oldest incomplete mds request,

-- 
2.53.0

Re: [PATCH v2 3/5] ceph: add wait_list_lock for wait-list serialization

[Viacheslav Dubeyko]

On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@clyso.com>
> 
> Add a dedicated spinlock to protect the waiting_for_map and
> s_waiting lists, which are currently serialized by mdsc->mutex.
> This is a preparatory step for removing mdsc->mutex from the
> kick_requests() path: xa_for_each() is internally locked and the
> per-request filter checks are lockless, so only list_del_init()
> needs external protection.
> 
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  fs/ceph/mds_client.c | 18 +++++++++++++++++-
>  fs/ceph/mds_client.h |  3 +++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 239ed34886f9..a250938f32d3 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3669,7 +3669,9 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  			doutc(cl, "no mdsmap, waiting for map\n");
>  			trace_ceph_mdsc_suspend_request(mdsc,
> session, req,
>  							ceph_mdsc_su
> spend_reason_no_mdsmap);
> +			spin_lock(&mdsc->wait_list_lock);
>  			list_add(&req->r_wait, &mdsc-
> >waiting_for_map);
> +			spin_unlock(&mdsc->wait_list_lock);
>  			return;
>  		}
>  		if (!(mdsc->fsc->mount_options->flags &
> @@ -3692,7 +3694,9 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  		doutc(cl, "no mds or not active, waiting for
> map\n");
>  		trace_ceph_mdsc_suspend_request(mdsc, session, req,
>  						ceph_mdsc_suspend_re
> ason_no_active_mds);
> +		spin_lock(&mdsc->wait_list_lock);
>  		list_add(&req->r_wait, &mdsc->waiting_for_map);
> +		spin_unlock(&mdsc->wait_list_lock);
>  		return;
>  	}
>  
> @@ -3740,9 +3744,12 @@ static void __do_request(struct
> ceph_mds_client *mdsc,
>  			if (ceph_test_mount_opt(mdsc->fsc,
> CLEANRECOVER)) {
>  				trace_ceph_mdsc_suspend_request(mdsc
> , session, req,
>  								ceph
> _mdsc_suspend_reason_rejected);
> +				spin_lock(&mdsc->wait_list_lock);
>  				list_add(&req->r_wait, &mdsc-
> >waiting_for_map);
> -			} else
> +				spin_unlock(&mdsc->wait_list_lock);
> +			} else {
>  				err = -EACCES;
> +			}

Do we really need to add the curly brackets here?

Thanks,
Slava.

>  			goto out_session;
>  		}
>  
> @@ -3757,7 +3764,9 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  		}
>  		trace_ceph_mdsc_suspend_request(mdsc, session, req,
>  						ceph_mdsc_suspend_re
> ason_session);
> +		spin_lock(&mdsc->wait_list_lock);
>  		list_add(&req->r_wait, &session->s_waiting);
> +		spin_unlock(&mdsc->wait_list_lock);
>  		goto out_session;
>  	}
>  
> @@ -3850,7 +3859,9 @@ static void __wake_requests(struct
> ceph_mds_client *mdsc,
>  	struct ceph_mds_request *req;
>  	LIST_HEAD(tmp_list);
>  
> +	spin_lock(&mdsc->wait_list_lock);
>  	list_splice_init(head, &tmp_list);
> +	spin_unlock(&mdsc->wait_list_lock);
>  
>  	while (!list_empty(&tmp_list)) {
>  		req = list_entry(tmp_list.next,
> @@ -3892,7 +3903,9 @@ static void kick_requests(struct
> ceph_mds_client *mdsc, int mds)
>  		if (req->r_session &&
>  		    req->r_session->s_mds == mds) {
>  			doutc(cl, " kicking tid %llu\n", req-
> >r_tid);
> +			spin_lock(&mdsc->wait_list_lock);
>  			list_del_init(&req->r_wait);
> +			spin_unlock(&mdsc->wait_list_lock);
>  			trace_ceph_mdsc_resume_request(mdsc, req);
>  			__do_request(mdsc, req);
>  		}
> @@ -6359,6 +6372,7 @@ int ceph_mdsc_init(struct ceph_fs_client *fsc)
>  	mdsc->snap_realms = RB_ROOT;
>  	INIT_LIST_HEAD(&mdsc->snap_empty);
>  	spin_lock_init(&mdsc->snap_empty_lock);
> +	spin_lock_init(&mdsc->wait_list_lock);
>  #if BITS_PER_LONG == 64
>  	xa_init(&mdsc->request_tree);
>  #else
> @@ -6445,7 +6459,9 @@ static void wait_requests(struct
> ceph_mds_client *mdsc)
>  		mutex_lock(&mdsc->mutex);
>  		while ((req = __get_oldest_req(mdsc))) {
>  			doutc(cl, "timed out on tid %llu\n", req-
> >r_tid);
> +			spin_lock(&mdsc->wait_list_lock);
>  			list_del_init(&req->r_wait);
> +			spin_unlock(&mdsc->wait_list_lock);
>  			__unregister_request(mdsc, req);
>  		}
>  	}
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index c2de551607e3..0a9f89498604 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -534,6 +534,9 @@ struct ceph_mds_client {
>  	struct list_head        snap_empty;
>  	int			num_snap_realms;
>  	spinlock_t              snap_empty_lock;  /* protect
> snap_empty */
> +	spinlock_t              wait_list_lock;   /* protect
> waiting_for_map
> +						   * and s_waiting
> lists
> +						   */
>  
>  	u64                    last_tid;      /* most recent mds
> request */
>  	atomic64_t             oldest_tid;    /* oldest incomplete
> mds request,

Re: [PATCH v2 3/5] ceph: add wait_list_lock for wait-list serialization

[Xiubo Li]

On Thu, 16 Jul 2026 at 02:30, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> > From: Xiubo Li <xiubo.li@clyso.com>
> >
> > Add a dedicated spinlock to protect the waiting_for_map and
> > s_waiting lists, which are currently serialized by mdsc->mutex.
> > This is a preparatory step for removing mdsc->mutex from the
> > kick_requests() path: xa_for_each() is internally locked and the
> > per-request filter checks are lockless, so only list_del_init()
> > needs external protection.
> >
> > Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> > ---
> >  fs/ceph/mds_client.c | 18 +++++++++++++++++-
> >  fs/ceph/mds_client.h |  3 +++
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 239ed34886f9..a250938f32d3 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -3669,7 +3669,9 @@ static void __do_request(struct ceph_mds_client
> > *mdsc,
> >                       doutc(cl, "no mdsmap, waiting for map\n");
> >                       trace_ceph_mdsc_suspend_request(mdsc,
> > session, req,
> >                                                       ceph_mdsc_su
> > spend_reason_no_mdsmap);
> > +                     spin_lock(&mdsc->wait_list_lock);
> >                       list_add(&req->r_wait, &mdsc-
> > >waiting_for_map);
> > +                     spin_unlock(&mdsc->wait_list_lock);
> >                       return;
> >               }
> >               if (!(mdsc->fsc->mount_options->flags &
> > @@ -3692,7 +3694,9 @@ static void __do_request(struct ceph_mds_client
> > *mdsc,
> >               doutc(cl, "no mds or not active, waiting for
> > map\n");
> >               trace_ceph_mdsc_suspend_request(mdsc, session, req,
> >                                               ceph_mdsc_suspend_re
> > ason_no_active_mds);
> > +             spin_lock(&mdsc->wait_list_lock);
> >               list_add(&req->r_wait, &mdsc->waiting_for_map);
> > +             spin_unlock(&mdsc->wait_list_lock);
> >               return;
> >       }
> >
> > @@ -3740,9 +3744,12 @@ static void __do_request(struct
> > ceph_mds_client *mdsc,
> >                       if (ceph_test_mount_opt(mdsc->fsc,
> > CLEANRECOVER)) {
> >                               trace_ceph_mdsc_suspend_request(mdsc
> > , session, req,
> >                                                               ceph
> > _mdsc_suspend_reason_rejected);
> > +                             spin_lock(&mdsc->wait_list_lock);
> >                               list_add(&req->r_wait, &mdsc-
> > >waiting_for_map);
> > -                     } else
> > +                             spin_unlock(&mdsc->wait_list_lock);
> > +                     } else {
> >                               err = -EACCES;
> > +                     }
>
> Do we really need to add the curly brackets here?
>

Yeah.

Else the checkpatch.pl will warn:

WARNING: braces {} are necessary for all arms of this statement

It's a hard requirement of kernel coding style: if one branch of an if
has braces, the other must too.


Thanks
Xiubo

> Thanks,
> Slava.
[......]

[PATCH v2 4/5] ceph: move mdsc->mutex into __do_request()

[Xiubo Li via B4 Relay]

From: Xiubo Li <xiubo.li@clyso.com>

__do_request() now acquires mdsc->mutex on entry and unlocks at
every exit point, so callers no longer need to hold it.  As a
result kick_requests() is now completely free of mdsc->mutex:
xa_for_each() is internally locked, list_del_init() is guarded by
wait_list_lock, and __do_request() manages its own serialization.

Remove the no-longer-needed mutex_lock/unlock around __do_request()
in ceph_mdsc_submit_request() and handle_forward().  The do_request()
wrapper introduced in the previous commit is no longer necessary
and is dropped.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/mds_client.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index a250938f32d3..be0dae919d69 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -3637,9 +3637,12 @@ static void __do_request(struct ceph_mds_client *mdsc,
 	int err = 0;
 	bool random;
 
+	mutex_lock(&mdsc->mutex);
+
 	if (req->r_err || test_bit(CEPH_MDS_R_GOT_RESULT, &req->r_req_flags)) {
 		if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
 			__unregister_request(mdsc, req);
+		mutex_unlock(&mdsc->mutex);
 		return;
 	}
 
@@ -3672,6 +3675,7 @@ static void __do_request(struct ceph_mds_client *mdsc,
 			spin_lock(&mdsc->wait_list_lock);
 			list_add(&req->r_wait, &mdsc->waiting_for_map);
 			spin_unlock(&mdsc->wait_list_lock);
+			mutex_unlock(&mdsc->mutex);
 			return;
 		}
 		if (!(mdsc->fsc->mount_options->flags &
@@ -3697,6 +3701,7 @@ static void __do_request(struct ceph_mds_client *mdsc,
 		spin_lock(&mdsc->wait_list_lock);
 		list_add(&req->r_wait, &mdsc->waiting_for_map);
 		spin_unlock(&mdsc->wait_list_lock);
+		mutex_unlock(&mdsc->mutex);
 		return;
 	}
 
@@ -3770,6 +3775,8 @@ static void __do_request(struct ceph_mds_client *mdsc,
 		goto out_session;
 	}
 
+	mutex_unlock(&mdsc->mutex);
+
 	/* send request */
 	req->r_resend_mds = -1;   /* forget any previous mds hint */
 
@@ -3800,6 +3807,7 @@ static void __do_request(struct ceph_mds_client *mdsc,
 			err = wait_on_bit(&di->flags, CEPH_DENTRY_ASYNC_CREATE_BIT,
 					  TASK_KILLABLE);
 			if (err) {
+				mutex_lock(&mdsc->mutex);
 				mutex_lock(&req->r_fill_mutex);
 				set_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags);
 				mutex_unlock(&req->r_fill_mutex);
@@ -3837,6 +3845,8 @@ static void __do_request(struct ceph_mds_client *mdsc,
 
 	err = __send_request(session, req, false);
 
+	mutex_lock(&mdsc->mutex);
+
 out_session:
 	ceph_put_mds_session(session);
 finish:
@@ -3846,12 +3856,10 @@ static void __do_request(struct ceph_mds_client *mdsc,
 		complete_request(mdsc, req);
 		__unregister_request(mdsc, req);
 	}
+	mutex_unlock(&mdsc->mutex);
 	return;
 }
 
-/*
- * called under mdsc->mutex
- */
 static void __wake_requests(struct ceph_mds_client *mdsc,
 			    struct list_head *head)
 {
@@ -3969,11 +3977,12 @@ int ceph_mdsc_submit_request(struct ceph_mds_client *mdsc, struct inode *dir,
 	doutc(cl, "submit_request on %p for inode %p\n", req, dir);
 	mutex_lock(&mdsc->mutex);
 	__register_request(mdsc, req, dir);
+	mutex_unlock(&mdsc->mutex);
+
 	trace_ceph_mdsc_submit_request(mdsc, req);
 	if (!req->r_err)
 		__do_request(mdsc, req);
 	err = req->r_err;
-	mutex_unlock(&mdsc->mutex);
 	return err;
 }
 
@@ -4346,13 +4355,14 @@ static void handle_forward(struct ceph_mds_client *mdsc,
 		req->r_num_fwd = fwd_seq;
 		req->r_resend_mds = next_mds;
 		put_request_session(req);
-		__do_request(mdsc, req);
 	}
 	mutex_unlock(&mdsc->mutex);
 
 	/* kick calling process */
 	if (aborted)
 		complete_request(mdsc, req);
+	else if (!test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
+		__do_request(mdsc, req);
 	ceph_mdsc_put_request(req);
 	return;
 
@@ -4676,11 +4686,9 @@ static void handle_session(struct ceph_mds_session *session,
 
 	mutex_unlock(&session->s_mutex);
 	if (wake) {
-		mutex_lock(&mdsc->mutex);
 		__wake_requests(mdsc, &session->s_waiting);
 		if (wake == 2)
 			kick_requests(mdsc, mds);
-		mutex_unlock(&mdsc->mutex);
 	}
 	if (op == CEPH_SESSION_CLOSE)
 		ceph_put_mds_session(session);
@@ -5326,9 +5334,7 @@ static int send_mds_reconnect(struct ceph_mds_client *mdsc,
 
 	mutex_unlock(&session->s_mutex);
 
-	mutex_lock(&mdsc->mutex);
 	__wake_requests(mdsc, &session->s_waiting);
-	mutex_unlock(&mdsc->mutex);
 
 	up_read(&mdsc->snap_rwsem);
 	ceph_pagelist_release(recon_state.pagelist);
@@ -5773,8 +5779,8 @@ static void ceph_mdsc_reset_workfn(struct work_struct *work)
 		}
 		sessions[i]->s_state = CEPH_MDS_SESSION_CLOSED;
 		__unregister_session(mdsc, sessions[i]);
-		__wake_requests(mdsc, &sessions[i]->s_waiting);
 		mutex_unlock(&mdsc->mutex);
+		__wake_requests(mdsc, &sessions[i]->s_waiting);
 
 		mutex_lock(&sessions[i]->s_mutex);
 		cleanup_session_requests(mdsc, sessions[i]);
@@ -5785,9 +5791,7 @@ static void ceph_mdsc_reset_workfn(struct work_struct *work)
 
 		ceph_put_mds_session(sessions[i]);
 
-		mutex_lock(&mdsc->mutex);
 		kick_requests(mdsc, mds);
-		mutex_unlock(&mdsc->mutex);
 
 		torn_down++;
 		pr_info_client(cl, "mds%d session reset complete\n", mds);
@@ -5903,8 +5907,8 @@ static void check_new_map(struct ceph_mds_client *mdsc,
 			/* force close session for stopped mds */
 			ceph_get_mds_session(s);
 			__unregister_session(mdsc, s);
-			__wake_requests(mdsc, &s->s_waiting);
 			mutex_unlock(&mdsc->mutex);
+			__wake_requests(mdsc, &s->s_waiting);
 
 			mutex_lock(&s->s_mutex);
 			cleanup_session_requests(mdsc, s);
@@ -5913,8 +5917,8 @@ static void check_new_map(struct ceph_mds_client *mdsc,
 
 			ceph_put_mds_session(s);
 
-			mutex_lock(&mdsc->mutex);
 			kick_requests(mdsc, i);
+			mutex_lock(&mdsc->mutex);
 			continue;
 		}
 
@@ -5958,8 +5962,8 @@ static void check_new_map(struct ceph_mds_client *mdsc,
 			    oldstate != CEPH_MDS_STATE_STARTING)
 				pr_info_client(cl, "mds%d recovery completed\n",
 					       s->s_mds);
-			kick_requests(mdsc, i);
 			mutex_unlock(&mdsc->mutex);
+			kick_requests(mdsc, i);
 			mutex_lock(&s->s_mutex);
 			mutex_lock(&mdsc->mutex);
 			ceph_kick_flushing_caps(mdsc, s);
@@ -6931,8 +6935,8 @@ void ceph_mdsc_force_umount(struct ceph_mds_client *mdsc)
 
 		if (session->s_state == CEPH_MDS_SESSION_REJECTED)
 			__unregister_session(mdsc, session);
-		__wake_requests(mdsc, &session->s_waiting);
 		mutex_unlock(&mdsc->mutex);
+		__wake_requests(mdsc, &session->s_waiting);
 
 		mutex_lock(&session->s_mutex);
 		__close_session(mdsc, session);
@@ -6943,11 +6947,11 @@ void ceph_mdsc_force_umount(struct ceph_mds_client *mdsc)
 		mutex_unlock(&session->s_mutex);
 		ceph_put_mds_session(session);
 
-		mutex_lock(&mdsc->mutex);
 		kick_requests(mdsc, mds);
+		mutex_lock(&mdsc->mutex);
 	}
-	__wake_requests(mdsc, &mdsc->waiting_for_map);
 	mutex_unlock(&mdsc->mutex);
+	__wake_requests(mdsc, &mdsc->waiting_for_map);
 }
 
 static void ceph_mdsc_stop(struct ceph_mds_client *mdsc)
@@ -7091,8 +7095,8 @@ void ceph_mdsc_handle_fsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
 err_out:
 	mutex_lock(&mdsc->mutex);
 	mdsc->mdsmap_err = err;
-	__wake_requests(mdsc, &mdsc->waiting_for_map);
 	mutex_unlock(&mdsc->mutex);
+	__wake_requests(mdsc, &mdsc->waiting_for_map);
 }
 
 /*
@@ -7143,11 +7147,11 @@ void ceph_mdsc_handle_mdsmap(struct ceph_mds_client *mdsc, struct ceph_msg *msg)
 	mdsc->fsc->max_file_size = min((loff_t)mdsc->mdsmap->m_max_file_size,
 					MAX_LFS_FILESIZE);
 
+	mutex_unlock(&mdsc->mutex);
 	__wake_requests(mdsc, &mdsc->waiting_for_map);
 	ceph_monc_got_map(&mdsc->fsc->client->monc, CEPH_SUB_MDSMAP,
 			  mdsc->mdsmap->m_epoch);
 
-	mutex_unlock(&mdsc->mutex);
 	schedule_delayed(mdsc, 0);
 	return;
 
@@ -7245,8 +7249,8 @@ static void mds_peer_reset(struct ceph_connection *con)
 		ceph_get_mds_session(s);
 		s->s_state = CEPH_MDS_SESSION_CLOSED;
 		__unregister_session(mdsc, s);
-		__wake_requests(mdsc, &s->s_waiting);
 		mutex_unlock(&mdsc->mutex);
+		__wake_requests(mdsc, &s->s_waiting);
 
 		mutex_lock(&s->s_mutex);
 		cleanup_session_requests(mdsc, s);
@@ -7255,9 +7259,7 @@ static void mds_peer_reset(struct ceph_connection *con)
 
 		wake_up_all(&mdsc->session_close_wq);
 
-		mutex_lock(&mdsc->mutex);
 		kick_requests(mdsc, s->s_mds);
-		mutex_unlock(&mdsc->mutex);
 
 		ceph_put_mds_session(s);
 		break;

-- 
2.53.0

Re: [PATCH v2 4/5] ceph: move mdsc->mutex into __do_request()

[Viacheslav Dubeyko]

On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@clyso.com>
> 
> __do_request() now acquires mdsc->mutex on entry and unlocks at
> every exit point, so callers no longer need to hold it.  As a
> result kick_requests() is now completely free of mdsc->mutex:
> xa_for_each() is internally locked, list_del_init() is guarded by
> wait_list_lock, and __do_request() manages its own serialization.
> 
> Remove the no-longer-needed mutex_lock/unlock around __do_request()
> in ceph_mdsc_submit_request() and handle_forward().  The do_request()
> wrapper introduced in the previous commit is no longer necessary
> and is dropped.

Which do_request() wrapper do you mean here? Maybe, I am missing
something.

> 
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  fs/ceph/mds_client.c | 48 +++++++++++++++++++++++++-----------------
> ------
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index a250938f32d3..be0dae919d69 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3637,9 +3637,12 @@ static void __do_request(struct
> ceph_mds_client *mdsc,
>  	int err = 0;
>  	bool random;
>  
> +	mutex_lock(&mdsc->mutex);
> +
>  	if (req->r_err || test_bit(CEPH_MDS_R_GOT_RESULT, &req-
> >r_req_flags)) {
>  		if (test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
>  			__unregister_request(mdsc, req);
> +		mutex_unlock(&mdsc->mutex);
>  		return;
>  	}
>  
> @@ -3672,6 +3675,7 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  			spin_lock(&mdsc->wait_list_lock);
>  			list_add(&req->r_wait, &mdsc-
> >waiting_for_map);
>  			spin_unlock(&mdsc->wait_list_lock);
> +			mutex_unlock(&mdsc->mutex);
>  			return;
>  		}
>  		if (!(mdsc->fsc->mount_options->flags &
> @@ -3697,6 +3701,7 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  		spin_lock(&mdsc->wait_list_lock);
>  		list_add(&req->r_wait, &mdsc->waiting_for_map);
>  		spin_unlock(&mdsc->wait_list_lock);
> +		mutex_unlock(&mdsc->mutex);
>  		return;
>  	}
>  
> @@ -3770,6 +3775,8 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  		goto out_session;
>  	}
>  
> +	mutex_unlock(&mdsc->mutex);
> +
>  	/* send request */
>  	req->r_resend_mds = -1;   /* forget any previous mds hint */
>  
> @@ -3800,6 +3807,7 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  			err = wait_on_bit(&di->flags,
> CEPH_DENTRY_ASYNC_CREATE_BIT,
>  					  TASK_KILLABLE);
>  			if (err) {
> +				mutex_lock(&mdsc->mutex);
>  				mutex_lock(&req->r_fill_mutex);
>  				set_bit(CEPH_MDS_R_ABORTED, &req-
> >r_req_flags);
>  				mutex_unlock(&req->r_fill_mutex);
> @@ -3837,6 +3845,8 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  
>  	err = __send_request(session, req, false);
>  
> +	mutex_lock(&mdsc->mutex);
> +
>  out_session:
>  	ceph_put_mds_session(session);
>  finish:
> @@ -3846,12 +3856,10 @@ static void __do_request(struct
> ceph_mds_client *mdsc,
>  		complete_request(mdsc, req);
>  		__unregister_request(mdsc, req);
>  	}
> +	mutex_unlock(&mdsc->mutex);
>  	return;
>  }
>  
> -/*
> - * called under mdsc->mutex
> - */
>  static void __wake_requests(struct ceph_mds_client *mdsc,
>  			    struct list_head *head)
>  {
> @@ -3969,11 +3977,12 @@ int ceph_mdsc_submit_request(struct
> ceph_mds_client *mdsc, struct inode *dir,
>  	doutc(cl, "submit_request on %p for inode %p\n", req, dir);
>  	mutex_lock(&mdsc->mutex);
>  	__register_request(mdsc, req, dir);
> +	mutex_unlock(&mdsc->mutex);
> +
>  	trace_ceph_mdsc_submit_request(mdsc, req);
>  	if (!req->r_err)
>  		__do_request(mdsc, req);
>  	err = req->r_err;
> -	mutex_unlock(&mdsc->mutex);
>  	return err;
>  }
>  
> @@ -4346,13 +4355,14 @@ static void handle_forward(struct
> ceph_mds_client *mdsc,
>  		req->r_num_fwd = fwd_seq;
>  		req->r_resend_mds = next_mds;
>  		put_request_session(req);
> -		__do_request(mdsc, req);
>  	}
>  	mutex_unlock(&mdsc->mutex);
>  
>  	/* kick calling process */
>  	if (aborted)
>  		complete_request(mdsc, req);
> +	else if (!test_bit(CEPH_MDS_R_ABORTED, &req->r_req_flags))
> +		__do_request(mdsc, req);
>  	ceph_mdsc_put_request(req);
>  	return;
>  
> @@ -4676,11 +4686,9 @@ static void handle_session(struct
> ceph_mds_session *session,
>  
>  	mutex_unlock(&session->s_mutex);
>  	if (wake) {
> -		mutex_lock(&mdsc->mutex);
>  		__wake_requests(mdsc, &session->s_waiting);
>  		if (wake == 2)
>  			kick_requests(mdsc, mds);
> -		mutex_unlock(&mdsc->mutex);
>  	}
>  	if (op == CEPH_SESSION_CLOSE)
>  		ceph_put_mds_session(session);
> @@ -5326,9 +5334,7 @@ static int send_mds_reconnect(struct
> ceph_mds_client *mdsc,
>  
>  	mutex_unlock(&session->s_mutex);
>  
> -	mutex_lock(&mdsc->mutex);
>  	__wake_requests(mdsc, &session->s_waiting);
> -	mutex_unlock(&mdsc->mutex);
>  
>  	up_read(&mdsc->snap_rwsem);
>  	ceph_pagelist_release(recon_state.pagelist);
> @@ -5773,8 +5779,8 @@ static void ceph_mdsc_reset_workfn(struct
> work_struct *work)
>  		}
>  		sessions[i]->s_state = CEPH_MDS_SESSION_CLOSED;
>  		__unregister_session(mdsc, sessions[i]);
> -		__wake_requests(mdsc, &sessions[i]->s_waiting);
>  		mutex_unlock(&mdsc->mutex);
> +		__wake_requests(mdsc, &sessions[i]->s_waiting);
>  
>  		mutex_lock(&sessions[i]->s_mutex);
>  		cleanup_session_requests(mdsc, sessions[i]);
> @@ -5785,9 +5791,7 @@ static void ceph_mdsc_reset_workfn(struct
> work_struct *work)
>  
>  		ceph_put_mds_session(sessions[i]);
>  
> -		mutex_lock(&mdsc->mutex);
>  		kick_requests(mdsc, mds);
> -		mutex_unlock(&mdsc->mutex);
>  
>  		torn_down++;
>  		pr_info_client(cl, "mds%d session reset complete\n",
> mds);
> @@ -5903,8 +5907,8 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  			/* force close session for stopped mds */
>  			ceph_get_mds_session(s);
>  			__unregister_session(mdsc, s);
> -			__wake_requests(mdsc, &s->s_waiting);
>  			mutex_unlock(&mdsc->mutex);
> +			__wake_requests(mdsc, &s->s_waiting);
>  
>  			mutex_lock(&s->s_mutex);
>  			cleanup_session_requests(mdsc, s);
> @@ -5913,8 +5917,8 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  
>  			ceph_put_mds_session(s);
>  
> -			mutex_lock(&mdsc->mutex);
>  			kick_requests(mdsc, i);
> +			mutex_lock(&mdsc->mutex);
>  			continue;
>  		}
>  
> @@ -5958,8 +5962,8 @@ static void check_new_map(struct
> ceph_mds_client *mdsc,
>  			    oldstate != CEPH_MDS_STATE_STARTING)
>  				pr_info_client(cl, "mds%d recovery
> completed\n",
>  					       s->s_mds);
> -			kick_requests(mdsc, i);
>  			mutex_unlock(&mdsc->mutex);
> +			kick_requests(mdsc, i);
>  			mutex_lock(&s->s_mutex);
>  			mutex_lock(&mdsc->mutex);
>  			ceph_kick_flushing_caps(mdsc, s);
> @@ -6931,8 +6935,8 @@ void ceph_mdsc_force_umount(struct
> ceph_mds_client *mdsc)
>  
>  		if (session->s_state == CEPH_MDS_SESSION_REJECTED)
>  			__unregister_session(mdsc, session);
> -		__wake_requests(mdsc, &session->s_waiting);
>  		mutex_unlock(&mdsc->mutex);
> +		__wake_requests(mdsc, &session->s_waiting);
>  
>  		mutex_lock(&session->s_mutex);
>  		__close_session(mdsc, session);
> @@ -6943,11 +6947,11 @@ void ceph_mdsc_force_umount(struct
> ceph_mds_client *mdsc)
>  		mutex_unlock(&session->s_mutex);
>  		ceph_put_mds_session(session);
>  
> -		mutex_lock(&mdsc->mutex);
>  		kick_requests(mdsc, mds);
> +		mutex_lock(&mdsc->mutex);
>  	}
> -	__wake_requests(mdsc, &mdsc->waiting_for_map);
>  	mutex_unlock(&mdsc->mutex);
> +	__wake_requests(mdsc, &mdsc->waiting_for_map);
>  }
>  
>  static void ceph_mdsc_stop(struct ceph_mds_client *mdsc)
> @@ -7091,8 +7095,8 @@ void ceph_mdsc_handle_fsmap(struct
> ceph_mds_client *mdsc, struct ceph_msg *msg)
>  err_out:
>  	mutex_lock(&mdsc->mutex);
>  	mdsc->mdsmap_err = err;
> -	__wake_requests(mdsc, &mdsc->waiting_for_map);
>  	mutex_unlock(&mdsc->mutex);
> +	__wake_requests(mdsc, &mdsc->waiting_for_map);
>  }
>  
>  /*
> @@ -7143,11 +7147,11 @@ void ceph_mdsc_handle_mdsmap(struct
> ceph_mds_client *mdsc, struct ceph_msg *msg)
>  	mdsc->fsc->max_file_size = min((loff_t)mdsc->mdsmap-
> >m_max_file_size,
>  					MAX_LFS_FILESIZE);
>  
> +	mutex_unlock(&mdsc->mutex);
>  	__wake_requests(mdsc, &mdsc->waiting_for_map);
>  	ceph_monc_got_map(&mdsc->fsc->client->monc, CEPH_SUB_MDSMAP,
>  			  mdsc->mdsmap->m_epoch);
>  
> -	mutex_unlock(&mdsc->mutex);
>  	schedule_delayed(mdsc, 0);
>  	return;
>  
> @@ -7245,8 +7249,8 @@ static void mds_peer_reset(struct
> ceph_connection *con)
>  		ceph_get_mds_session(s);
>  		s->s_state = CEPH_MDS_SESSION_CLOSED;
>  		__unregister_session(mdsc, s);
> -		__wake_requests(mdsc, &s->s_waiting);
>  		mutex_unlock(&mdsc->mutex);
> +		__wake_requests(mdsc, &s->s_waiting);
>  
>  		mutex_lock(&s->s_mutex);
>  		cleanup_session_requests(mdsc, s);
> @@ -7255,9 +7259,7 @@ static void mds_peer_reset(struct
> ceph_connection *con)
>  
>  		wake_up_all(&mdsc->session_close_wq);
>  
> -		mutex_lock(&mdsc->mutex);
>  		kick_requests(mdsc, s->s_mds);
> -		mutex_unlock(&mdsc->mutex);
>  
>  		ceph_put_mds_session(s);
>  		break;

Maybe, I am missing something. But kick_requests() reads req-
>r_attempts [1] and req->r_session [2] with no lock at all. While
__do_request()/__prepare_send_request() can write those same fields
[3,4] from another thread without holding mdsc->mutex at the write
point either. Could it be the issue?

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3832
[2]
https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3834
[3]
https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3658
[4]
https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3475

Re: [PATCH v2 4/5] ceph: move mdsc->mutex into __do_request()

[Xiubo Li]

On Thu, 16 Jul 2026 at 02:53, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> > From: Xiubo Li <xiubo.li@clyso.com>
> >
> > __do_request() now acquires mdsc->mutex on entry and unlocks at
> > every exit point, so callers no longer need to hold it.  As a
> > result kick_requests() is now completely free of mdsc->mutex:
> > xa_for_each() is internally locked, list_del_init() is guarded by
> > wait_list_lock, and __do_request() manages its own serialization.
> >
> > Remove the no-longer-needed mutex_lock/unlock around __do_request()
> > in ceph_mdsc_submit_request() and handle_forward().  The do_request()
> > wrapper introduced in the previous commit is no longer necessary
> > and is dropped.
>
> Which do_request() wrapper do you mean here? Maybe, I am missing
> something.

The commit message is stale — do_request() wrapper was in an earlier
draft and was already dropped. Current code has none.
Let me fix it.

[......]
> > @@ -7255,9 +7259,7 @@ static void mds_peer_reset(struct
> > ceph_connection *con)
> >
> >               wake_up_all(&mdsc->session_close_wq);
> >
> > -             mutex_lock(&mdsc->mutex);
> >               kick_requests(mdsc, s->s_mds);
> > -             mutex_unlock(&mdsc->mutex);
> >
> >               ceph_put_mds_session(s);
> >               break;
>
> Maybe, I am missing something. But kick_requests() reads req-
> >r_attempts [1] and req->r_session [2] with no lock at all. While
> __do_request()/__prepare_send_request() can write those same fields
> [3,4] from another thread without holding mdsc->mutex at the write
> point either. Could it be the issue?
>

It's fine.

r_attempts is indeed read and written without any lock now, but
the value is monotonic and the read side is purely advisory in
kick_requests() — it decides whether to pull a request off the
wait list and retry it.  The worst case is that kick_requests()
either processes one extra request (which __do_request() will
immediately bail out on because GOT_RESULT or r_err is already
set), or misses one (which the next kick_requests() round will
pick up).  On all architectures Linux supports, an aligned int
read/write is single-instruction atomic, so there is no torn-read
risk.

r_session is a write-once field — set once in __do_request() and
never changed afterwards.  The reader sees either NULL (and skips
the request) or a valid, stable pointer.  No concurrent writes
occur after the initial store.

That said, this is a data race in the formal sense and KCSAN will
flag it.  I'll add READ_ONCE()/WRITE_ONCE() annotations in the
next revision to make the intent explicit and keep the tooling
quiet.

Thanks
Xiubo

> Thanks,
> Slava.
>
> [1]
> https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3832
> [2]
> https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3834
> [3]
> https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3658
> [4]
> https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3475

[PATCH v2 5/5] ceph: narrow mdsc->mutex scope in replay_unsafe_requests

[Xiubo Li via B4 Relay]

From: Xiubo Li <xiubo.li@clyso.com>

__send_request() is lockless so holding mdsc->mutex across the
replay loop only serializes the list iteration itself.  Collect
the unsafe list entries under the mutex, then replay them without
it.  The old-request tree walk uses xa_for_each() which is
internally locked.

Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
---
 fs/ceph/mds_client.c | 51 ++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index be0dae919d69..c313574def74 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4744,30 +4744,66 @@ static void replay_unsafe_requests(struct ceph_mds_client *mdsc,
 #else
 	struct rb_node *p;
 #endif
+	LIST_HEAD(replay_list);
 
 	doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
 
+#if BITS_PER_LONG == 64
+	/* collect unsafe requests under the mutex */
 	mutex_lock(&mdsc->mutex);
-	list_for_each_entry_safe(req, nreq, &session->s_unsafe, r_unsafe_item)
+	list_for_each_entry_safe(req, nreq, &session->s_unsafe,
+				 r_unsafe_item) {
+		ceph_mdsc_get_request(req);
+		list_move(&req->r_unsafe_item, &replay_list);
+	}
+	mutex_unlock(&mdsc->mutex);
+
+	/* replay unsafe requests (local list, no mutex needed) */
+	list_for_each_entry_safe(req, nreq, &replay_list, r_unsafe_item) {
 		__send_request(session, req, true);
+		list_del_init(&req->r_unsafe_item);
+		ceph_mdsc_put_request(req);
+	}
 
 	/*
-	 * also re-send old requests when MDS enters reconnect stage. So that MDS
-	 * can process completed request in clientreplay stage.
+	 * also re-send old requests when MDS enters reconnect stage.
+	 * xa_for_each() is internally locked.
 	 */
-#if BITS_PER_LONG == 64
 	idx = 0;
 	xa_for_each(&mdsc->request_tree, idx, req) {
-#else
+		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags))
+			continue;
+		if (req->r_attempts == 0)
+			continue;
+		if (!req->r_session)
+			continue;
+		if (req->r_session->s_mds != session->s_mds)
+			continue;
+
+		ceph_mdsc_release_dir_caps_async(req);
+
+		ceph_mdsc_get_request(req);
+		__send_request(session, req, true);
+		ceph_mdsc_put_request(req);
+	}
+#else /* BITS_PER_LONG != 64 — keep mutex for rb-tree iteration */
+	mutex_lock(&mdsc->mutex);
+	list_for_each_entry_safe(req, nreq, &session->s_unsafe,
+				 r_unsafe_item)
+		__send_request(session, req, true);
+
+	/*
+	 * also re-send old requests when MDS enters reconnect stage.
+	 * Must hold mutex for rb_first()/rb_next().
+	 */
 	p = rb_first(&mdsc->request_tree);
 	while (p) {
 		req = rb_entry(p, struct ceph_mds_request, r_node);
 		p = rb_next(p);
-#endif
 		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req->r_req_flags))
 			continue;
 		if (req->r_attempts == 0)
-			continue; /* only old requests */
+			continue;
 		if (!req->r_session)
 			continue;
 		if (req->r_session->s_mds != session->s_mds)
@@ -4778,6 +4814,7 @@ static void replay_unsafe_requests(struct ceph_mds_client *mdsc,
 		__send_request(session, req, true);
 	}
 	mutex_unlock(&mdsc->mutex);
+#endif
 }
 
 static int send_reconnect_partial(struct ceph_reconnect_state *recon_state)

-- 
2.53.0

Re: [PATCH v2 5/5] ceph: narrow mdsc->mutex scope in replay_unsafe_requests

[Viacheslav Dubeyko]

On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@clyso.com>
> 
> __send_request() is lockless so holding mdsc->mutex across the
> replay loop only serializes the list iteration itself.  Collect
> the unsafe list entries under the mutex, then replay them without
> it.  The old-request tree walk uses xa_for_each() which is
> internally locked.
> 
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  fs/ceph/mds_client.c | 51
> ++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index be0dae919d69..c313574def74 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4744,30 +4744,66 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  #else
>  	struct rb_node *p;
>  #endif
> +	LIST_HEAD(replay_list);
>  
>  	doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
>  
> +#if BITS_PER_LONG == 64
> +	/* collect unsafe requests under the mutex */
>  	mutex_lock(&mdsc->mutex);
> -	list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> r_unsafe_item)
> +	list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> +				 r_unsafe_item) {
> +		ceph_mdsc_get_request(req);
> +		list_move(&req->r_unsafe_item, &replay_list);
> +	}
> +	mutex_unlock(&mdsc->mutex);
> +
> +	/* replay unsafe requests (local list, no mutex needed) */
> +	list_for_each_entry_safe(req, nreq, &replay_list,
> r_unsafe_item) {
>  		__send_request(session, req, true);
> +		list_del_init(&req->r_unsafe_item);
> +		ceph_mdsc_put_request(req);
> +	}
>  
>  	/*
> -	 * also re-send old requests when MDS enters reconnect
> stage. So that MDS
> -	 * can process completed request in clientreplay stage.
> +	 * also re-send old requests when MDS enters reconnect
> stage.
> +	 * xa_for_each() is internally locked.
>  	 */
> -#if BITS_PER_LONG == 64
>  	idx = 0;
>  	xa_for_each(&mdsc->request_tree, idx, req) {
> -#else
> +		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> >r_req_flags))
> +			continue;
> +		if (req->r_attempts == 0)
> +			continue;
> +		if (!req->r_session)
> +			continue;
> +		if (req->r_session->s_mds != session->s_mds)
> +			continue;
> +
> +		ceph_mdsc_release_dir_caps_async(req);
> +
> +		ceph_mdsc_get_request(req);
> +		__send_request(session, req, true);
> +		ceph_mdsc_put_request(req);
> +	}
> +#else /* BITS_PER_LONG != 64 — keep mutex for rb-tree iteration */
> +	mutex_lock(&mdsc->mutex);
> +	list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> +				 r_unsafe_item)
> +		__send_request(session, req, true);
> +
> +	/*
> +	 * also re-send old requests when MDS enters reconnect
> stage.
> +	 * Must hold mutex for rb_first()/rb_next().
> +	 */
>  	p = rb_first(&mdsc->request_tree);
>  	while (p) {
>  		req = rb_entry(p, struct ceph_mds_request, r_node);
>  		p = rb_next(p);
> -#endif
>  		if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> >r_req_flags))
>  			continue;
>  		if (req->r_attempts == 0)
> -			continue; /* only old requests */
> +			continue;
>  		if (!req->r_session)
>  			continue;
>  		if (req->r_session->s_mds != session->s_mds)
> @@ -4778,6 +4814,7 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  		__send_request(session, req, true);
>  	}
>  	mutex_unlock(&mdsc->mutex);
> +#endif
>  }
>  
>  static int send_reconnect_partial(struct ceph_reconnect_state
> *recon_state)

I believe we have problem in the patch.

Once mdsc->mutex is dropped in the new code, a concurrent "safe" reply
for one of the unsafe requests currently sitting on the private
replay_list can call __unregister_request() → list_del_init(&req-
>r_unsafe_item) on a node that Thread A (running
replay_unsafe_requests()) is simultaneously iterating and unlinking via
its own unlocked list_for_each_entry_safe(...) { ...;
list_del_init(&req->r_unsafe_item); ... }.

Two threads mutating the same prev/next pointers with no shared lock is
a linked-list corruption — it can skip or duplicate entries in
replay_list, dereference a pointer clobbered mid-update, or trigger a
list_del corruption BUG() if that's enabled.

What do you think?

Thanks,
Slava.

Re: [PATCH v2 5/5] ceph: narrow mdsc->mutex scope in replay_unsafe_requests

[Xiubo Li]

。

On Thu, 16 Jul 2026 at 03:12, Viacheslav Dubeyko <slava@dubeyko.com> wrote:
>
> On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> > From: Xiubo Li <xiubo.li@clyso.com>
> >
> > __send_request() is lockless so holding mdsc->mutex across the
> > replay loop only serializes the list iteration itself.  Collect
> > the unsafe list entries under the mutex, then replay them without
> > it.  The old-request tree walk uses xa_for_each() which is
> > internally locked.
> >
> > Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> > ---
> >  fs/ceph/mds_client.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index be0dae919d69..c313574def74 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -4744,30 +4744,66 @@ static void replay_unsafe_requests(struct
> > ceph_mds_client *mdsc,
> >  #else
> >       struct rb_node *p;
> >  #endif
> > +     LIST_HEAD(replay_list);
> >
> >       doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
> >
> > +#if BITS_PER_LONG == 64
> > +     /* collect unsafe requests under the mutex */
> >       mutex_lock(&mdsc->mutex);
> > -     list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> > r_unsafe_item)
> > +     list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> > +                              r_unsafe_item) {
> > +             ceph_mdsc_get_request(req);
> > +             list_move(&req->r_unsafe_item, &replay_list);
> > +     }
> > +     mutex_unlock(&mdsc->mutex);
> > +
> > +     /* replay unsafe requests (local list, no mutex needed) */
> > +     list_for_each_entry_safe(req, nreq, &replay_list,
> > r_unsafe_item) {
> >               __send_request(session, req, true);
> > +             list_del_init(&req->r_unsafe_item);
> > +             ceph_mdsc_put_request(req);
> > +     }
> >
> >       /*
> > -      * also re-send old requests when MDS enters reconnect
> > stage. So that MDS
> > -      * can process completed request in clientreplay stage.
> > +      * also re-send old requests when MDS enters reconnect
> > stage.
> > +      * xa_for_each() is internally locked.
> >        */
> > -#if BITS_PER_LONG == 64
> >       idx = 0;
> >       xa_for_each(&mdsc->request_tree, idx, req) {
> > -#else
> > +             if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> > >r_req_flags))
> > +                     continue;
> > +             if (req->r_attempts == 0)
> > +                     continue;
> > +             if (!req->r_session)
> > +                     continue;
> > +             if (req->r_session->s_mds != session->s_mds)
> > +                     continue;
> > +
> > +             ceph_mdsc_release_dir_caps_async(req);
> > +
> > +             ceph_mdsc_get_request(req);
> > +             __send_request(session, req, true);
> > +             ceph_mdsc_put_request(req);
> > +     }
> > +#else /* BITS_PER_LONG != 64 — keep mutex for rb-tree iteration */
> > +     mutex_lock(&mdsc->mutex);
> > +     list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> > +                              r_unsafe_item)
> > +             __send_request(session, req, true);
> > +
> > +     /*
> > +      * also re-send old requests when MDS enters reconnect
> > stage.
> > +      * Must hold mutex for rb_first()/rb_next().
> > +      */
> >       p = rb_first(&mdsc->request_tree);
> >       while (p) {
> >               req = rb_entry(p, struct ceph_mds_request, r_node);
> >               p = rb_next(p);
> > -#endif
> >               if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> > >r_req_flags))
> >                       continue;
> >               if (req->r_attempts == 0)
> > -                     continue; /* only old requests */
> > +                     continue;
> >               if (!req->r_session)
> >                       continue;
> >               if (req->r_session->s_mds != session->s_mds)
> > @@ -4778,6 +4814,7 @@ static void replay_unsafe_requests(struct
> > ceph_mds_client *mdsc,
> >               __send_request(session, req, true);
> >       }
> >       mutex_unlock(&mdsc->mutex);
> > +#endif
> >  }
> >
> >  static int send_reconnect_partial(struct ceph_reconnect_state
> > *recon_state)
>
> I believe we have problem in the patch.
>
> Once mdsc->mutex is dropped in the new code, a concurrent "safe" reply
> for one of the unsafe requests currently sitting on the private
> replay_list can call __unregister_request() → list_del_init(&req-
> >r_unsafe_item) on a node that Thread A (running
> replay_unsafe_requests()) is simultaneously iterating and unlinking via
> its own unlocked list_for_each_entry_safe(...) { ...;
> list_del_init(&req->r_unsafe_item); ... }.
>
> Two threads mutating the same prev/next pointers with no shared lock is
> a linked-list corruption — it can skip or duplicate entries in
> replay_list, dereference a pointer clobbered mid-update, or trigger a
> list_del corruption BUG() if that's enabled.
>
> What do you think?
>

You're right. Let me fix this.

Thanks
Xiubo

> Thanks,
> Slava.