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

[David Laight <david.laight.linux@gmail.com>]

On Mon, 13 Jul 2026 17:46:07 +0800
Xiubo Li via B4 Relay <devnull+xiubo.li.clyso.com@kernel.org> wrote:

> From: Xiubo Li <xiubo.li@clyso.com>
> 
> 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.

	David

> 
> Signed-off-by: Xiubo Li <xiubo.li@clyso.com>
> ---
>  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 */
>