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@xxxxxxxxx>

Thanks
- Xiubo

Max Kellermann <max.kellermann@xxxxxxxxx> 于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@xxxxxxxxx>
> ---
>  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
>
>