[PATCH] fs/ceph/mds_client: drop mdsc->mutex before decoding the MDS reply

[PATCH] fs/ceph/mds_client: drop mdsc->mutex before decoding the MDS reply

[Max Kellermann]

handle_reply() held `mdsc->mutex` across parse_reply_info(),
i.e. across the full decode of the reply message.  For large replies
(a big readdir allocates and parses many dir_entries), this can take a
while and blocks ceph_mdsc_submit_request() calls meanwhile.

The decode does not need `mdsc->mutex`: parse_reply_info() mostly
fills the request's `r_reply_info`.  Create replies may also add
delegated inode numbers to the session xarray, but that xarray is
protected by its own lock and is not serialized by `mdsc->mutex`
today.  By the time we reach parse_reply_info(), all
`mdsc->mutex`-protected state has already been updated under the lock
(the request has either been unregistered (safe reply) or added to the
session's unsafe list (unsafe reply)) and the request is pinned by the
reference taken in lookup_get_request().

Drop `mdsc->mutex` before calling parse_reply_info() so reply decoding
no longer blocks request submission.  This only widens the existing
unlocked window that already covers the heavier ceph_fill_trace() /
ceph_readdir_prepopulate() processing, so no new races are introduced.

Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
---
 fs/ceph/mds_client.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 853bf698b356..bb8570d31208 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -4089,13 +4089,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
 		list_add_tail(&req->r_unsafe_item, &req->r_session->s_unsafe);
 	}
 
+	/*
+	 * Now that all mutex-protected state has been updated above
+	 * (the request has been unregistered or added to the
+	 * session's unsafe list), we can unlock it.
+	 */
+	mutex_unlock(&mdsc->mutex);
+
 	doutc(cl, "tid %lld result %d\n", tid, result);
 	if (test_bit(CEPHFS_FEATURE_REPLY_ENCODING, &session->s_features))
 		err = parse_reply_info(session, msg, req, (u64)-1);
 	else
 		err = parse_reply_info(session, msg, req,
 				       session->s_con.peer_features);
-	mutex_unlock(&mdsc->mutex);
 
 	/* Must find target inode outside of mutexes to avoid deadlocks */
 	rinfo = &req->r_reply_info;
-- 
2.47.3

Re: [PATCH] fs/ceph/mds_client: drop mdsc->mutex before decoding the MDS reply

[Xiubo Li]

Hi Max

Nice catch and nice improvement. LGTM.

Reviewed-by: Xiubo Li <xiubo.li@clyso.com>

Thanks
- Xiubo

Max Kellermann <max.kellermann@ionos.com> 于2026年7月9日周四 04:40写道：
>
> handle_reply() held `mdsc->mutex` across parse_reply_info(),
> i.e. across the full decode of the reply message.  For large replies
> (a big readdir allocates and parses many dir_entries), this can take a
> while and blocks ceph_mdsc_submit_request() calls meanwhile.
>
> The decode does not need `mdsc->mutex`: parse_reply_info() mostly
> fills the request's `r_reply_info`.  Create replies may also add
> delegated inode numbers to the session xarray, but that xarray is
> protected by its own lock and is not serialized by `mdsc->mutex`
> today.  By the time we reach parse_reply_info(), all
> `mdsc->mutex`-protected state has already been updated under the lock
> (the request has either been unregistered (safe reply) or added to the
> session's unsafe list (unsafe reply)) and the request is pinned by the
> reference taken in lookup_get_request().
>
> Drop `mdsc->mutex` before calling parse_reply_info() so reply decoding
> no longer blocks request submission.  This only widens the existing
> unlocked window that already covers the heavier ceph_fill_trace() /
> ceph_readdir_prepopulate() processing, so no new races are introduced.
>
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---
>  fs/ceph/mds_client.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 853bf698b356..bb8570d31208 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4089,13 +4089,19 @@ static void handle_reply(struct ceph_mds_session *session, struct ceph_msg *msg)
>                 list_add_tail(&req->r_unsafe_item, &req->r_session->s_unsafe);
>         }
>
> +       /*
> +        * Now that all mutex-protected state has been updated above
> +        * (the request has been unregistered or added to the
> +        * session's unsafe list), we can unlock it.
> +        */
> +       mutex_unlock(&mdsc->mutex);
> +
>         doutc(cl, "tid %lld result %d\n", tid, result);
>         if (test_bit(CEPHFS_FEATURE_REPLY_ENCODING, &session->s_features))
>                 err = parse_reply_info(session, msg, req, (u64)-1);
>         else
>                 err = parse_reply_info(session, msg, req,
>                                        session->s_con.peer_features);
> -       mutex_unlock(&mdsc->mutex);
>
>         /* Must find target inode outside of mutexes to avoid deadlocks */
>         rinfo = &req->r_reply_info;
> --
> 2.47.3
>
>