On Mon, Oct 24, 2022 at 02:33:05PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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);
> +}
FWIW, I'm not really a fan of placing inline code in the on-disk
format definition headers because combining code and type
definitions eventually leads to dependency hell.
I'm going to say it's OK for these functions to be placed here
because they have no external dependencies and are directly related
to the on-disk structures, but I think we need to be careful about
how much code we include into this header as opposed to the type
specific header files (such as fs/xfs/xfs_extfree_item.h)...
> @@ -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);
That looks like a broken kmem->kmalloc conversion. Did you mean to
convert this to allocation to use kzalloc() at the same time?
Everything else looks ok, so with the above fixed up
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
