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

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

[Liang Jie]

From: Liang Jie <liangjie@lixiang.com>

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@lixiang.com>
---
 fs/ceph/mds_client.c         | 16 ++++++++--------
 include/linux/ceph/ceph_fs.h | 14 --------------
 2 files changed, 8 insertions(+), 22 deletions(-)

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;
-- 
2.25.1

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@lixiang.com>
> 
> 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@lixiang.com>
> ---
>  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@ibm.com>

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;

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

[Ilya Dryomov]

On Fri, Jan 10, 2025 at 8:25 PM Viacheslav Dubeyko
<Slava.Dubeyko@ibm.com> wrote:
>
> On Fri, 2025-01-10 at 18:05 +0800, Liang Jie wrote:
> > From: Liang Jie <liangjie@lixiang.com>
> >
> > 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@lixiang.com>
> > ---
> >  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@ibm.com>

Applied.

Thanks,

                Ilya

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

[Liang Jie]

On Wed, 15 Jan 2025 21:15:11 +0100, Ilya Dryomov <idryomov@gmail.com> wrote:
> On Fri, Jan 10, 2025 at 8:25=E2=80=AFPM Viacheslav Dubeyko
> <Slava.Dubeyko@ibm.com> wrote:
> >
> > On Fri, 2025-01-10 at 18:05 +0800, Liang Jie wrote:
> > > From: Liang Jie <liangjie@lixiang.com>
> > >
> > > 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@lixiang.com>
> > > ---
> > >  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@ibm.com>
> 
> Applied.
> 
> Thanks,
> 
>                 Ilya

Hi Ilya,

I have recently received a warning from the kernel test robot about an indentation
issue adjacent to the changes made in my this patch.

