On Mon, Sep 18, 2023 at 10:43:08AM +0200, Ilya Dryomov wrote:
> 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.
Now dropped, thanks.
greg k-h
