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

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Oct 26, 2022 at 09:05:43AM +1100, Dave Chinner wrote:
> On Mon, Oct 24, 2022 at 02:33:05PM -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>
> > ---
> >  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?

Oops, yeah.

> Everything else looks ok, so with the above fixed up
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks!

--D

> 
> -- 
> Dave Chinner
> david@fromorbit.com