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

[Xiubo Li]

。

On Thu, 16 Jul 2026 at 03:12, 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>
> >
> > __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 <xiubo.li@xxxxxxxxx>
> > ---
> >  fs/ceph/mds_client.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 44 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index be0dae919d69..c313574def74 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -4744,30 +4744,66 @@ static void replay_unsafe_requests(struct
> > ceph_mds_client *mdsc,
> >  #else
> >       struct rb_node *p;
> >  #endif
> > +     LIST_HEAD(replay_list);
> >
> >       doutc(mdsc->fsc->client, "mds%d\n", session->s_mds);
> >
> > +#if BITS_PER_LONG == 64
> > +     /* 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);
> > +
> > +     /* replay unsafe requests (local list, no mutex needed) */
> > +     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);
> > +     }
> >
> >       /*
> > -      * also re-send old requests when MDS enters reconnect
> > stage. So that MDS
> > -      * can process completed request in clientreplay stage.
> > +      * also re-send old requests when MDS enters reconnect
> > stage.
> > +      * xa_for_each() is internally locked.
> >        */
> > -#if BITS_PER_LONG == 64
> >       idx = 0;
> >       xa_for_each(&mdsc->request_tree, idx, req) {
> > -#else
> > +             if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> > >r_req_flags))
> > +                     continue;
> > +             if (req->r_attempts == 0)
> > +                     continue;
> > +             if (!req->r_session)
> > +                     continue;
> > +             if (req->r_session->s_mds != session->s_mds)
> > +                     continue;
> > +
> > +             ceph_mdsc_release_dir_caps_async(req);
> > +
> > +             ceph_mdsc_get_request(req);
> > +             __send_request(session, req, true);
> > +             ceph_mdsc_put_request(req);
> > +     }
> > +#else /* BITS_PER_LONG != 64 — keep mutex for rb-tree iteration */
> > +     mutex_lock(&mdsc->mutex);
> > +     list_for_each_entry_safe(req, nreq, &session->s_unsafe,
> > +                              r_unsafe_item)
> > +             __send_request(session, req, true);
> > +
> > +     /*
> > +      * also re-send old requests when MDS enters reconnect
> > stage.
> > +      * Must hold mutex for rb_first()/rb_next().
> > +      */
> >       p = rb_first(&mdsc->request_tree);
> >       while (p) {
> >               req = rb_entry(p, struct ceph_mds_request, r_node);
> >               p = rb_next(p);
> > -#endif
> >               if (test_bit(CEPH_MDS_R_GOT_UNSAFE, &req-
> > >r_req_flags))
> >                       continue;
> >               if (req->r_attempts == 0)
> > -                     continue; /* only old requests */
> > +                     continue;
> >               if (!req->r_session)
> >                       continue;
> >               if (req->r_session->s_mds != session->s_mds)
> > @@ -4778,6 +4814,7 @@ static void replay_unsafe_requests(struct
> > ceph_mds_client *mdsc,
> >               __send_request(session, req, true);
> >       }
> >       mutex_unlock(&mdsc->mutex);
> > +#endif
> >  }
> >
> >  static int send_reconnect_partial(struct ceph_reconnect_state
> > *recon_state)
>
> I believe we have problem in the patch.
>
> Once mdsc->mutex is dropped in the new code, a concurrent "safe" reply
> for one of the unsafe requests currently sitting on the private
> replay_list can call __unregister_request() → list_del_init(&req-
> >r_unsafe_item) on a node that Thread A (running
> replay_unsafe_requests()) is simultaneously iterating and unlinking via
> its own unlocked list_for_each_entry_safe(...) { ...;
> list_del_init(&req->r_unsafe_item); ... }.
>
> Two threads mutating the same prev/next pointers with no shared lock is
> a linked-list corruption — it can skip or duplicate entries in
> replay_list, dereference a pointer clobbered mid-update, or trigger a
> list_del corruption BUG() if that's enabled.
>
> What do you think?
>

You're right. Let me fix this.

Thanks
Xiubo

> Thanks,
> Slava.