Re: [PATCH 1/5] ceph: convert oldest_tid to atomic64_t

[Xiubo Li]

On Wed, 15 Jul 2026 at 17:55, David Laight <david.laight.linux@xxxxxxxxx> wrote:
>
> On Mon, 13 Jul 2026 17:46:07 +0800
> Xiubo Li via B4 Relay <devnull+xiubo.li.clyso.com@xxxxxxxxxx> wrote:
>
> > From: Xiubo Li <xiubo.li@xxxxxxxxx>
> >
> > The oldest_client_tid sent in the MDS request header is advisory:
> > a stale value is harmless -- at worst the MDS may resend a reply
> > we already have, or skip one we still need (which will just be
> > retried).  With the plain u64 read, however, the compiler is free
> > to split or cache the load, which is undefined behaviour when the
> > write side runs under mdsc->mutex on a different CPU.
> >
> > Convert mdsc->oldest_tid from u64 to atomic64_t so that reads
> > are guaranteed to be single-copy atomic on all architectures.
> > This removes the one remaining reason __prepare_send_request()
> > and __send_request() needed to be called under mdsc->mutex, so
> > drop those comments as well.
> >
> > All write sites (__register_request, __unregister_request) still
> > run under mdsc->mutex, so use atomic64_set() for clarity.
>
> If the read side doesn't hold the lock then it makes little difference
> whether the write side holds it.
> It the write is split (eg on 32bit) then even an atomic read could
> get an invalid value.
>
> On 64bit this change probably has no effect.
> But some 32bit architectures cannot do atomic 64bit read/write so
> they are expensive operations.
> x86-32 now requires the 64bit 'compare exchange' that can be used
> for 64bit load/store so they are only moderately expensive not
> stupidly expensive.
>

Agreed — atomic64_read is unnecessarily expensive on 32-bit for
an advisory value, and on 64-bit a plain u64 load/store is already
single-instruction atomic.  Let me dropped the atomic64_t conversion
in v3.

The read side will use READ_ONCE() and the write side WRITE_ONCE(),
which prevents the compiler from splitting or caching the access.
The value is monotonic and advisory: on 32-bit a torn read is
harmless because at worst the MDS trims its completed_requests
table slightly earlier or later than optimal, which the replay
journal rectifies.

Thanks
Xiubo

>         David
>
> >
> > Signed-off-by: Xiubo Li <xiubo.li@xxxxxxxxx>
> > ---
> >  fs/ceph/mds_client.c | 21 ++++++++-------------
> >  fs/ceph/mds_client.h |  2 +-
> >  2 files changed, 9 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 9f84ef2ac6e4..98d0a5baff70 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -1235,8 +1235,9 @@ static void __register_request(struct ceph_mds_client *mdsc,
> >       if (!req->r_mnt_idmap)
> >               req->r_mnt_idmap = &nop_mnt_idmap;
> >
> > -     if (mdsc->oldest_tid == 0 && req->r_op != CEPH_MDS_OP_SETFILELOCK)
> > -             mdsc->oldest_tid = req->r_tid;
> > +     if (atomic64_read(&mdsc->oldest_tid) == 0 &&
> > +         req->r_op != CEPH_MDS_OP_SETFILELOCK)
> > +             atomic64_set(&mdsc->oldest_tid, req->r_tid);
> >
> >       if (dir) {
> >               struct ceph_inode_info *ci = ceph_inode(dir);
> > @@ -1257,14 +1258,14 @@ static void __unregister_request(struct ceph_mds_client *mdsc,
> >       /* Never leave an unregistered request on an unsafe list! */
> >       list_del_init(&req->r_unsafe_item);
> >
> > -     if (req->r_tid == mdsc->oldest_tid) {
> > +     if (req->r_tid == atomic64_read(&mdsc->oldest_tid)) {
> >               struct rb_node *p = rb_next(&req->r_node);
> > -             mdsc->oldest_tid = 0;
> > +             atomic64_set(&mdsc->oldest_tid, 0);
> >               while (p) {
> >                       struct ceph_mds_request *next_req =
> >                               rb_entry(p, struct ceph_mds_request, r_node);
> >                       if (next_req->r_op != CEPH_MDS_OP_SETFILELOCK) {
> > -                             mdsc->oldest_tid = next_req->r_tid;
> > +                             atomic64_set(&mdsc->oldest_tid, next_req->r_tid);
> >                               break;
> >                       }
> >                       p = rb_next(p);
> > @@ -1693,7 +1694,7 @@ create_session_full_msg(struct ceph_mds_client *mdsc, int op, u64 seq)
> >       ceph_encode_32(&p, 0);
> >
> >       /* version == 7, oldest_client_tid */
> > -     ceph_encode_64(&p, mdsc->oldest_tid);
> > +     ceph_encode_64(&p, atomic64_read(&mdsc->oldest_tid));
> >
> >       msg->front.iov_len = p - msg->front.iov_base;
> >       msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> > @@ -2759,7 +2760,7 @@ static struct ceph_mds_request *__get_oldest_req(struct ceph_mds_client *mdsc)
> >
> >  static inline  u64 __get_oldest_tid(struct ceph_mds_client *mdsc)
> >  {
> > -     return mdsc->oldest_tid;
> > +     return atomic64_read(&mdsc->oldest_tid);
> >  }
> >
> >  #if IS_ENABLED(CONFIG_FS_ENCRYPTION)
> > @@ -3438,9 +3439,6 @@ static void complete_request(struct ceph_mds_client *mdsc,
> >       complete_all(&req->r_completion);
> >  }
> >
> > -/*
> > - * called under mdsc->mutex
> > - */
> >  static int __prepare_send_request(struct ceph_mds_session *session,
> >                                 struct ceph_mds_request *req,
> >                                 bool drop_cap_releases)
> > @@ -3555,9 +3553,6 @@ static int __prepare_send_request(struct ceph_mds_session *session,
> >       return 0;
> >  }
> >
> > -/*
> > - * called under mdsc->mutex
> > - */
> >  static int __send_request(struct ceph_mds_session *session,
> >                         struct ceph_mds_request *req,
> >                         bool drop_cap_releases)
> > diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> > index 731d6ad04956..3b614b5df18c 100644
> > --- a/fs/ceph/mds_client.h
> > +++ b/fs/ceph/mds_client.h
> > @@ -532,7 +532,7 @@ struct ceph_mds_client {
> >       spinlock_t              snap_empty_lock;  /* protect snap_empty */
> >
> >       u64                    last_tid;      /* most recent mds request */
> > -     u64                    oldest_tid;    /* oldest incomplete mds request,
> > +     atomic64_t             oldest_tid;    /* oldest incomplete mds request,
> >                                                excluding setfilelock requests */
> >       struct rb_root         request_tree;  /* pending mds requests */
> >       struct delayed_work    delayed_work;  /* delayed work */
> >
>