Re: [PATCH 6.5 113/285] ceph: make members in struct ceph_mds_request_args_ext a union

[Ilya Dryomov <idryomov@xxxxxxxxx>]

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?

    union ceph_mds_request_args_ext {
        union {
            union ceph_mds_request_args old;
            struct { ... } __attribute__ ((packed)) setattr_ext;
        };
    }

Thanks,

                Ilya