| Message ID | 166664718541.2688790.5847203715269286943.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix various problems with log intent item recovery | expand |
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); > +} These are all just open-coded versions of struct_size(). It's better to use those, instead, as they will never overflow. (They saturate at SIZE_MAX.) i.e. what you proposed here: https://lore.kernel.org/linux-xfs/20210311031745.GT3419940@magnolia/ Otherwise, yeah, looks good. -Kees
On Mon, 2022-10-24 at 14:33 -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> Looks ok to me Reviewed-by: Allison Henderson <allison.henderson@oracle.com> > --- > 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); > +} > + > typedef struct xfs_efi_log_format_32 { > uint16_t efi_type; /* efi log item type > */ > uint16_t efi_size; /* size of this item > */ > @@ -624,6 +632,14 @@ typedef struct xfs_efi_log_format_32 { > xfs_extent_32_t efi_extents[]; /* array of extents > to free */ > } __attribute__((packed)) xfs_efi_log_format_32_t; > > +static inline size_t > +xfs_efi_log_format32_sizeof( > + unsigned int nr) > +{ > + return sizeof(struct xfs_efi_log_format_32) + > + nr * sizeof(struct xfs_extent_32); > +} > + > typedef struct xfs_efi_log_format_64 { > uint16_t efi_type; /* efi log item type > */ > uint16_t efi_size; /* size of this item > */ > @@ -632,6 +648,14 @@ typedef struct xfs_efi_log_format_64 { > xfs_extent_64_t efi_extents[]; /* array of extents > to free */ > } xfs_efi_log_format_64_t; > > +static inline size_t > +xfs_efi_log_format64_sizeof( > + unsigned int nr) > +{ > + return sizeof(struct xfs_efi_log_format_64) + > + nr * sizeof(struct xfs_extent_64); > +} > + > /* > * This is the structure used to lay out an efd log item in the > * log. The efd_extents array is a variable size array whose > @@ -645,6 +669,14 @@ typedef struct xfs_efd_log_format { > xfs_extent_t efd_extents[]; /* array of extents > freed */ > } xfs_efd_log_format_t; > > +static inline size_t > +xfs_efd_log_format_sizeof( > + unsigned int nr) > +{ > + return sizeof(struct xfs_efd_log_format) + > + nr * sizeof(struct xfs_extent); > +} > + > typedef struct xfs_efd_log_format_32 { > uint16_t efd_type; /* efd log item type > */ > uint16_t efd_size; /* size of this item > */ > @@ -653,6 +685,14 @@ typedef struct xfs_efd_log_format_32 { > xfs_extent_32_t efd_extents[]; /* array of extents > freed */ > } __attribute__((packed)) xfs_efd_log_format_32_t; > > +static inline size_t > +xfs_efd_log_format32_sizeof( > + unsigned int nr) > +{ > + return sizeof(struct xfs_efd_log_format_32) + > + nr * sizeof(struct xfs_extent_32); > +} > + > typedef struct xfs_efd_log_format_64 { > uint16_t efd_type; /* efd log item type > */ > uint16_t efd_size; /* size of this item > */ > @@ -661,6 +701,14 @@ typedef struct xfs_efd_log_format_64 { > xfs_extent_64_t efd_extents[]; /* array of extents > freed */ > } xfs_efd_log_format_64_t; > > +static inline size_t > +xfs_efd_log_format64_sizeof( > + unsigned int nr) > +{ > + return sizeof(struct xfs_efd_log_format_64) + > + nr * sizeof(struct xfs_extent_64); > +} > + > /* > * RUI/RUD (reverse mapping) log format definitions > */ > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c > index 466cc5c5cd33..bffb2b91e39a 100644 > --- a/fs/xfs/xfs_extfree_item.c > +++ b/fs/xfs/xfs_extfree_item.c > @@ -66,27 +66,16 @@ xfs_efi_release( > xfs_efi_item_free(efip); > } > > -/* > - * This returns the number of iovecs needed to log the given efi > item. > - * We only need 1 iovec for an efi item. It just logs the > efi_log_format > - * structure. > - */ > -static inline int > -xfs_efi_item_sizeof( > - struct xfs_efi_log_item *efip) > -{ > - return sizeof(struct xfs_efi_log_format) + > - efip->efi_format.efi_nextents * sizeof(xfs_extent_t); > -} > - > STATIC void > xfs_efi_item_size( > struct xfs_log_item *lip, > int *nvecs, > int *nbytes) > { > + struct xfs_efi_log_item *efip = EFI_ITEM(lip); > + > *nvecs += 1; > - *nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip)); > + *nbytes += xfs_efi_log_format_sizeof(efip- > >efi_format.efi_nextents); > } > > /* > @@ -112,7 +101,7 @@ xfs_efi_item_format( > > xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFI_FORMAT, > &efip->efi_format, > - xfs_efi_item_sizeof(efip)); > + xfs_efi_log_format_sizeof(efip- > >efi_format.efi_nextents)); > } > > > @@ -155,13 +144,11 @@ xfs_efi_init( > > { > struct xfs_efi_log_item *efip; > - uint size; > > ASSERT(nextents > 0); > if (nextents > XFS_EFI_MAX_FAST_EXTENTS) { > - size = (uint)(sizeof(struct xfs_efi_log_item) + > - (nextents * sizeof(xfs_extent_t))); > - efip = kmem_zalloc(size, 0); > + efip = kmem_zalloc(xfs_efi_log_item_sizeof(nextents), > + GFP_KERNEL | __GFP_NOFAIL); > } else { > efip = kmem_cache_zalloc(xfs_efi_cache, > GFP_KERNEL | __GFP_NOFAIL); > @@ -188,12 +175,9 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, > xfs_efi_log_format_t *dst_efi_fmt) > { > xfs_efi_log_format_t *src_efi_fmt = buf->i_addr; > uint i; > - uint len = sizeof(xfs_efi_log_format_t) + > - src_efi_fmt->efi_nextents * sizeof(xfs_extent_t); > - uint len32 = sizeof(xfs_efi_log_format_32_t) + > - src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t); > - uint len64 = sizeof(xfs_efi_log_format_64_t) + > - src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t); > + uint len = xfs_efi_log_format_sizeof(src_efi_fmt- > >efi_nextents); > + uint len32 = xfs_efi_log_format32_sizeof(src_efi_fmt- > >efi_nextents); > + uint len64 = xfs_efi_log_format64_sizeof(src_efi_fmt- > >efi_nextents); > > if (buf->i_len == len) { > memcpy(dst_efi_fmt, src_efi_fmt, > @@ -251,27 +235,16 @@ xfs_efd_item_free(struct xfs_efd_log_item > *efdp) > kmem_cache_free(xfs_efd_cache, efdp); > } > > -/* > - * This returns the number of iovecs needed to log the given efd > item. > - * We only need 1 iovec for an efd item. It just logs the > efd_log_format > - * structure. > - */ > -static inline int > -xfs_efd_item_sizeof( > - struct xfs_efd_log_item *efdp) > -{ > - return sizeof(xfs_efd_log_format_t) + > - efdp->efd_format.efd_nextents * sizeof(xfs_extent_t); > -} > - > STATIC void > xfs_efd_item_size( > struct xfs_log_item *lip, > int *nvecs, > int *nbytes) > { > + struct xfs_efd_log_item *efdp = EFD_ITEM(lip); > + > *nvecs += 1; > - *nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip)); > + *nbytes += xfs_efd_log_format_sizeof(efdp- > >efd_format.efd_nextents); > } > > /* > @@ -296,7 +269,7 @@ xfs_efd_item_format( > > xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFD_FORMAT, > &efdp->efd_format, > - xfs_efd_item_sizeof(efdp)); > + xfs_efd_log_format_sizeof(efdp- > >efd_format.efd_nextents)); > } > > /* > @@ -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); > } else { > efdp = kmem_cache_zalloc(xfs_efd_cache, > GFP_KERNEL | __GFP_NOFAIL); > @@ -738,8 +710,7 @@ xlog_recover_efi_commit_pass2( > > efi_formatp = item->ri_buf[0].i_addr; > > - if (item->ri_buf[0].i_len < > - offsetof(struct xfs_efi_log_format, > efi_extents)) { > + if (item->ri_buf[0].i_len < xfs_efi_log_format_sizeof(0)) { > XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log- > >l_mp); > return -EFSCORRUPTED; > } > @@ -782,10 +753,10 @@ xlog_recover_efd_commit_pass2( > struct xfs_efd_log_format *efd_formatp; > > efd_formatp = item->ri_buf[0].i_addr; > - ASSERT((item->ri_buf[0].i_len == > (sizeof(xfs_efd_log_format_32_t) + > - (efd_formatp->efd_nextents * > sizeof(xfs_extent_32_t)))) || > - (item->ri_buf[0].i_len == > (sizeof(xfs_efd_log_format_64_t) + > - (efd_formatp->efd_nextents * > sizeof(xfs_extent_64_t))))); > + ASSERT(item->ri_buf[0].i_len == xfs_efd_log_format32_sizeof( > + efd_formatp- > >efd_nextents) || > + item->ri_buf[0].i_len == xfs_efd_log_format64_sizeof( > + efd_formatp- > >efd_nextents)); > > xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp- > >efd_efi_id); > return 0; > diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h > index 186d0f2137f1..da6a5afa607c 100644 > --- a/fs/xfs/xfs_extfree_item.h > +++ b/fs/xfs/xfs_extfree_item.h > @@ -52,6 +52,14 @@ struct xfs_efi_log_item { > xfs_efi_log_format_t efi_format; > }; > > +static inline size_t > +xfs_efi_log_item_sizeof( > + unsigned int nr) > +{ > + return offsetof(struct xfs_efi_log_item, efi_format) + > + xfs_efi_log_format_sizeof(nr); > +} > + > /* > * This is the "extent free done" log item. It is used to log > * the fact that some extents earlier mentioned in an efi item > @@ -64,6 +72,14 @@ struct xfs_efd_log_item { > xfs_efd_log_format_t efd_format; > }; > > +static inline size_t > +xfs_efd_log_item_sizeof( > + unsigned int nr) > +{ > + return offsetof(struct xfs_efd_log_item, efd_format) + > + xfs_efd_log_format_sizeof(nr); > +} > + > /* > * Max number of extents in fast allocation path. > */ > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 8485e3b37ca0..ee4b429a2f2c 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -2028,18 +2028,14 @@ xfs_init_caches(void) > goto out_destroy_trans_cache; > > xfs_efd_cache = kmem_cache_create("xfs_efd_item", > - (sizeof(struct > xfs_efd_log_item) + > - XFS_EFD_MAX_FAST_EXTENTS * > - sizeof(struct xfs_extent)), > - 0, 0, NULL); > + xfs_efd_log_item_sizeof(XFS_EFD_MAX_FAST_EXTE > NTS), > + 0, 0, NULL); > if (!xfs_efd_cache) > goto out_destroy_buf_item_cache; > > xfs_efi_cache = kmem_cache_create("xfs_efi_item", > - (sizeof(struct > xfs_efi_log_item) + > - XFS_EFI_MAX_FAST_EXTENTS * > - sizeof(struct xfs_extent)), > - 0, 0, NULL); > + xfs_efi_log_item_sizeof(XFS_EFI_MAX_FAST_EXTE > NTS), > + 0, 0, NULL); > if (!xfs_efi_cache) > goto out_destroy_efd_cache; > >
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? Everything else looks ok, so with the above fixed up Reviewed-by: Dave Chinner <dchinner@redhat.com>
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
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.
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); +} + typedef struct xfs_efi_log_format_32 { uint16_t efi_type; /* efi log item type */ uint16_t efi_size; /* size of this item */ @@ -624,6 +632,14 @@ typedef struct xfs_efi_log_format_32 { xfs_extent_32_t efi_extents[]; /* array of extents to free */ } __attribute__((packed)) xfs_efi_log_format_32_t; +static inline size_t +xfs_efi_log_format32_sizeof( + unsigned int nr) +{ + return sizeof(struct xfs_efi_log_format_32) + + nr * sizeof(struct xfs_extent_32); +} + typedef struct xfs_efi_log_format_64 { uint16_t efi_type; /* efi log item type */ uint16_t efi_size; /* size of this item */ @@ -632,6 +648,14 @@ typedef struct xfs_efi_log_format_64 { xfs_extent_64_t efi_extents[]; /* array of extents to free */ } xfs_efi_log_format_64_t; +static inline size_t +xfs_efi_log_format64_sizeof( + unsigned int nr) +{ + return sizeof(struct xfs_efi_log_format_64) + + nr * sizeof(struct xfs_extent_64); +} + /* * This is the structure used to lay out an efd log item in the * log. The efd_extents array is a variable size array whose @@ -645,6 +669,14 @@ typedef struct xfs_efd_log_format { xfs_extent_t efd_extents[]; /* array of extents freed */ } xfs_efd_log_format_t; +static inline size_t +xfs_efd_log_format_sizeof( + unsigned int nr) +{ + return sizeof(struct xfs_efd_log_format) + + nr * sizeof(struct xfs_extent); +} + typedef struct xfs_efd_log_format_32 { uint16_t efd_type; /* efd log item type */ uint16_t efd_size; /* size of this item */ @@ -653,6 +685,14 @@ typedef struct xfs_efd_log_format_32 { xfs_extent_32_t efd_extents[]; /* array of extents freed */ } __attribute__((packed)) xfs_efd_log_format_32_t; +static inline size_t +xfs_efd_log_format32_sizeof( + unsigned int nr) +{ + return sizeof(struct xfs_efd_log_format_32) + + nr * sizeof(struct xfs_extent_32); +} + typedef struct xfs_efd_log_format_64 { uint16_t efd_type; /* efd log item type */ uint16_t efd_size; /* size of this item */ @@ -661,6 +701,14 @@ typedef struct xfs_efd_log_format_64 { xfs_extent_64_t efd_extents[]; /* array of extents freed */ } xfs_efd_log_format_64_t; +static inline size_t +xfs_efd_log_format64_sizeof( + unsigned int nr) +{ + return sizeof(struct xfs_efd_log_format_64) + + nr * sizeof(struct xfs_extent_64); +} + /* * RUI/RUD (reverse mapping) log format definitions */ diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c index 466cc5c5cd33..bffb2b91e39a 100644 --- a/fs/xfs/xfs_extfree_item.c +++ b/fs/xfs/xfs_extfree_item.c @@ -66,27 +66,16 @@ xfs_efi_release( xfs_efi_item_free(efip); } -/* - * This returns the number of iovecs needed to log the given efi item. - * We only need 1 iovec for an efi item. It just logs the efi_log_format - * structure. - */ -static inline int -xfs_efi_item_sizeof( - struct xfs_efi_log_item *efip) -{ - return sizeof(struct xfs_efi_log_format) + - efip->efi_format.efi_nextents * sizeof(xfs_extent_t); -} - STATIC void xfs_efi_item_size( struct xfs_log_item *lip, int *nvecs, int *nbytes) { + struct xfs_efi_log_item *efip = EFI_ITEM(lip); + *nvecs += 1; - *nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip)); + *nbytes += xfs_efi_log_format_sizeof(efip->efi_format.efi_nextents); } /* @@ -112,7 +101,7 @@ xfs_efi_item_format( xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFI_FORMAT, &efip->efi_format, - xfs_efi_item_sizeof(efip)); + xfs_efi_log_format_sizeof(efip->efi_format.efi_nextents)); } @@ -155,13 +144,11 @@ xfs_efi_init( { struct xfs_efi_log_item *efip; - uint size; ASSERT(nextents > 0); if (nextents > XFS_EFI_MAX_FAST_EXTENTS) { - size = (uint)(sizeof(struct xfs_efi_log_item) + - (nextents * sizeof(xfs_extent_t))); - efip = kmem_zalloc(size, 0); + efip = kmem_zalloc(xfs_efi_log_item_sizeof(nextents), + GFP_KERNEL | __GFP_NOFAIL); } else { efip = kmem_cache_zalloc(xfs_efi_cache, GFP_KERNEL | __GFP_NOFAIL); @@ -188,12 +175,9 @@ xfs_efi_copy_format(xfs_log_iovec_t *buf, xfs_efi_log_format_t *dst_efi_fmt) { xfs_efi_log_format_t *src_efi_fmt = buf->i_addr; uint i; - uint len = sizeof(xfs_efi_log_format_t) + - src_efi_fmt->efi_nextents * sizeof(xfs_extent_t); - uint len32 = sizeof(xfs_efi_log_format_32_t) + - src_efi_fmt->efi_nextents * sizeof(xfs_extent_32_t); - uint len64 = sizeof(xfs_efi_log_format_64_t) + - src_efi_fmt->efi_nextents * sizeof(xfs_extent_64_t); + uint len = xfs_efi_log_format_sizeof(src_efi_fmt->efi_nextents); + uint len32 = xfs_efi_log_format32_sizeof(src_efi_fmt->efi_nextents); + uint len64 = xfs_efi_log_format64_sizeof(src_efi_fmt->efi_nextents); if (buf->i_len == len) { memcpy(dst_efi_fmt, src_efi_fmt, @@ -251,27 +235,16 @@ xfs_efd_item_free(struct xfs_efd_log_item *efdp) kmem_cache_free(xfs_efd_cache, efdp); } -/* - * This returns the number of iovecs needed to log the given efd item. - * We only need 1 iovec for an efd item. It just logs the efd_log_format - * structure. - */ -static inline int -xfs_efd_item_sizeof( - struct xfs_efd_log_item *efdp) -{ - return sizeof(xfs_efd_log_format_t) + - efdp->efd_format.efd_nextents * sizeof(xfs_extent_t); -} - STATIC void xfs_efd_item_size( struct xfs_log_item *lip, int *nvecs, int *nbytes) { + struct xfs_efd_log_item *efdp = EFD_ITEM(lip); + *nvecs += 1; - *nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip)); + *nbytes += xfs_efd_log_format_sizeof(efdp->efd_format.efd_nextents); } /* @@ -296,7 +269,7 @@ xfs_efd_item_format( xlog_copy_iovec(lv, &vecp, XLOG_REG_TYPE_EFD_FORMAT, &efdp->efd_format, - xfs_efd_item_sizeof(efdp)); + xfs_efd_log_format_sizeof(efdp->efd_format.efd_nextents)); } /* @@ -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); } else { efdp = kmem_cache_zalloc(xfs_efd_cache, GFP_KERNEL | __GFP_NOFAIL); @@ -738,8 +710,7 @@ xlog_recover_efi_commit_pass2( efi_formatp = item->ri_buf[0].i_addr; - if (item->ri_buf[0].i_len < - offsetof(struct xfs_efi_log_format, efi_extents)) { + if (item->ri_buf[0].i_len < xfs_efi_log_format_sizeof(0)) { XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, log->l_mp); return -EFSCORRUPTED; } @@ -782,10 +753,10 @@ xlog_recover_efd_commit_pass2( struct xfs_efd_log_format *efd_formatp; efd_formatp = item->ri_buf[0].i_addr; - ASSERT((item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_32_t) + - (efd_formatp->efd_nextents * sizeof(xfs_extent_32_t)))) || - (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) + - (efd_formatp->efd_nextents * sizeof(xfs_extent_64_t))))); + ASSERT(item->ri_buf[0].i_len == xfs_efd_log_format32_sizeof( + efd_formatp->efd_nextents) || + item->ri_buf[0].i_len == xfs_efd_log_format64_sizeof( + efd_formatp->efd_nextents)); xlog_recover_release_intent(log, XFS_LI_EFI, efd_formatp->efd_efi_id); return 0; diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h index 186d0f2137f1..da6a5afa607c 100644 --- a/fs/xfs/xfs_extfree_item.h +++ b/fs/xfs/xfs_extfree_item.h @@ -52,6 +52,14 @@ struct xfs_efi_log_item { xfs_efi_log_format_t efi_format; }; +static inline size_t +xfs_efi_log_item_sizeof( + unsigned int nr) +{ + return offsetof(struct xfs_efi_log_item, efi_format) + + xfs_efi_log_format_sizeof(nr); +} + /* * This is the "extent free done" log item. It is used to log * the fact that some extents earlier mentioned in an efi item @@ -64,6 +72,14 @@ struct xfs_efd_log_item { xfs_efd_log_format_t efd_format; }; +static inline size_t +xfs_efd_log_item_sizeof( + unsigned int nr) +{ + return offsetof(struct xfs_efd_log_item, efd_format) + + xfs_efd_log_format_sizeof(nr); +} + /* * Max number of extents in fast allocation path. */ diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 8485e3b37ca0..ee4b429a2f2c 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -2028,18 +2028,14 @@ xfs_init_caches(void) goto out_destroy_trans_cache; xfs_efd_cache = kmem_cache_create("xfs_efd_item", - (sizeof(struct xfs_efd_log_item) + - XFS_EFD_MAX_FAST_EXTENTS * - sizeof(struct xfs_extent)), - 0, 0, NULL); + xfs_efd_log_item_sizeof(XFS_EFD_MAX_FAST_EXTENTS), + 0, 0, NULL); if (!xfs_efd_cache) goto out_destroy_buf_item_cache; xfs_efi_cache = kmem_cache_create("xfs_efi_item", - (sizeof(struct xfs_efi_log_item) + - XFS_EFI_MAX_FAST_EXTENTS * - sizeof(struct xfs_extent)), - 0, 0, NULL); + xfs_efi_log_item_sizeof(XFS_EFI_MAX_FAST_EXTENTS), + 0, 0, NULL); if (!xfs_efi_cache) goto out_destroy_efd_cache;