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

[Xiubo Li]

On Thu, 16 Jul 2026 at 02:53, Viacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
>
> 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.

The commit message is stale — do_request() wrapper was in an earlier
draft and was already dropped. Current code has none.
Let me fix it.

[......]
> > @@ -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?
>

It's fine.

r_attempts is indeed read and written without any lock now, but
the value is monotonic and the read side is purely advisory in
kick_requests() — it decides whether to pull a request off the
wait list and retry it.  The worst case is that kick_requests()
either processes one extra request (which __do_request() will
immediately bail out on because GOT_RESULT or r_err is already
set), or misses one (which the next kick_requests() round will
pick up).  On all architectures Linux supports, an aligned int
read/write is single-instruction atomic, so there is no torn-read
risk.

r_session is a write-once field — set once in __do_request() and
never changed afterwards.  The reader sees either NULL (and skips
the request) or a valid, stable pointer.  No concurrent writes
occur after the initial store.

That said, this is a data race in the formal sense and KCSAN will
flag it.  I'll add READ_ONCE()/WRITE_ONCE() annotations in the
next revision to make the intent explicit and keep the tooling
quiet.

Thanks
Xiubo

> 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