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

[Xiubo Li]

As I know there is no. Yeah, with this option it will quit simple and
easier to maintain then.

Thanks
Xiubo

On Thu, 16 Jul 2026 at 01:56, Viacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
>
> On Wed, 2026-07-15 at 10:21 +0800, Xiubo Li wrote:
> > Hi Slava,
> >
> > On Wed, 15 Jul 2026 at 02:35, Viacheslav Dubeyko <slava@xxxxxxxxxxx>
> > wrote:
> > >
> > > On Mon, 2026-07-13 at 17:46 +0800, Xiubo Li via B4 Relay wrote:
> > > > From: Xiubo Li <xiubo.li@xxxxxxxxx>
> > > >
> > [......]
> > > > --- 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?
> >
> >
>
> Do we know any customer that uses 32-bit builds? If there is no such
> users, then we can restrict the builds by "depends on 64BIT" in
> Kconfig. Otherwise, we have to provide opportunity to execute 32-bit
> builds by proper supporting rbtree for this case. But, frankly
> speaking, I would like to prefer simple solution like "depends on
> 64BIT".
>
> Thanks,
> Slava.