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

On Mon, 2026-07-13 at 17:46 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <[email protected]>
> 
> __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 <[email protected]>
> ---
>  fs/ceph/mds_client.c | 45 +++++++++++++++++++++++++-----------------
> ---
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 6f9049148721..ef54d19ace2a 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -3577,9 +3577,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;
>  	}
>  
> @@ -3612,6 +3615,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 &
> @@ -3637,6 +3641,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;
>  	}
>  
> @@ -3710,6 +3715,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 */
>  
> @@ -3740,6 +3747,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);
> @@ -3777,6 +3785,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:
> @@ -3786,6 +3796,7 @@ static void __do_request(struct ceph_mds_client
> *mdsc,
>  		complete_request(mdsc, req);
>  		__unregister_request(mdsc, req);
>  	}
> +	mutex_unlock(&mdsc->mutex);
>  	return;
>  }
>  
> @@ -3900,10 +3911,11 @@ 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);
>  	__do_request(mdsc, req);
>  	err = req->r_err;
> -	mutex_unlock(&mdsc->mutex);
>  	return err;
>  }
>  
> @@ -4276,13 +4288,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;
>  
> @@ -4606,11 +4619,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);
> @@ -5245,9 +5256,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);
> @@ -5672,8 +5681,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]);
> @@ -5684,9 +5693,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);
> @@ -5802,8 +5809,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);
> @@ -5812,8 +5819,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;
>  		}
>  
> @@ -5857,8 +5864,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);
> @@ -6766,8 +6773,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);
> @@ -6778,11 +6785,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)
> @@ -6924,8 +6931,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);
>  }
>  
>  /*
> @@ -6976,11 +6983,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;
>  
> @@ -7078,8 +7085,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);
> @@ -7088,9 +7095,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;


If I am not wrong, we still have comment [1]:

/*
 * called under mdsc->mutex
 */
static void __wake_requests(struct ceph_mds_client *mdsc,
			    struct list_head *head)

Thanks,
Slava.

[1]
https://elixir.bootlin.com/linux/v7.2-rc3/source/fs/ceph/mds_client.c#L3793