Re: [PATCH 2/5] ceph: replace the request_tree rbtree with an xarray keyed by r_tid.

Hi Slava,

On Wed, 15 Jul 2026 at 02:35, Viacheslav Dubeyko <[email protected]> wrote:
>
> On Mon, 2026-07-13 at 17:46 +0800, Xiubo Li via B4 Relay wrote:
> > From: Xiubo Li <[email protected]>
> >
[......]
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -1183,7 +1183,6 @@ void ceph_mdsc_release_request(struct kref
> > *kref)
> >       kmem_cache_free(ceph_mds_request_cachep, req);
> >  }
> >
> > -DEFINE_RB_FUNCS(request, struct ceph_mds_request, r_tid, r_node)
> >
> >  /*
> >   * lookup session, bump ref if found.
> > @@ -1195,7 +1194,7 @@ lookup_get_request(struct ceph_mds_client
> > *mdsc, u64 tid)
> >  {
> >       struct ceph_mds_request *req;
> >
> > -     req = lookup_request(&mdsc->request_tree, tid);
> > +     req = xa_load(&mdsc->request_tree, tid);
> >       if (req)
> >               ceph_mdsc_get_request(req);
> >
> > @@ -1229,7 +1228,7 @@ static void __register_request(struct
> > ceph_mds_client *mdsc,
> >       }
> >       doutc(cl, "%p tid %lld\n", req, req->r_tid);
> >       ceph_mdsc_get_request(req);
> > -     insert_request(&mdsc->request_tree, req);
> > +     xa_store(&mdsc->request_tree, req->r_tid, req, GFP_NOFS);
>
> Should we check the return value of xa_store()? What about this?
>
>         err = xa_err(xa_store(&mdsc->request_tree, req->r_tid, req,
> GFP_NOFS));
>         if (err) {
>                 ceph_mdsc_put_request(req);
>                 return err;
>         }
>

Good Catch. Let me fix it.

> >
> >       req->r_cred = get_current_cred();
> >       if (!req->r_mnt_idmap)
> > @@ -1259,20 +1258,20 @@ static void __unregister_request(struct
> > ceph_mds_client *mdsc,
> >       list_del_init(&req->r_unsafe_item);
> >

[......]

> > @@ -534,7 +533,7 @@ struct ceph_mds_client {
> >       u64                    last_tid;      /* most recent mds
> > request */
> >       atomic64_t             oldest_tid;    /* oldest incomplete
> > mds request,
> >                                                excluding
> > setfilelock requests */
> > -     struct rb_root         request_tree;  /* pending mds
> > requests */
> > +     struct xarray           request_tree;  /* pending mds
> > requests */
>
> Transaction IDs are u64; xarray indices are unsigned long. req->r_tid,
> mdsc->last_tid, and want_tid/last_tid in the drain paths are all u64,
> but xa_store(), xa_load(), xa_erase(), and xa_find()'s max parameter
> all take unsigned long. CEPH_FS in fs/ceph/Kconfig only depends on INET
> — it's not gated on 64BIT — so this driver still builds for 32-bit
> targets, where unsigned long truncates r_tid to 32 bits. Are we safe
> here?
>

Yeah, you are right. We have two options to fix it. Just add
"depends on 64BIT" in Kconfig or fall back to the rbtree code on
32-bit with an #if BITS_PER_LONG == 64 guard, similar to what we
already do for s_delegated_inos. I went with the #if guard in v2
because it keeps the driver buildable on 32-bit without introducing
a regression — the rbtree handles u64 keys correctly there. The
downside is a few extra #if/#else/#endif blocks, but they are
mostly confined to the iteration headers; the loop bodies are
shared. Only flush_mdlog_and_wait_mdsc_unsafe_requests() needs a
full alternate implementation because the two control flows are
fundamentally different.

What do you prefer?

Thanks
Xiubo Li