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