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

On Mon, 2026-07-13 at 17:46 +0800, Xiubo Li via B4 Relay wrote:
> From: Xiubo Li <[email protected]>
> 
> __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 <[email protected]>
> ---
>  fs/ceph/mds_client.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index ef54d19ace2a..6c5a8c651d3e 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -4673,12 +4673,25 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  {
>  	struct ceph_mds_request *req, *nreq;
>  	unsigned long idx;
> +	LIST_HEAD(replay_list);
>  
>  	doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
>  
> +	/* 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);

This loop is done right.

> +
> +	/* replay unsafe requests — __send_request is lockless */
> +	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);
> +	}

However, this one has issue, if I am right. No ceph_mdsc_get_request()
anywhere in it. It has always relied entirely on the caller holding
mdsc->mutex for the duration of the loop.

While this loop is inside __send_request() or
ceph_mdsc_release_dir_caps_async() for one req, nothing stops another
thread from acquiring mdsc->mutex and completing/timing-out/aborting
that same request through the now-fully-unlocked
__do_request()/wait_requests() path, dropping the last reference and
freeing it — at which point the next loop iteration's xa_find() touches
freed memory. Am I right here?

Thanks,
Slava.

>  
>  	/*
>  	 * also re-send old requests when MDS enters reconnect
> stage. So that MDS
> @@ -4699,7 +4712,6 @@ static void replay_unsafe_requests(struct
> ceph_mds_client *mdsc,
>  
>  		__send_request(session, req, true);
>  	}
> -	mutex_unlock(&mdsc->mutex);
>  }
>  
>  static int send_reconnect_partial(struct ceph_reconnect_state
> *recon_state)