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@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?
>
> 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@xxxxxxxxxxxxx
