Re: [PATCH] ceph: streamline request head structures in MDS client

[Viacheslav Dubeyko]

On Fri, 2025-01-10 at 18:05 +0800, Liang Jie wrote:
> From: Liang Jie <liangjie@xxxxxxxxxxx>
> 
> The existence of the ceph_mds_request_head_old structure in the MDS
> client
> code is no longer required due to improvements in handling different
> MDS
> request header versions. This patch removes the now redundant
> ceph_mds_request_head_old structure and replaces its usage with the
> flexible and extensible ceph_mds_request_head structure.
> 
> Changes include:
> - Modification of find_legacy_request_head to directly cast the
> pointer to
>   ceph_mds_request_head_legacy without going through the old
> structure.
> - Update sizeof calculations in create_request_message to use
> offsetofend
>   for consistency and future-proofing, rather than referencing the
> old
>   structure.
> - Use of the structured ceph_mds_request_head directly instead of the
> old
>   one.
> 
> Additionally, this consolidation normalizes the handling of
> request_head_version v1 to align with versions v2 and v3, leading to
> a
> more consistent and maintainable codebase.
> 
> These changes simplify the codebase and reduce potential confusion
> stemming
> from the existence of an obsolete structure.
> 
> Signed-off-by: Liang Jie <liangjie@xxxxxxxxxxx>
> ---
>  fs/ceph/mds_client.c         | 16 ++++++++--------
>  include/linux/ceph/ceph_fs.h | 14 --------------
>  2 files changed, 8 insertions(+), 22 deletions(-)
> 

Looks good to me. Nice cleanup. 

Reviewed-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>

Thanks,
Slava.

> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 785fe489ef4b..2196e404318c 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -2945,12 +2945,12 @@ static struct ceph_mds_request_head_legacy *
>  find_legacy_request_head(void *p, u64 features)
>  {
>  	bool legacy = !(features & CEPH_FEATURE_FS_BTIME);
> -	struct ceph_mds_request_head_old *ohead;
> +	struct ceph_mds_request_head *head;
>  
>  	if (legacy)
>  		return (struct ceph_mds_request_head_legacy *)p;
> -	ohead = (struct ceph_mds_request_head_old *)p;
> -	return (struct ceph_mds_request_head_legacy *)&ohead-
> >oldest_client_tid;
> +	head = (struct ceph_mds_request_head *)p;
> +	return (struct ceph_mds_request_head_legacy *)&head-
> >oldest_client_tid;
>  }
>  
>  /*
> @@ -3020,7 +3020,7 @@ static struct ceph_msg
> *create_request_message(struct ceph_mds_session *session,
>  	if (legacy)
>  		len = sizeof(struct ceph_mds_request_head_legacy);
>  	else if (request_head_version == 1)
> -		len = sizeof(struct ceph_mds_request_head_old);
> +		len = offsetofend(struct ceph_mds_request_head,
> args);
>  	else if (request_head_version == 2)
>  		len = offsetofend(struct ceph_mds_request_head,
> ext_num_fwd);
>  	else
> @@ -3104,11 +3104,11 @@ static struct ceph_msg
> *create_request_message(struct ceph_mds_session *session,
>  		msg->hdr.version = cpu_to_le16(3);
>  		p = msg->front.iov_base + sizeof(*lhead);
>  	} else if (request_head_version == 1) {
> -		struct ceph_mds_request_head_old *ohead = msg-
> >front.iov_base;
> +		struct ceph_mds_request_head *nhead = msg-
> >front.iov_base;
>  
>  		msg->hdr.version = cpu_to_le16(4);
> -		ohead->version = cpu_to_le16(1);
> -		p = msg->front.iov_base + sizeof(*ohead);
> +		nhead->version = cpu_to_le16(1);
> +		p = msg->front.iov_base + offsetofend(struct
> ceph_mds_request_head, args);
>  	} else if (request_head_version == 2) {
>  		struct ceph_mds_request_head *nhead = msg-
> >front.iov_base;
>  
> @@ -3265,7 +3265,7 @@ static int __prepare_send_request(struct
> ceph_mds_session *session,
>  	 * so we limit to retry at most 256 times.
>  	 */
>  	if (req->r_attempts) {
> -	       old_max_retry = sizeof_field(struct
> ceph_mds_request_head_old,
> +		old_max_retry = sizeof_field(struct
> ceph_mds_request_head,
>  					    num_retry);
>  	       old_max_retry = 1 << (old_max_retry * BITS_PER_BYTE);
>  	       if ((old_version && req->r_attempts >= old_max_retry)
> ||
> diff --git a/include/linux/ceph/ceph_fs.h
> b/include/linux/ceph/ceph_fs.h
> index 2d7d86f0290d..c7f2c63b3bc3 100644
> --- a/include/linux/ceph/ceph_fs.h
> +++ b/include/linux/ceph/ceph_fs.h
> @@ -504,20 +504,6 @@ struct ceph_mds_request_head_legacy {
>  
>  #define CEPH_MDS_REQUEST_HEAD_VERSION  3
>  
> -struct ceph_mds_request_head_old {
> -	__le16 version;                /* struct version */
> -	__le64 oldest_client_tid;
> -	__le32 mdsmap_epoch;           /* on client */
> -	__le32 flags;                  /* CEPH_MDS_FLAG_* */
> -	__u8 num_retry, num_fwd;       /* count retry, fwd attempts
> */
> -	__le16 num_releases;           /* # include cap/lease
> release records */
> -	__le32 op;                     /* mds op code */
> -	__le32 caller_uid, caller_gid;
> -	__le64 ino;                    /* use this ino for openc,
> mkdir, mknod,
> -					  etc. (if replaying) */
> -	union ceph_mds_request_args_ext args;
> -} __attribute__ ((packed));
> -
>  struct ceph_mds_request_head {
>  	__le16 version;                /* struct version */
>  	__le64 oldest_client_tid;