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

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 6/6] xfs: refactor all the EFI/EFD log item sizeof logic
Date: Wed, 26 Oct 2022 09:22:40 +1100	[thread overview]
Message-ID: <20221025222240.GH3600936@dread.disaster.area> (raw)
In-Reply-To: <Y1heWlHlPLaxmDQe@magnolia>

On Tue, Oct 25, 2022 at 03:08:26PM -0700, Darrick J. Wong wrote:
> 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.

FWIW, I just went back an looked at the efip allocation and you did
the same thing there, I just didn't notice it on the first pass....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com