Message ID | 20250110100524.1891669-1-buaajxlj@163.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ceph: streamline request head structures in MDS client | expand |
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;
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
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
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
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;