Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic

[Allison Henderson <allison.henderson@oracle.com>]

On Mon, 2022-10-24 at 14:33 -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Refactor all the open-coded sizeof logic for EFI/EFD log item and log
> format structures into common helper functions whose names reflect
> the
> struct names.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks ok to me
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |   48 ++++++++++++++++++++++++++++
>  fs/xfs/xfs_extfree_item.c      |   69 ++++++++++++------------------
> ----------
>  fs/xfs/xfs_extfree_item.h      |   16 +++++++++
>  fs/xfs/xfs_super.c             |   12 ++-----
>  4 files changed, 88 insertions(+), 57 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h
> b/fs/xfs/libxfs/xfs_log_format.h
> index 2f41fa8477c9..f13e0809dc63 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -616,6 +616,14 @@ typedef struct xfs_efi_log_format {
>         xfs_extent_t            efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_t;
>  
> +static inline size_t
> +xfs_efi_log_format_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format) +
> +                       nr * sizeof(struct xfs_extent);
> +}
> +
>  typedef struct xfs_efi_log_format_32 {
>         uint16_t                efi_type;       /* efi log item type
> */
>         uint16_t                efi_size;       /* size of this item
> */
> @@ -624,6 +632,14 @@ typedef struct xfs_efi_log_format_32 {
>         xfs_extent_32_t         efi_extents[];  /* array of extents
> to free */
>  } __attribute__((packed)) xfs_efi_log_format_32_t;
>  
> +static inline size_t
> +xfs_efi_log_format32_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format_32) +
> +                       nr * sizeof(struct xfs_extent_32);
> +}
> +
>  typedef struct xfs_efi_log_format_64 {
>         uint16_t                efi_type;       /* efi log item type
> */
>         uint16_t                efi_size;       /* size of this item
> */
> @@ -632,6 +648,14 @@ typedef struct xfs_efi_log_format_64 {
>         xfs_extent_64_t         efi_extents[];  /* array of extents
> to free */
>  } xfs_efi_log_format_64_t;
>  
> +static inline size_t
> +xfs_efi_log_format64_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efi_log_format_64) +
> +                       nr * sizeof(struct xfs_extent_64);
> +}
> +
>  /*
>   * This is the structure used to lay out an efd log item in the
>   * log.  The efd_extents array is a variable size array whose
> @@ -645,6 +669,14 @@ typedef struct xfs_efd_log_format {
>         xfs_extent_t            efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_t;
>  
> +static inline size_t
> +xfs_efd_log_format_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format) +
> +                       nr * sizeof(struct xfs_extent);
> +}
> +
>  typedef struct xfs_efd_log_format_32 {
>         uint16_t                efd_type;       /* efd log item type
> */
>         uint16_t                efd_size;       /* size of this item
> */
> @@ -653,6 +685,14 @@ typedef struct xfs_efd_log_format_32 {
>         xfs_extent_32_t         efd_extents[];  /* array of extents
> freed */
>  } __attribute__((packed)) xfs_efd_log_format_32_t;
>  
> +static inline size_t
> +xfs_efd_log_format32_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format_32) +
> +                       nr * sizeof(struct xfs_extent_32);
> +}
> +
>  typedef struct xfs_efd_log_format_64 {
>         uint16_t                efd_type;       /* efd log item type
> */
>         uint16_t                efd_size;       /* size of this item
> */
> @@ -661,6 +701,14 @@ typedef struct xfs_efd_log_format_64 {
>         xfs_extent_64_t         efd_extents[];  /* array of extents
> freed */
>  } xfs_efd_log_format_64_t;
>  
> +static inline size_t
> +xfs_efd_log_format64_sizeof(
> +       unsigned int            nr)
> +{
> +       return sizeof(struct xfs_efd_log_format_64) +
> +                       nr * sizeof(struct xfs_extent_64);
> +}
> +
>  /*
>   * RUI/RUD (reverse mapping) log format definitions
>   */
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 466cc5c5cd33..bffb2b91e39a 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -66,27 +66,16 @@ xfs_efi_release(
>         xfs_efi_item_free(efip);
>  }
>  
> -/*
> - * This returns the number of iovecs needed to log the given efi
> item.
> - * We only need 1 iovec for an efi item.  It just logs the
> efi_log_format
> - * structure.
> - */
> -static inline int
> -xfs_efi_item_sizeof(
> -       struct xfs_efi_log_item *efip)
> -{
> -       return sizeof(struct xfs_efi_log_format) +
> -              efip->efi_format.efi_nextents * sizeof(xfs_extent_t);
> -}
> -
>  STATIC void
>  xfs_efi_item_size(
>         struct xfs_log_item     *lip,
>         int                     *nvecs,
>         int                     *nbytes)
>  {
> +       struct xfs_efi_log_item *efip = EFI_ITEM(lip);
> +
>         *nvecs += 1;
> -       *nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip));
> +       *nbytes += xfs_efi_log_format_sizeof(efip-
> >efi_format.efi_nextents);
>  }
>  
>  /*
> @@ -112,7 +101,7 @@ xfs_efi_item_format(
>  
>         xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFI_FORMAT,
>                         &efip->efi_format,
> -                       xfs_efi_item_sizeof(efip));
> +                       xfs_efi_log_format_sizeof(efip-
> >efi_format.efi_nextents));
>  }
>  
>  
> @@ -155,13 +144,11 @@ xfs_efi_init(
>  
>  {
>         struct xfs_efi_log_item *efip;
> -       uint                    size;
>  
>         ASSERT(nextents > 0);
>         if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
> -               size = (uint)(sizeof(struct xfs_efi_log_item) +
> -                       (nextents * sizeof(xfs_extent_t)));
> -               efip = kmem_zalloc(size, 0);
> +               efip = kmem_zalloc(xfs_efi_log_item_sizeof(nextents),
> +                               GFP_KERNEL | __GFP_NOFAIL);
>         } else {
>                 efip = kmem_cache_zalloc(xfs_efi_cache,
>                                          GFP_KERNEL | __GFP_NOFAIL);
> @@ -188,12 +175,9 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf,
> xfs_efi_log_format_t *dst_efi_fmt)
>  {
>         xfs_efi_log_format_t *src_efi_fmt = buf->i_addr;
>         uint i;
> -       uint len = sizeof(xfs_efi_log_format_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_t);
> -       uint len32 = sizeof(xfs_efi_log_format_32_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t);
> -       uint len64 = sizeof(xfs_efi_log_format_64_t) +
> -               src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t);
> +       uint len = xfs_efi_log_format_sizeof(src_efi_fmt-
> >efi_nextents);
> +       uint len32 = xfs_efi_log_format32_sizeof(src_efi_fmt-
> >efi_nextents);
> +       uint len64 = xfs_efi_log_format64_sizeof(src_efi_fmt-
> >efi_nextents);
>  
>         if (buf->i_len == len) {
>                 memcpy(dst_efi_fmt, src_efi_fmt,
> @@ -251,27 +235,16 @@ xfs_efd_item_free(struct xfs_efd_log_item
> *efdp)
>                 kmem_cache_free(xfs_efd_cache, efdp);
>  }
>  
> -/*
> - * This returns the number of iovecs needed to log the given efd
> item.
> - * We only need 1 iovec for an efd item.  It just logs the
> efd_log_format
> - * structure.
> - */
> -static inline int
> -xfs_efd_item_sizeof(
> -       struct xfs_efd_log_item *efdp)
> -{
> -       return sizeof(xfs_efd_log_format_t) +
> -              efdp->efd_format.efd_nextents * sizeof(xfs_extent_t);
> -}
> -
>  STATIC void
>  xfs_efd_item_size(
>         struct xfs_log_item     *lip,
>         int                     *nvecs,
>         int                     *nbytes)
>  {
> +       struct xfs_efd_log_item *efdp = EFD_ITEM(lip);
> +
>         *nvecs += 1;
> -       *nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip));
> +       *nbytes += xfs_efd_log_format_sizeof(efdp-
> >efd_format.efd_nextents);
>  }
>  
>  /*
> @@ -296,7 +269,7 @@ xfs_efd_item_format(
>  
>         xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFD_FORMAT,
>                         &efdp->efd_format,
> -                       xfs_efd_item_sizeof(efdp));
> +                       xfs_efd_log_format_sizeof(efdp-
> >efd_format.efd_nextents));
>  }
>  
>  /*
> @@ -345,9 +318,8 @@ xfs_trans_get_efd(
>         ASSERT(nextents > 0);
>  
>         if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> -               efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> -                               nextents * sizeof(struct xfs_extent),
> -                               0);
> +               efdp = kmem_zalloc(xfs_efd_log_item_sizeof(nextents),
> +                               GFP_KERNEL | __GFP_NOFAIL);
>         } else {
>                 efdp = kmem_cache_zalloc(xfs_efd_cache,
>                                         GFP_KERNEL | __GFP_NOFAIL);
> @@ -738,8 +710,7 @@ xlog_recover_efi_commit_pass2(
>  
>         efi_formatp = item->ri_buf[0].i_addr;
>  
> -       if (item->ri_buf[0].i_len <
> -                       offsetof(struct xfs_efi_log_format,
> efi_extents)) {
> +       if (item->ri_buf[0].i_len < xfs_efi_log_format_sizeof(0)) {
>                 XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log-
> >l_mp);
>                 return -EFSCORRUPTED;
>         }
> @@ -782,10 +753,10 @@ xlog_recover_efd_commit_pass2(
>         struct xfs_efd_log_format       *efd_formatp;
>  
>         efd_formatp = item->ri_buf[0].i_addr;
> -       ASSERT((item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_32_t) +
> -               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_32_t)))) ||
> -              (item->ri_buf[0].i_len ==
> (sizeof(xfs_efd_log_format_64_t) +
> -               (efd_formatp->efd_nextents *
> sizeof(xfs_extent_64_t)))));
> +       ASSERT(item->ri_buf[0].i_len == xfs_efd_log_format32_sizeof(
> +                                               efd_formatp-
> >efd_nextents) ||
> +              item->ri_buf[0].i_len == xfs_efd_log_format64_sizeof(
> +                                               efd_formatp-
> >efd_nextents));
>  
>         xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp-
> >efd_efi_id);
>         return 0;
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 186d0f2137f1..da6a5afa607c 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -52,6 +52,14 @@ struct xfs_efi_log_item {
>         xfs_efi_log_format_t    efi_format;
>  };
>  
> +static inline size_t
> +xfs_efi_log_item_sizeof(
> +       unsigned int            nr)
> +{
> +       return offsetof(struct xfs_efi_log_item, efi_format) +
> +                       xfs_efi_log_format_sizeof(nr);
> +}
> +
>  /*
>   * This is the "extent free done" log item.  It is used to log
>   * the fact that some extents earlier mentioned in an efi item
> @@ -64,6 +72,14 @@ struct xfs_efd_log_item {
>         xfs_efd_log_format_t    efd_format;
>  };
>  
> +static inline size_t
> +xfs_efd_log_item_sizeof(
> +       unsigned int            nr)
> +{
> +       return offsetof(struct xfs_efd_log_item, efd_format) +
> +                       xfs_efd_log_format_sizeof(nr);
> +}
> +
>  /*
>   * Max number of extents in fast allocation path.
>   */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 8485e3b37ca0..ee4b429a2f2c 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2028,18 +2028,14 @@ xfs_init_caches(void)
>                 goto out_destroy_trans_cache;
>  
>         xfs_efd_cache = kmem_cache_create("xfs_efd_item",
> -                                       (sizeof(struct
> xfs_efd_log_item) +
> -                                       XFS_EFD_MAX_FAST_EXTENTS *
> -                                       sizeof(struct xfs_extent)),
> -                                       0, 0, NULL);
> +                       xfs_efd_log_item_sizeof(XFS_EFD_MAX_FAST_EXTE
> NTS),
> +                       0, 0, NULL);
>         if (!xfs_efd_cache)
>                 goto out_destroy_buf_item_cache;
>  
>         xfs_efi_cache = kmem_cache_create("xfs_efi_item",
> -                                        (sizeof(struct
> xfs_efi_log_item) +
> -                                        XFS_EFI_MAX_FAST_EXTENTS *
> -                                        sizeof(struct xfs_extent)),
> -                                        0, 0, NULL);
> +                       xfs_efi_log_item_sizeof(XFS_EFI_MAX_FAST_EXTE
> NTS),
> +                       0, 0, NULL);
>         if (!xfs_efi_cache)
>                 goto out_destroy_efd_cache;
>  
>