On Mon, Sep 18, 2023 at 10:20 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
>
> On 9/18/23 16:04, Ilya Dryomov wrote:
> > On Sun, Sep 17, 2023 at 9:49 PM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >> 6.5-stable review patch. If anyone has any objections, please let me know.
> >>
> >> ------------------
> >>
> >> From: Xiubo Li <xiubli@xxxxxxxxxx>
> >>
> >> [ Upstream commit 3af5ae22030cb59fab4fba35f5a2b62f47e14df9 ]
> >>
> >> In ceph mainline it will allow to set the btime in the setattr request
> >> and just add a 'btime' member in the union 'ceph_mds_request_args' and
> >> then bump up the header version to 4. That means the total size of union
> >> 'ceph_mds_request_args' will increase sizeof(struct ceph_timespec) bytes,
> >> but in kclient it will increase the sizeof(setattr_ext) bytes for each
> >> request.
> >>
> >> Since the MDS will always depend on the header's vesion and front_len
> >> members to decode the 'ceph_mds_request_head' struct, at the same time
> >> kclient hasn't supported the 'btime' feature yet in setattr request,
> >> so it's safe to do this change here.
> >>
> >> This will save 48 bytes memories for each request.
> >>
> >> Fixes: 4f1ddb1ea874 ("ceph: implement updated ceph_mds_request_head structure")
> >> Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> >> Reviewed-by: Milind Changire <mchangir@xxxxxxxxxx>
> >> Signed-off-by: Ilya Dryomov <idryomov@xxxxxxxxx>
> >> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> >> ---
> >> include/linux/ceph/ceph_fs.h | 24 +++++++++++++-----------
> >> 1 file changed, 13 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/include/linux/ceph/ceph_fs.h b/include/linux/ceph/ceph_fs.h
> >> index 49586ff261520..b4fa2a25b7d95 100644
> >> --- a/include/linux/ceph/ceph_fs.h
> >> +++ b/include/linux/ceph/ceph_fs.h
> >> @@ -462,17 +462,19 @@ union ceph_mds_request_args {
> >> } __attribute__ ((packed));
> >>
> >> union ceph_mds_request_args_ext {
> >> - union ceph_mds_request_args old;
> >> - struct {
> >> - __le32 mode;
> >> - __le32 uid;
> >> - __le32 gid;
> >> - struct ceph_timespec mtime;
> >> - struct ceph_timespec atime;
> >> - __le64 size, old_size; /* old_size needed by truncate */
> >> - __le32 mask; /* CEPH_SETATTR_* */
> >> - struct ceph_timespec btime;
> >> - } __attribute__ ((packed)) setattr_ext;
> >> + union {
> >> + union ceph_mds_request_args old;
> >> + struct {
> >> + __le32 mode;
> >> + __le32 uid;
> >> + __le32 gid;
> >> + struct ceph_timespec mtime;
> >> + struct ceph_timespec atime;
> >> + __le64 size, old_size; /* old_size needed by truncate */
> >> + __le32 mask; /* CEPH_SETATTR_* */
> >> + struct ceph_timespec btime;
> >> + } __attribute__ ((packed)) setattr_ext;
> >> + };
> > Hi Xiubo,
> >
> > I was going to ask whether it makes sense to backport this change, but,
> > after looking at it, the change seems bogus to me even in mainline. You
> > added a union inside siting memory use but ceph_mds_request_args_ext was
> > already a union before:
> >
> > union ceph_mds_request_args_ext {
> > union ceph_mds_request_args old;
> > struct { ... } __attribute__ ((packed)) setattr_ext;
> > }
> >
> > What is being achieved here?
>
> As I remembered there has other changes in this union in the beginning.
> And that patch seems being abandoned and missing this one.
>
> Let's skip backporting this one and in the upstream just revert it.
OK, I will send a revert to ceph-devel list.
Greg, please drop this one from all stable branches.
Thanks,
Ilya