The warning is as follows:
(link: https://lore.kernel.org/oe-kbuild-all/202501172005.IoKVy2Op-lkp@intel.com/)

> New smatch warnings:
> fs/ceph/mds_client.c:3298 __prepare_send_request() warn: inconsistent indenting
> 
> vim +3298 fs/ceph/mds_client.c
> 
> 2f2dc053404feb Sage Weil      2009-10-06  3272  
> ...
> ce0d5bd3a6c176 Xiubo Li       2023-07-25  3295  	if (req->r_attempts) {
> 164b631927701b Liang Jie      2025-01-10  3296  		old_max_retry = sizeof_field(struct ceph_mds_request_head,
> ce0d5bd3a6c176 Xiubo Li       2023-07-25  3297  					    num_retry);
> ce0d5bd3a6c176 Xiubo Li       2023-07-25 @3298  	       old_max_retry = 1 << (old_max_retry * BITS_PER_BYTE);
> ce0d5bd3a6c176 Xiubo Li       2023-07-25  3299  	       if ((old_version && req->r_attempts >= old_max_retry) ||
> ce0d5bd3a6c176 Xiubo Li       2023-07-25  3300  		   ((uint32_t)req->r_attempts >= U32_MAX)) {
> 38d46409c4639a Xiubo Li       2023-06-12  3301  			pr_warn_ratelimited_client(cl, "request tid %llu seq overflow\n",
> 38d46409c4639a Xiubo Li       2023-06-12  3302  						   req->r_tid);
> 546a5d6122faae Xiubo Li       2022-03-30  3303  			return -EMULTIHOP;
> 546a5d6122faae Xiubo Li       2022-03-30  3304  	       }
> ce0d5bd3a6c176 Xiubo Li       2023-07-25  3305  	}

The warning indicates suspect code indent on line 3298, existing before my
proposed changes were made. After investigating the issue, it has become clear
that the warning stems from a pre-existing code block that uses 15 spaces for
indentation instead of conforming to the standard 16-space (two tabs) indentation.

I am writing to seek your advice on how best to proceed. Would you recommend that
I adjust the indentation within the scope of my current patch, or would it be more
appropriate to address this as a separate style fix, given that the indentation
issue is not directly introduced by my changes?

I am happy to make the necessary adjustments to ensure the code base adheres to the
kernel's coding standards. However, I also want to respect the best practices of
contributing to the project and maintain the clarity and focus of my patch.

Kindly advise on the preferred way forward.

Thank you for your time and guidance.

Best regards,
Liang Jie

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

[Ilya Dryomov]

On Sat, Jan 18, 2025 at 2:53 PM Liang Jie <buaajxlj@163.com> wrote:
>
> On Wed, 15 Jan 2025 21:15:11 +0100, Ilya Dryomov <idryomov@gmail.com> wrote:
> > On Fri, Jan 10, 2025 at 8:25=E2=80=AFPM Viacheslav Dubeyko
> > <Slava.Dubeyko@ibm.com> wrote:
> > >
> > > On Fri, 2025-01-10 at 18:05 +0800, Liang Jie wrote:
> > > > From: Liang Jie <liangjie@lixiang.com>
> > > >
> > > > 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@lixiang.com>
> > > > ---
> > > >  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@ibm.com>
> >
> > Applied.
> >
> > Thanks,
> >
> >                 Ilya
>
> Hi Ilya,
>
> I have recently received a warning from the kernel test robot about an indentation
> issue adjacent to the changes made in my this patch.
>
> The warning is as follows:
> (link: https://lore.kernel.org/oe-kbuild-all/202501172005.IoKVy2Op-lkp@intel.com/)
>
> > New smatch warnings:
> > fs/ceph/mds_client.c:3298 __prepare_send_request() warn: inconsistent indenting
> >
> > vim +3298 fs/ceph/mds_client.c
> >
> > 2f2dc053404feb Sage Weil      2009-10-06  3272
> > ...
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25  3295        if (req->r_attempts) {
> > 164b631927701b Liang Jie      2025-01-10  3296                old_max_retry = sizeof_field(struct ceph_mds_request_head,
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25  3297                                            num_retry);
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25 @3298               old_max_retry = 1 << (old_max_retry * BITS_PER_BYTE);
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25  3299               if ((old_version && req->r_attempts >= old_max_retry) ||
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25  3300                   ((uint32_t)req->r_attempts >= U32_MAX)) {
> > 38d46409c4639a Xiubo Li       2023-06-12  3301                        pr_warn_ratelimited_client(cl, "request tid %llu seq overflow\n",
> > 38d46409c4639a Xiubo Li       2023-06-12  3302                                                   req->r_tid);
> > 546a5d6122faae Xiubo Li       2022-03-30  3303                        return -EMULTIHOP;
> > 546a5d6122faae Xiubo Li       2022-03-30  3304               }
> > ce0d5bd3a6c176 Xiubo Li       2023-07-25  3305        }
>
> The warning indicates suspect code indent on line 3298, existing before my
> proposed changes were made. After investigating the issue, it has become clear
> that the warning stems from a pre-existing code block that uses 15 spaces for
> indentation instead of conforming to the standard 16-space (two tabs) indentation.
>
> I am writing to seek your advice on how best to proceed. Would you recommend that
> I adjust the indentation within the scope of my current patch, or would it be more
> appropriate to address this as a separate style fix, given that the indentation
> issue is not directly introduced by my changes?

Hi Liang,

I noticed the indentation mismatch myself and, when applying, edited
your patch to be consistent with the pre-existing block (it's actually
a tab + 7 spaces, no idea why):

https://github.com/ceph/ceph-client/commit/929fba81687e4ba6ed9af18fc1f34e76ac90772a

It looks like this warning was generated based on the patch as posted,
not as applied, so it can be ignored.

>
> I am happy to make the necessary adjustments to ensure the code base adheres to the
> kernel's coding standards. However, I also want to respect the best practices of
> contributing to the project and maintain the clarity and focus of my patch.
>
> Kindly advise on the preferred way forward.
>
> Thank you for your time and guidance.

Thank you for being thorough!

                Ilya