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.
> Everything else looks ok, so with the above fixed up
>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
Thanks!
--D
>
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
