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

From: Viacheslav Dubeyko

Date: Wed Jul 15 2026 - 14:57:25 EST

On Wed, 2026-07-15 at 11:58 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <xiubo.li@xxxxxxxxx>
>
> __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@xxxxxxxxx>
> ---
> 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