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

[Viacheslav Dubeyko]

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.