cleanup the EFI/EFD definitions

cleanup the EFI/EFD definitions

[Christoph Hellwig]

Hi all,

this series cleans up how we define the on-disk EFI and EFD items.  The
last patch is a rebased version of the patch from Gustavo to remove the
one item arrays in these formats.

Diffstat
 libxfs/xfs_log_format.h |   69 ++++++---------------
 xfs_extfree_item.c      |  157 ++++++++++++++++++------------------------------
 xfs_extfree_item.h      |   16 ++++
 xfs_ondisk.h            |    9 +-
 xfs_super.c             |   12 +--
 5 files changed, 103 insertions(+), 160 deletions(-)

[PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2

[Christoph Hellwig]

We never actually look at the extent array in the efd items, and should
eventually stop writing them out at all when it is time for a incompat
log change.  Ѕo don't bother with the asserts at all, and thus with the
the structures defined just to be used with it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_log_format.h | 20 ++------------------
 fs/xfs/xfs_extfree_item.c      | 10 ++--------
 fs/xfs/xfs_extfree_item.h      |  2 +-
 fs/xfs/xfs_ondisk.h            |  2 --
 4 files changed, 5 insertions(+), 29 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 8bd00da6d2a40f..ea0fe9f121adff 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -598,29 +598,13 @@ typedef struct xfs_efi_log_format_64 {
  * log.  The efd_extents array is a variable size array whose
  * size is given by efd_nextents;
  */
-typedef struct xfs_efd_log_format {
+struct xfs_efd_log_format {
 	uint16_t		efd_type;	/* efd log item type */
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
 	xfs_extent_t		efd_extents[1];	/* array of extents freed */
-} xfs_efd_log_format_t;
-
-typedef struct xfs_efd_log_format_32 {
-	uint16_t		efd_type;	/* efd log item type */
-	uint16_t		efd_size;	/* size of this item */
-	uint32_t		efd_nextents;	/* # of extents freed */
-	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_32_t		efd_extents[1];	/* array of extents freed */
-} __attribute__((packed)) xfs_efd_log_format_32_t;
-
-typedef struct xfs_efd_log_format_64 {
-	uint16_t		efd_type;	/* efd log item type */
-	uint16_t		efd_size;	/* size of this item */
-	uint32_t		efd_nextents;	/* # of extents freed */
-	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_64_t		efd_extents[1];	/* array of extents freed */
-} xfs_efd_log_format_64_t;
+};
 
 /*
  * RUI/RUD (reverse mapping) log format definitions
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 93223ebb33721e..ac17fdb9283489 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -253,7 +253,7 @@ static inline int
 xfs_efd_item_sizeof(
 	struct xfs_efd_log_item *efdp)
 {
-	return sizeof(xfs_efd_log_format_t) +
+	return sizeof(struct xfs_efd_log_format) +
 	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
 }
 
@@ -743,13 +743,7 @@ xlog_recover_efd_commit_pass2(
 	struct xlog_recover_item	*item,
 	xfs_lsn_t			lsn)
 {
-	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 - 1) * sizeof(xfs_extent_32_t)))) ||
-	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
-		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t)))));
+	struct xfs_efd_log_format	*efd_formatp = item->ri_buf[0].i_addr;
 
 	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 cd2860c875bf50..6b80452ad2a71b 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -61,7 +61,7 @@ struct xfs_efd_log_item {
 	struct xfs_log_item	efd_item;
 	struct xfs_efi_log_item *efd_efip;
 	uint			efd_next_extent;
-	xfs_efd_log_format_t	efd_format;
+	struct xfs_efd_log_format efd_format;
 };
 
 /*
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 0aa87c2101049c..7328ff92e0ee8a 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -118,8 +118,6 @@ xfs_check_ondisk_structs(void)
 	/* log structures */
 	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,	28);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,	32);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	32);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
-- 
2.30.1

Re: [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:27:58AM +0200, Christoph Hellwig wrote:
> We never actually look at the extent array in the efd items, and should
> eventually stop writing them out at all when it is time for a incompat
> log change.  Ѕo don't bother with the asserts at all, and thus with the
> the structures defined just to be used with it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems reasonable enough...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_log_format.h | 20 ++------------------
>  fs/xfs/xfs_extfree_item.c      | 10 ++--------
>  fs/xfs/xfs_extfree_item.h      |  2 +-
>  fs/xfs/xfs_ondisk.h            |  2 --
>  4 files changed, 5 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 8bd00da6d2a40f..ea0fe9f121adff 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -598,29 +598,13 @@ typedef struct xfs_efi_log_format_64 {
>   * log.  The efd_extents array is a variable size array whose
>   * size is given by efd_nextents;
>   */
> -typedef struct xfs_efd_log_format {
> +struct xfs_efd_log_format {
>  	uint16_t		efd_type;	/* efd log item type */
>  	uint16_t		efd_size;	/* size of this item */
>  	uint32_t		efd_nextents;	/* # of extents freed */
>  	uint64_t		efd_efi_id;	/* id of corresponding efi */
>  	xfs_extent_t		efd_extents[1];	/* array of extents freed */
> -} xfs_efd_log_format_t;
> -
> -typedef struct xfs_efd_log_format_32 {
> -	uint16_t		efd_type;	/* efd log item type */
> -	uint16_t		efd_size;	/* size of this item */
> -	uint32_t		efd_nextents;	/* # of extents freed */
> -	uint64_t		efd_efi_id;	/* id of corresponding efi */
> -	xfs_extent_32_t		efd_extents[1];	/* array of extents freed */
> -} __attribute__((packed)) xfs_efd_log_format_32_t;
> -
> -typedef struct xfs_efd_log_format_64 {
> -	uint16_t		efd_type;	/* efd log item type */
> -	uint16_t		efd_size;	/* size of this item */
> -	uint32_t		efd_nextents;	/* # of extents freed */
> -	uint64_t		efd_efi_id;	/* id of corresponding efi */
> -	xfs_extent_64_t		efd_extents[1];	/* array of extents freed */
> -} xfs_efd_log_format_64_t;
> +};
>  
>  /*
>   * RUI/RUD (reverse mapping) log format definitions
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 93223ebb33721e..ac17fdb9283489 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -253,7 +253,7 @@ static inline int
>  xfs_efd_item_sizeof(
>  	struct xfs_efd_log_item *efdp)
>  {
> -	return sizeof(xfs_efd_log_format_t) +
> +	return sizeof(struct xfs_efd_log_format) +
>  	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
>  }
>  
> @@ -743,13 +743,7 @@ xlog_recover_efd_commit_pass2(
>  	struct xlog_recover_item	*item,
>  	xfs_lsn_t			lsn)
>  {
> -	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 - 1) * sizeof(xfs_extent_32_t)))) ||
> -	       (item->ri_buf[0].i_len == (sizeof(xfs_efd_log_format_64_t) +
> -		((efd_formatp->efd_nextents - 1) * sizeof(xfs_extent_64_t)))));
> +	struct xfs_efd_log_format	*efd_formatp = item->ri_buf[0].i_addr;
>  
>  	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 cd2860c875bf50..6b80452ad2a71b 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -61,7 +61,7 @@ struct xfs_efd_log_item {
>  	struct xfs_log_item	efd_item;
>  	struct xfs_efi_log_item *efd_efip;
>  	uint			efd_next_extent;
> -	xfs_efd_log_format_t	efd_format;
> +	struct xfs_efd_log_format efd_format;
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 0aa87c2101049c..7328ff92e0ee8a 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -118,8 +118,6 @@ xfs_check_ondisk_structs(void)
>  	/* log structures */
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_32,	28);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format_64,	32);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	32);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
> -- 
> 2.30.1
> 

[PATCH 2/7] xfs: clean up the EFI and EFD log format handling

[Christoph Hellwig]

The extent structure embedded into the EFI and EFD items was originally
defined as a structure with implicit padding, which makes it a mess
to handle between 32-bit and 64-bit kernels as the 32-bit ABI packs them
tight, while the 64-bit ABI implicitly adds an implicit 4-bye padding.
Log recovery has been able to deal with both formats for a long time,
although in a rather messy way where the default definition varies
between the ABIs, but log recovery has two extra special cases for
padded or unpadded variants.

Change this to always write the properly fully padded EFI and EFD
structures to the log, and only special case the unpadded one during
recovery.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_log_format.h |  49 ++++++--------
 fs/xfs/xfs_extfree_item.c      | 115 ++++++++++++++-------------------
 fs/xfs/xfs_extfree_item.h      |   2 +-
 fs/xfs/xfs_ondisk.h            |   5 +-
 4 files changed, 71 insertions(+), 100 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index ea0fe9f121adff..639035052b4f65 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -542,56 +542,43 @@ xfs_blft_from_flags(struct xfs_buf_log_format *blf)
 /*
  * EFI/EFD log format definitions
  */
-typedef struct xfs_extent {
-	xfs_fsblock_t	ext_start;
-	xfs_extlen_t	ext_len;
-} xfs_extent_t;
 
-/*
- * Since an xfs_extent_t has types (start:64, len: 32)
- * there are different alignments on 32 bit and 64 bit kernels.
- * So we provide the different variants for use by a
- * conversion routine.
- */
-typedef struct xfs_extent_32 {
-	uint64_t	ext_start;
-	uint32_t	ext_len;
-} __attribute__((packed)) xfs_extent_32_t;
-
-typedef struct xfs_extent_64 {
+struct xfs_extent {
 	uint64_t	ext_start;
 	uint32_t	ext_len;
 	uint32_t	ext_pad;
-} xfs_extent_64_t;
+};
 
 /*
  * This is the structure used to lay out an efi log item in the
  * log.  The efi_extents field is a variable size array whose
  * size is given by efi_nextents.
  */
-typedef struct xfs_efi_log_format {
+struct xfs_efi_log_format {
 	uint16_t		efi_type;	/* efi log item type */
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_t		efi_extents[1];	/* array of extents to free */
-} xfs_efi_log_format_t;
+	struct xfs_extent	efi_extents[1];	/* array of extents to free */
+};
 
-typedef struct xfs_efi_log_format_32 {
-	uint16_t		efi_type;	/* efi log item type */
-	uint16_t		efi_size;	/* size of this item */
-	uint32_t		efi_nextents;	/* # extents to free */
-	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_32_t		efi_extents[1];	/* array of extents to free */
-} __attribute__((packed)) xfs_efi_log_format_32_t;
+/*
+ * Version of the xfs_extent and xfs_efi_log_format structures that do not
+ * contain padding.  These used to be written to the log by older 32-bit kernels
+ * and will be dealt with transparently by log recovery.
+ */
+struct xfs_extent_32 {
+	uint64_t	ext_start;
+	uint32_t	ext_len;
+} __attribute__((packed));
 
-typedef struct xfs_efi_log_format_64 {
+struct xfs_efi_log_format_32 {
 	uint16_t		efi_type;	/* efi log item type */
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	xfs_extent_64_t		efi_extents[1];	/* array of extents to free */
-} xfs_efi_log_format_64_t;
+	struct xfs_extent_32	efi_extents[1];	/* array of extents to free */
+} __attribute__((packed));
 
 /*
  * This is the structure used to lay out an efd log item in the
@@ -603,7 +590,7 @@ struct xfs_efd_log_format {
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	xfs_extent_t		efd_extents[1];	/* array of extents freed */
+	struct xfs_extent	efd_extents[1];	/* array of extents freed */
 };
 
 /*
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index ac17fdb9283489..ed8d0790908ea7 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -74,7 +74,7 @@ xfs_efi_item_sizeof(
 	struct xfs_efi_log_item *efip)
 {
 	return sizeof(struct xfs_efi_log_format) +
-	       (efip->efi_format.efi_nextents - 1) * sizeof(xfs_extent_t);
+	       (efip->efi_format.efi_nextents - 1) * sizeof(struct xfs_extent);
 }
 
 STATIC void
@@ -158,7 +158,7 @@ xfs_efi_init(
 	ASSERT(nextents > 0);
 	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
 		size = (uint)(sizeof(struct xfs_efi_log_item) +
-			((nextents - 1) * sizeof(xfs_extent_t)));
+			((nextents - 1) * sizeof(struct xfs_extent)));
 		efip = kmem_zalloc(size, 0);
 	} else {
 		efip = kmem_cache_zalloc(xfs_efi_zone,
@@ -174,61 +174,6 @@ xfs_efi_init(
 	return efip;
 }
 
-/*
- * Copy an EFI format buffer from the given buf, and into the destination
- * EFI format structure.
- * The given buffer can be in 32 bit or 64 bit form (which has different padding),
- * one of which will be the native format for this kernel.
- * It will handle the conversion of formats if necessary.
- */
-STATIC int
-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 - 1) * sizeof(xfs_extent_t);  
-	uint len32 = sizeof(xfs_efi_log_format_32_t) + 
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_32_t);  
-	uint len64 = sizeof(xfs_efi_log_format_64_t) + 
-		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_64_t);  
-
-	if (buf->i_len == len) {
-		memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
-		return 0;
-	} else if (buf->i_len == len32) {
-		xfs_efi_log_format_32_t *src_efi_fmt_32 = buf->i_addr;
-
-		dst_efi_fmt->efi_type     = src_efi_fmt_32->efi_type;
-		dst_efi_fmt->efi_size     = src_efi_fmt_32->efi_size;
-		dst_efi_fmt->efi_nextents = src_efi_fmt_32->efi_nextents;
-		dst_efi_fmt->efi_id       = src_efi_fmt_32->efi_id;
-		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
-			dst_efi_fmt->efi_extents[i].ext_start =
-				src_efi_fmt_32->efi_extents[i].ext_start;
-			dst_efi_fmt->efi_extents[i].ext_len =
-				src_efi_fmt_32->efi_extents[i].ext_len;
-		}
-		return 0;
-	} else if (buf->i_len == len64) {
-		xfs_efi_log_format_64_t *src_efi_fmt_64 = buf->i_addr;
-
-		dst_efi_fmt->efi_type     = src_efi_fmt_64->efi_type;
-		dst_efi_fmt->efi_size     = src_efi_fmt_64->efi_size;
-		dst_efi_fmt->efi_nextents = src_efi_fmt_64->efi_nextents;
-		dst_efi_fmt->efi_id       = src_efi_fmt_64->efi_id;
-		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
-			dst_efi_fmt->efi_extents[i].ext_start =
-				src_efi_fmt_64->efi_extents[i].ext_start;
-			dst_efi_fmt->efi_extents[i].ext_len =
-				src_efi_fmt_64->efi_extents[i].ext_len;
-		}
-		return 0;
-	}
-	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
-	return -EFSCORRUPTED;
-}
-
 static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
 {
 	return container_of(lip, struct xfs_efd_log_item, efd_item);
@@ -254,7 +199,7 @@ xfs_efd_item_sizeof(
 	struct xfs_efd_log_item *efdp)
 {
 	return sizeof(struct xfs_efd_log_format) +
-	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
+	       (efdp->efd_format.efd_nextents - 1) * sizeof(struct xfs_extent);
 }
 
 STATIC void
@@ -687,6 +632,36 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
 	.iop_relog	= xfs_efi_item_relog,
 };
 
+/*
+ * Convert from an unpadded EFI log item written by old 32-bit kernels to the
+ * proper format.
+ */
+static int
+xfs_efi_copy_format_32(
+	struct xfs_efi_log_format	*dst,
+	struct xfs_log_iovec		*buf)
+{
+	struct xfs_efi_log_format_32	*src = buf->i_addr;
+	unsigned int			i;
+
+	if (buf->i_len != sizeof(*src) +
+	    (src->efi_nextents - 1) * sizeof(struct xfs_extent_32)) {
+		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
+		return -EFSCORRUPTED;
+	}
+
+	dst->efi_type = src->efi_type;
+	dst->efi_size = src->efi_size;
+	dst->efi_nextents = src->efi_nextents;
+	dst->efi_id = src->efi_id;
+	for (i = 0; i < dst->efi_nextents; i++) {
+		dst->efi_extents[i].ext_start = src->efi_extents[i].ext_start;
+		dst->efi_extents[i].ext_len = src->efi_extents[i].ext_len;
+	}
+
+	return 0;
+}
+
 /*
  * This routine is called to create an in-core extent free intent
  * item from the efi format structure which was logged on disk.
@@ -703,18 +678,22 @@ xlog_recover_efi_commit_pass2(
 {
 	struct xfs_mount		*mp = log->l_mp;
 	struct xfs_efi_log_item		*efip;
-	struct xfs_efi_log_format	*efi_formatp;
+	struct xfs_log_iovec		*buf = &item->ri_buf[0];
+	struct xfs_efi_log_format	*src = buf->i_addr;
 	int				error;
 
-	efi_formatp = item->ri_buf[0].i_addr;
+	efip = xfs_efi_init(mp, src->efi_nextents);
 
-	efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
-	error = xfs_efi_copy_format(&item->ri_buf[0], &efip->efi_format);
-	if (error) {
-		xfs_efi_item_free(efip);
-		return error;
+	if (buf->i_len != sizeof(*src) +
+	    (src->efi_nextents - 1) * sizeof(struct xfs_extent)) {
+		error = xfs_efi_copy_format_32(&efip->efi_format, buf);
+		if (error)
+			goto out_free_efi;
+	} else {
+		memcpy(&efip->efi_format, src, buf->i_len);
 	}
-	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
+
+	atomic_set(&efip->efi_next_extent, efip->efi_format.efi_nextents);
 	/*
 	 * Insert the intent into the AIL directly and drop one reference so
 	 * that finishing or canceling the work will drop the other.
@@ -722,6 +701,10 @@ xlog_recover_efi_commit_pass2(
 	xfs_trans_ail_insert(log->l_ailp, &efip->efi_item, lsn);
 	xfs_efi_release(efip);
 	return 0;
+
+out_free_efi:
+	xfs_efi_item_free(efip);
+	return error;
 }
 
 const struct xlog_recover_item_ops xlog_efi_item_ops = {
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 6b80452ad2a71b..e09afd0f63ff59 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -49,7 +49,7 @@ struct xfs_efi_log_item {
 	struct xfs_log_item	efi_item;
 	atomic_t		efi_refcount;
 	atomic_t		efi_next_extent;
-	xfs_efi_log_format_t	efi_format;
+	struct xfs_efi_log_format efi_format;
 };
 
 /*
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 7328ff92e0ee8a..739476f7dffa21 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -118,10 +118,11 @@ xfs_check_ondisk_structs(void)
 	/* log structures */
 	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	32);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	32);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	32);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_extent,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_icreate_log,		28);
 	XFS_CHECK_STRUCT_SIZE(xfs_ictimestamp_t,		8);
-- 
2.30.1

Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:27:59AM +0200, Christoph Hellwig wrote:
> The extent structure embedded into the EFI and EFD items was originally
> defined as a structure with implicit padding, which makes it a mess
> to handle between 32-bit and 64-bit kernels as the 32-bit ABI packs them
> tight, while the 64-bit ABI implicitly adds an implicit 4-bye padding.
> Log recovery has been able to deal with both formats for a long time,
> although in a rather messy way where the default definition varies
> between the ABIs, but log recovery has two extra special cases for
> padded or unpadded variants.
> 
> Change this to always write the properly fully padded EFI and EFD
> structures to the log, and only special case the unpadded one during
> recovery.

Hmm... so the behavior change here is that 32-bit kernels will start
logging 16-byte xfs_extent structures (like 64-bit kernels)?  I see that
xfs_extent_32 was added for 2.6.18; won't this break recovery on
everything from before that?

Granted, 2.6.17 came out 15 years ago and the last 2.6.16 LTS kernel was
released in 2008 so maybe we don't care, but this would seem to be a
breaking change, right?  This seems like a reasonable change for all V5
filesystems (since that format emerged well after 2.6.18), but not so
good for V4.

--D

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_log_format.h |  49 ++++++--------
>  fs/xfs/xfs_extfree_item.c      | 115 ++++++++++++++-------------------
>  fs/xfs/xfs_extfree_item.h      |   2 +-
>  fs/xfs/xfs_ondisk.h            |   5 +-
>  4 files changed, 71 insertions(+), 100 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index ea0fe9f121adff..639035052b4f65 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -542,56 +542,43 @@ xfs_blft_from_flags(struct xfs_buf_log_format *blf)
>  /*
>   * EFI/EFD log format definitions
>   */
> -typedef struct xfs_extent {
> -	xfs_fsblock_t	ext_start;
> -	xfs_extlen_t	ext_len;
> -} xfs_extent_t;
>  
> -/*
> - * Since an xfs_extent_t has types (start:64, len: 32)
> - * there are different alignments on 32 bit and 64 bit kernels.
> - * So we provide the different variants for use by a
> - * conversion routine.
> - */
> -typedef struct xfs_extent_32 {
> -	uint64_t	ext_start;
> -	uint32_t	ext_len;
> -} __attribute__((packed)) xfs_extent_32_t;
> -
> -typedef struct xfs_extent_64 {
> +struct xfs_extent {
>  	uint64_t	ext_start;
>  	uint32_t	ext_len;
>  	uint32_t	ext_pad;
> -} xfs_extent_64_t;
> +};
>  
>  /*
>   * This is the structure used to lay out an efi log item in the
>   * log.  The efi_extents field is a variable size array whose
>   * size is given by efi_nextents.
>   */
> -typedef struct xfs_efi_log_format {
> +struct xfs_efi_log_format {
>  	uint16_t		efi_type;	/* efi log item type */
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	xfs_extent_t		efi_extents[1];	/* array of extents to free */
> -} xfs_efi_log_format_t;
> +	struct xfs_extent	efi_extents[1];	/* array of extents to free */
> +};
>  
> -typedef struct xfs_efi_log_format_32 {
> -	uint16_t		efi_type;	/* efi log item type */
> -	uint16_t		efi_size;	/* size of this item */
> -	uint32_t		efi_nextents;	/* # extents to free */
> -	uint64_t		efi_id;		/* efi identifier */
> -	xfs_extent_32_t		efi_extents[1];	/* array of extents to free */
> -} __attribute__((packed)) xfs_efi_log_format_32_t;
> +/*
> + * Version of the xfs_extent and xfs_efi_log_format structures that do not
> + * contain padding.  These used to be written to the log by older 32-bit kernels
> + * and will be dealt with transparently by log recovery.
> + */
> +struct xfs_extent_32 {
> +	uint64_t	ext_start;
> +	uint32_t	ext_len;
> +} __attribute__((packed));
>  
> -typedef struct xfs_efi_log_format_64 {
> +struct xfs_efi_log_format_32 {
>  	uint16_t		efi_type;	/* efi log item type */
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	xfs_extent_64_t		efi_extents[1];	/* array of extents to free */
> -} xfs_efi_log_format_64_t;
> +	struct xfs_extent_32	efi_extents[1];	/* array of extents to free */
> +} __attribute__((packed));
>  
>  /*
>   * This is the structure used to lay out an efd log item in the
> @@ -603,7 +590,7 @@ struct xfs_efd_log_format {
>  	uint16_t		efd_size;	/* size of this item */
>  	uint32_t		efd_nextents;	/* # of extents freed */
>  	uint64_t		efd_efi_id;	/* id of corresponding efi */
> -	xfs_extent_t		efd_extents[1];	/* array of extents freed */
> +	struct xfs_extent	efd_extents[1];	/* array of extents freed */
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index ac17fdb9283489..ed8d0790908ea7 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -74,7 +74,7 @@ xfs_efi_item_sizeof(
>  	struct xfs_efi_log_item *efip)
>  {
>  	return sizeof(struct xfs_efi_log_format) +
> -	       (efip->efi_format.efi_nextents - 1) * sizeof(xfs_extent_t);
> +	       (efip->efi_format.efi_nextents - 1) * sizeof(struct xfs_extent);
>  }
>  
>  STATIC void
> @@ -158,7 +158,7 @@ xfs_efi_init(
>  	ASSERT(nextents > 0);
>  	if (nextents > XFS_EFI_MAX_FAST_EXTENTS) {
>  		size = (uint)(sizeof(struct xfs_efi_log_item) +
> -			((nextents - 1) * sizeof(xfs_extent_t)));
> +			((nextents - 1) * sizeof(struct xfs_extent)));
>  		efip = kmem_zalloc(size, 0);
>  	} else {
>  		efip = kmem_cache_zalloc(xfs_efi_zone,
> @@ -174,61 +174,6 @@ xfs_efi_init(
>  	return efip;
>  }
>  
> -/*
> - * Copy an EFI format buffer from the given buf, and into the destination
> - * EFI format structure.
> - * The given buffer can be in 32 bit or 64 bit form (which has different padding),
> - * one of which will be the native format for this kernel.
> - * It will handle the conversion of formats if necessary.
> - */
> -STATIC int
> -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 - 1) * sizeof(xfs_extent_t);  
> -	uint len32 = sizeof(xfs_efi_log_format_32_t) + 
> -		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_32_t);  
> -	uint len64 = sizeof(xfs_efi_log_format_64_t) + 
> -		(src_efi_fmt->efi_nextents - 1) * sizeof(xfs_extent_64_t);  
> -
> -	if (buf->i_len == len) {
> -		memcpy((char *)dst_efi_fmt, (char*)src_efi_fmt, len);
> -		return 0;
> -	} else if (buf->i_len == len32) {
> -		xfs_efi_log_format_32_t *src_efi_fmt_32 = buf->i_addr;
> -
> -		dst_efi_fmt->efi_type     = src_efi_fmt_32->efi_type;
> -		dst_efi_fmt->efi_size     = src_efi_fmt_32->efi_size;
> -		dst_efi_fmt->efi_nextents = src_efi_fmt_32->efi_nextents;
> -		dst_efi_fmt->efi_id       = src_efi_fmt_32->efi_id;
> -		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
> -			dst_efi_fmt->efi_extents[i].ext_start =
> -				src_efi_fmt_32->efi_extents[i].ext_start;
> -			dst_efi_fmt->efi_extents[i].ext_len =
> -				src_efi_fmt_32->efi_extents[i].ext_len;
> -		}
> -		return 0;
> -	} else if (buf->i_len == len64) {
> -		xfs_efi_log_format_64_t *src_efi_fmt_64 = buf->i_addr;
> -
> -		dst_efi_fmt->efi_type     = src_efi_fmt_64->efi_type;
> -		dst_efi_fmt->efi_size     = src_efi_fmt_64->efi_size;
> -		dst_efi_fmt->efi_nextents = src_efi_fmt_64->efi_nextents;
> -		dst_efi_fmt->efi_id       = src_efi_fmt_64->efi_id;
> -		for (i = 0; i < dst_efi_fmt->efi_nextents; i++) {
> -			dst_efi_fmt->efi_extents[i].ext_start =
> -				src_efi_fmt_64->efi_extents[i].ext_start;
> -			dst_efi_fmt->efi_extents[i].ext_len =
> -				src_efi_fmt_64->efi_extents[i].ext_len;
> -		}
> -		return 0;
> -	}
> -	XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> -	return -EFSCORRUPTED;
> -}
> -
>  static inline struct xfs_efd_log_item *EFD_ITEM(struct xfs_log_item *lip)
>  {
>  	return container_of(lip, struct xfs_efd_log_item, efd_item);
> @@ -254,7 +199,7 @@ xfs_efd_item_sizeof(
>  	struct xfs_efd_log_item *efdp)
>  {
>  	return sizeof(struct xfs_efd_log_format) +
> -	       (efdp->efd_format.efd_nextents - 1) * sizeof(xfs_extent_t);
> +	       (efdp->efd_format.efd_nextents - 1) * sizeof(struct xfs_extent);
>  }
>  
>  STATIC void
> @@ -687,6 +632,36 @@ static const struct xfs_item_ops xfs_efi_item_ops = {
>  	.iop_relog	= xfs_efi_item_relog,
>  };
>  
> +/*
> + * Convert from an unpadded EFI log item written by old 32-bit kernels to the
> + * proper format.
> + */
> +static int
> +xfs_efi_copy_format_32(
> +	struct xfs_efi_log_format	*dst,
> +	struct xfs_log_iovec		*buf)
> +{
> +	struct xfs_efi_log_format_32	*src = buf->i_addr;
> +	unsigned int			i;
> +
> +	if (buf->i_len != sizeof(*src) +
> +	    (src->efi_nextents - 1) * sizeof(struct xfs_extent_32)) {
> +		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	dst->efi_type = src->efi_type;
> +	dst->efi_size = src->efi_size;
> +	dst->efi_nextents = src->efi_nextents;
> +	dst->efi_id = src->efi_id;
> +	for (i = 0; i < dst->efi_nextents; i++) {
> +		dst->efi_extents[i].ext_start = src->efi_extents[i].ext_start;
> +		dst->efi_extents[i].ext_len = src->efi_extents[i].ext_len;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * This routine is called to create an in-core extent free intent
>   * item from the efi format structure which was logged on disk.
> @@ -703,18 +678,22 @@ xlog_recover_efi_commit_pass2(
>  {
>  	struct xfs_mount		*mp = log->l_mp;
>  	struct xfs_efi_log_item		*efip;
> -	struct xfs_efi_log_format	*efi_formatp;
> +	struct xfs_log_iovec		*buf = &item->ri_buf[0];
> +	struct xfs_efi_log_format	*src = buf->i_addr;
>  	int				error;
>  
> -	efi_formatp = item->ri_buf[0].i_addr;
> +	efip = xfs_efi_init(mp, src->efi_nextents);
>  
> -	efip = xfs_efi_init(mp, efi_formatp->efi_nextents);
> -	error = xfs_efi_copy_format(&item->ri_buf[0], &efip->efi_format);
> -	if (error) {
> -		xfs_efi_item_free(efip);
> -		return error;
> +	if (buf->i_len != sizeof(*src) +
> +	    (src->efi_nextents - 1) * sizeof(struct xfs_extent)) {
> +		error = xfs_efi_copy_format_32(&efip->efi_format, buf);
> +		if (error)
> +			goto out_free_efi;
> +	} else {
> +		memcpy(&efip->efi_format, src, buf->i_len);
>  	}
> -	atomic_set(&efip->efi_next_extent, efi_formatp->efi_nextents);
> +
> +	atomic_set(&efip->efi_next_extent, efip->efi_format.efi_nextents);
>  	/*
>  	 * Insert the intent into the AIL directly and drop one reference so
>  	 * that finishing or canceling the work will drop the other.
> @@ -722,6 +701,10 @@ xlog_recover_efi_commit_pass2(
>  	xfs_trans_ail_insert(log->l_ailp, &efip->efi_item, lsn);
>  	xfs_efi_release(efip);
>  	return 0;
> +
> +out_free_efi:
> +	xfs_efi_item_free(efip);
> +	return error;
>  }
>  
>  const struct xlog_recover_item_ops xlog_efi_item_ops = {
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 6b80452ad2a71b..e09afd0f63ff59 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -49,7 +49,7 @@ struct xfs_efi_log_item {
>  	struct xfs_log_item	efi_item;
>  	atomic_t		efi_refcount;
>  	atomic_t		efi_next_extent;
> -	xfs_efi_log_format_t	efi_format;
> +	struct xfs_efi_log_format efi_format;
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 7328ff92e0ee8a..739476f7dffa21 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -118,10 +118,11 @@ xfs_check_ondisk_structs(void)
>  	/* log structures */
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	32);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	32);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_64,	32);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_extent,		16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_64,		16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_icreate_log,		28);
>  	XFS_CHECK_STRUCT_SIZE(xfs_ictimestamp_t,		8);
> -- 
> 2.30.1
> 

Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling

[Christoph Hellwig]

On Tue, Apr 20, 2021 at 10:05:29AM -0700, Darrick J. Wong wrote:
> Hmm... so the behavior change here is that 32-bit kernels will start
> logging 16-byte xfs_extent structures (like 64-bit kernels)?

Yes.

> I see that
> xfs_extent_32 was added for 2.6.18; won't this break recovery on
> everything from before that?

Where everything is a 32-bit kernel that doesn't align properly, yes.

> Granted, 2.6.17 came out 15 years ago and the last 2.6.16 LTS kernel was
> released in 2008 so maybe we don't care, but this would seem to be a
> breaking change, right?  This seems like a reasonable change for all V5
> filesystems (since that format emerged well after 2.6.18), but not so
> good for V4.

Err, why?

Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling

[Darrick J. Wong]

On Wed, Apr 21, 2021 at 07:55:41AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 20, 2021 at 10:05:29AM -0700, Darrick J. Wong wrote:
> > Hmm... so the behavior change here is that 32-bit kernels will start
> > logging 16-byte xfs_extent structures (like 64-bit kernels)?
> 
> Yes.
> 
> > I see that
> > xfs_extent_32 was added for 2.6.18; won't this break recovery on
> > everything from before that?
> 
> Where everything is a 32-bit kernel that doesn't align properly, yes.
> 
> > Granted, 2.6.17 came out 15 years ago and the last 2.6.16 LTS kernel was
> > released in 2008 so maybe we don't care, but this would seem to be a
> > breaking change, right?  This seems like a reasonable change for all V5
> > filesystems (since that format emerged well after 2.6.18), but not so
> > good for V4.
> 
> Err, why?

Log recovery on those old kernels will choke on the 16-bit xfs_extent
records, because they only know how to interpret a 12-bit xfs_extent.
In 2.6.17, xlog_recover_do_efi_trans did this:

	efi_formatp = (xfs_efi_log_format_t *)item->ri_buf[0].i_addr;
	ASSERT(item->ri_buf[0].i_len ==
	       (sizeof(xfs_efi_log_format_t) +
		((efi_formatp->efi_nextents - 1) * sizeof(xfs_extent_t))));

The ASSERT will trigger on the size being wrong due to the padding
error, but on non-DEBUG kernels that won't interrupt log recovery, so we
proceed on to this:

	memcpy((char *)&(efip->efi_format), (char *)efi_formatp,
	      sizeof(xfs_efi_log_format_t) +
	      ((efi_formatp->efi_nextents - 1) * sizeof(xfs_extent_t)));

In this particular case, we fail to copy the all of the bytes from the
recovered EFI into the new incore EFI log item.  If there is more than 1
extent, the fields in the (N+1)th incore extent won't line up with the
fields that were written to disk, which means we'll replay garbage.

--D

[PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof

[Christoph Hellwig]

xfs_efi_log_item only looks at the embedded xfs_efi_log_item structure,
so pass that directly and rename the function to xfs_efi_log_item_sizeof.
This allows using the helper in xlog_recover_efi_commit_pass2 as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index ed8d0790908ea7..7ae570d1944590 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -70,11 +70,11 @@ xfs_efi_release(
  * structure.
  */
 static inline int
-xfs_efi_item_sizeof(
-	struct xfs_efi_log_item *efip)
+xfs_efi_log_item_sizeof(
+	struct xfs_efi_log_format *elf)
 {
-	return sizeof(struct xfs_efi_log_format) +
-	       (efip->efi_format.efi_nextents - 1) * sizeof(struct xfs_extent);
+	return sizeof(*elf) +
+	       (elf->efi_nextents - 1) * sizeof(struct xfs_extent);
 }
 
 STATIC void
@@ -84,7 +84,7 @@ xfs_efi_item_size(
 	int			*nbytes)
 {
 	*nvecs += 1;
-	*nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip));
+	*nbytes += xfs_efi_log_item_sizeof(&EFI_ITEM(lip)->efi_format);
 }
 
 /*
@@ -110,7 +110,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_item_sizeof(&efip->efi_format));
 }
 
 
@@ -684,8 +684,7 @@ xlog_recover_efi_commit_pass2(
 
 	efip = xfs_efi_init(mp, src->efi_nextents);
 
-	if (buf->i_len != sizeof(*src) +
-	    (src->efi_nextents - 1) * sizeof(struct xfs_extent)) {
+	if (buf->i_len != xfs_efi_log_item_sizeof(src)) {
 		error = xfs_efi_copy_format_32(&efip->efi_format, buf);
 		if (error)
 			goto out_free_efi;
-- 
2.30.1

Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:28:00AM +0200, Christoph Hellwig wrote:
> xfs_efi_log_item only looks at the embedded xfs_efi_log_item structure,
> so pass that directly and rename the function to xfs_efi_log_item_sizeof.
> This allows using the helper in xlog_recover_efi_commit_pass2 as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_extfree_item.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index ed8d0790908ea7..7ae570d1944590 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -70,11 +70,11 @@ xfs_efi_release(
>   * structure.
>   */
>  static inline int
> -xfs_efi_item_sizeof(
> -	struct xfs_efi_log_item *efip)
> +xfs_efi_log_item_sizeof(

Shouldn't this be named xfs_efi_log_format_sizeof to correspond to the
name of the structure?  Two patches from now you (re)introduce
xfs_efi_item_sizeof that returns the size of a struct xfs_efi_log_item,
which is confusing.

--D


> +	struct xfs_efi_log_format *elf)
>  {
> -	return sizeof(struct xfs_efi_log_format) +
> -	       (efip->efi_format.efi_nextents - 1) * sizeof(struct xfs_extent);
> +	return sizeof(*elf) +
> +	       (elf->efi_nextents - 1) * sizeof(struct xfs_extent);
>  }
>  
>  STATIC void
> @@ -84,7 +84,7 @@ xfs_efi_item_size(
>  	int			*nbytes)
>  {
>  	*nvecs += 1;
> -	*nbytes += xfs_efi_item_sizeof(EFI_ITEM(lip));
> +	*nbytes += xfs_efi_log_item_sizeof(&EFI_ITEM(lip)->efi_format);
>  }
>  
>  /*
> @@ -110,7 +110,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_item_sizeof(&efip->efi_format));
>  }
>  
>  
> @@ -684,8 +684,7 @@ xlog_recover_efi_commit_pass2(
>  
>  	efip = xfs_efi_init(mp, src->efi_nextents);
>  
> -	if (buf->i_len != sizeof(*src) +
> -	    (src->efi_nextents - 1) * sizeof(struct xfs_extent)) {
> +	if (buf->i_len != xfs_efi_log_item_sizeof(src)) {
>  		error = xfs_efi_copy_format_32(&efip->efi_format, buf);
>  		if (error)
>  			goto out_free_efi;
> -- 
> 2.30.1
> 

Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof

[Christoph Hellwig]

On Tue, Apr 20, 2021 at 10:09:52AM -0700, Darrick J. Wong wrote:
> On Mon, Apr 19, 2021 at 10:28:00AM +0200, Christoph Hellwig wrote:
> > xfs_efi_log_item only looks at the embedded xfs_efi_log_item structure,
> > so pass that directly and rename the function to xfs_efi_log_item_sizeof.
> > This allows using the helper in xlog_recover_efi_commit_pass2 as well.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/xfs_extfree_item.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > index ed8d0790908ea7..7ae570d1944590 100644
> > --- a/fs/xfs/xfs_extfree_item.c
> > +++ b/fs/xfs/xfs_extfree_item.c
> > @@ -70,11 +70,11 @@ xfs_efi_release(
> >   * structure.
> >   */
> >  static inline int
> > -xfs_efi_item_sizeof(
> > -	struct xfs_efi_log_item *efip)
> > +xfs_efi_log_item_sizeof(
> 
> Shouldn't this be named xfs_efi_log_format_sizeof to correspond to the
> name of the structure?  Two patches from now you (re)introduce
> xfs_efi_item_sizeof that returns the size of a struct xfs_efi_log_item,
> which is confusing.

Well, that was the main reason to move these out of place, as they are
rather misnamed.

Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof

[Darrick J. Wong]

On Wed, Apr 21, 2021 at 07:56:28AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 20, 2021 at 10:09:52AM -0700, Darrick J. Wong wrote:
> > On Mon, Apr 19, 2021 at 10:28:00AM +0200, Christoph Hellwig wrote:
> > > xfs_efi_log_item only looks at the embedded xfs_efi_log_item structure,
> > > so pass that directly and rename the function to xfs_efi_log_item_sizeof.
> > > This allows using the helper in xlog_recover_efi_commit_pass2 as well.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_extfree_item.c | 15 +++++++--------
> > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > > index ed8d0790908ea7..7ae570d1944590 100644
> > > --- a/fs/xfs/xfs_extfree_item.c
> > > +++ b/fs/xfs/xfs_extfree_item.c
> > > @@ -70,11 +70,11 @@ xfs_efi_release(
> > >   * structure.
> > >   */
> > >  static inline int
> > > -xfs_efi_item_sizeof(
> > > -	struct xfs_efi_log_item *efip)
> > > +xfs_efi_log_item_sizeof(
> > 
> > Shouldn't this be named xfs_efi_log_format_sizeof to correspond to the
> > name of the structure?  Two patches from now you (re)introduce
> > xfs_efi_item_sizeof that returns the size of a struct xfs_efi_log_item,
> > which is confusing.
> 
> Well, that was the main reason to move these out of place, as they are
> rather misnamed.

I agree with the motivation, but not the names you've picked.  I don't
like xfs_efi_log_item_sizeof /not/ being the sizing function for struct
xfs_efi_log_item, because (in my head anyway) XXX_sizeof should be the
function for struct XXX.

IOWs I think that in the end these two helpers should be:

static inline int
xfs_efi_log_format_sizeof(
	struct xfs_efi_log_format *elf)
{
	return sizeof(*elf) + elf->efi_nextents * sizeof(struct xfs_extent);
}

static inline int
xfs_efi_log_item_sizeof(unsigned int nextents)
{
	return sizeof(struct xfs_efi_log_item) + nextents * sizeof(struct xfs_extent);
}

--D

[PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof

[Christoph Hellwig]

xfs_efd_log_item only looks at the embedded xfs_efd_log_item structure,
so pass that directly and rename the function to xfs_efd_log_item_sizeof.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index 7ae570d1944590..f15d6cfca6e2f1 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -195,11 +195,11 @@ xfs_efd_item_free(struct xfs_efd_log_item *efdp)
  * structure.
  */
 static inline int
-xfs_efd_item_sizeof(
-	struct xfs_efd_log_item *efdp)
+xfs_efd_log_item_sizeof(
+	struct xfs_efd_log_format *elf)
 {
 	return sizeof(struct xfs_efd_log_format) +
-	       (efdp->efd_format.efd_nextents - 1) * sizeof(struct xfs_extent);
+	       (elf->efd_nextents - 1) * sizeof(struct xfs_extent);
 }
 
 STATIC void
@@ -209,7 +209,7 @@ xfs_efd_item_size(
 	int			*nbytes)
 {
 	*nvecs += 1;
-	*nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip));
+	*nbytes += xfs_efd_log_item_sizeof(&EFD_ITEM(lip)->efd_format);
 }
 
 /*
@@ -234,7 +234,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_item_sizeof(&efdp->efd_format));
 }
 
 /*
-- 
2.30.1

Re: [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:28:01AM +0200, Christoph Hellwig wrote:
> xfs_efd_log_item only looks at the embedded xfs_efd_log_item structure,
> so pass that directly and rename the function to xfs_efd_log_item_sizeof.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_extfree_item.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index 7ae570d1944590..f15d6cfca6e2f1 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -195,11 +195,11 @@ xfs_efd_item_free(struct xfs_efd_log_item *efdp)
>   * structure.
>   */
>  static inline int
> -xfs_efd_item_sizeof(
> -	struct xfs_efd_log_item *efdp)
> +xfs_efd_log_item_sizeof(

Same naming complaint as the last patch, though the code changes
themselves look fine to me.

--D

> +	struct xfs_efd_log_format *elf)
>  {
>  	return sizeof(struct xfs_efd_log_format) +
> -	       (efdp->efd_format.efd_nextents - 1) * sizeof(struct xfs_extent);
> +	       (elf->efd_nextents - 1) * sizeof(struct xfs_extent);
>  }
>  
>  STATIC void
> @@ -209,7 +209,7 @@ xfs_efd_item_size(
>  	int			*nbytes)
>  {
>  	*nvecs += 1;
> -	*nbytes += xfs_efd_item_sizeof(EFD_ITEM(lip));
> +	*nbytes += xfs_efd_log_item_sizeof(&EFD_ITEM(lip)->efd_format);
>  }
>  
>  /*
> @@ -234,7 +234,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_item_sizeof(&efdp->efd_format));
>  }
>  
>  /*
> -- 
> 2.30.1
> 

[PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper

[Christoph Hellwig]

Add a helper to calculate the size of an xfs_efi_log_item structure
the specified number of extents.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c | 10 +++-------
 fs/xfs/xfs_extfree_item.h |  6 ++++++
 fs/xfs/xfs_super.c        |  6 ++----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index f15d6cfca6e2f1..afd568d426c1f1 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -153,17 +153,13 @@ 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 - 1) * sizeof(struct xfs_extent)));
-		efip = kmem_zalloc(size, 0);
-	} else {
+	if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
+		efip = kmem_zalloc(xfs_efi_item_sizeof(nextents), 0);
+	else
 		efip = kmem_cache_zalloc(xfs_efi_zone,
 					 GFP_KERNEL | __GFP_NOFAIL);
-	}
 
 	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
 	efip->efi_format.efi_nextents = nextents;
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index e09afd0f63ff59..d2577d872de771 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -52,6 +52,12 @@ struct xfs_efi_log_item {
 	struct xfs_efi_log_format efi_format;
 };
 
+static inline int xfs_efi_item_sizeof(unsigned int nextents)
+{
+	return sizeof(struct xfs_efi_log_item) +
+		(nextents - 1) * sizeof(struct xfs_extent);
+}
+
 /*
  * This is the "extent free done" log item.  It is used to log
  * the fact that some extents earlier mentioned in an efi item
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index a2dab05332ac27..c93710cb5ce3f0 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1961,10 +1961,8 @@ xfs_init_zones(void)
 		goto out_destroy_buf_item_zone;
 
 	xfs_efi_zone = kmem_cache_create("xfs_efi_item",
-					 (sizeof(struct xfs_efi_log_item) +
-					 (XFS_EFI_MAX_FAST_EXTENTS - 1) *
-					 sizeof(struct xfs_extent)),
-					 0, 0, NULL);
+			xfs_efi_item_sizeof(XFS_EFI_MAX_FAST_EXTENTS),
+			0, 0, NULL);
 	if (!xfs_efi_zone)
 		goto out_destroy_efd_zone;
 
-- 
2.30.1

Re: [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:28:02AM +0200, Christoph Hellwig wrote:
> Add a helper to calculate the size of an xfs_efi_log_item structure
> the specified number of extents.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Function name questions notwithstanding, this is a nice cleanup.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_extfree_item.c | 10 +++-------
>  fs/xfs/xfs_extfree_item.h |  6 ++++++
>  fs/xfs/xfs_super.c        |  6 ++----
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index f15d6cfca6e2f1..afd568d426c1f1 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -153,17 +153,13 @@ 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 - 1) * sizeof(struct xfs_extent)));
> -		efip = kmem_zalloc(size, 0);
> -	} else {
> +	if (nextents > XFS_EFI_MAX_FAST_EXTENTS)
> +		efip = kmem_zalloc(xfs_efi_item_sizeof(nextents), 0);
> +	else
>  		efip = kmem_cache_zalloc(xfs_efi_zone,
>  					 GFP_KERNEL | __GFP_NOFAIL);
> -	}
>  
>  	xfs_log_item_init(mp, &efip->efi_item, XFS_LI_EFI, &xfs_efi_item_ops);
>  	efip->efi_format.efi_nextents = nextents;
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index e09afd0f63ff59..d2577d872de771 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -52,6 +52,12 @@ struct xfs_efi_log_item {
>  	struct xfs_efi_log_format efi_format;
>  };
>  
> +static inline int xfs_efi_item_sizeof(unsigned int nextents)
> +{
> +	return sizeof(struct xfs_efi_log_item) +
> +		(nextents - 1) * sizeof(struct xfs_extent);
> +}
> +
>  /*
>   * This is the "extent free done" log item.  It is used to log
>   * the fact that some extents earlier mentioned in an efi item
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index a2dab05332ac27..c93710cb5ce3f0 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1961,10 +1961,8 @@ xfs_init_zones(void)
>  		goto out_destroy_buf_item_zone;
>  
>  	xfs_efi_zone = kmem_cache_create("xfs_efi_item",
> -					 (sizeof(struct xfs_efi_log_item) +
> -					 (XFS_EFI_MAX_FAST_EXTENTS - 1) *
> -					 sizeof(struct xfs_extent)),
> -					 0, 0, NULL);
> +			xfs_efi_item_sizeof(XFS_EFI_MAX_FAST_EXTENTS),
> +			0, 0, NULL);
>  	if (!xfs_efi_zone)
>  		goto out_destroy_efd_zone;
>  
> -- 
> 2.30.1
> 

[PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper

[Christoph Hellwig]

Add a helper to calculate the size of an xfs_efd_log_item structure
the specified number of extents.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_extfree_item.c | 10 +++-------
 fs/xfs/xfs_extfree_item.h |  6 ++++++
 fs/xfs/xfs_super.c        |  6 ++----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index afd568d426c1f1..a2abdfd3d076bf 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -268,15 +268,11 @@ xfs_trans_get_efd(
 	struct xfs_efd_log_item		*efdp;
 
 	ASSERT(nextents > 0);
-
-	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
-		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
-				(nextents - 1) * sizeof(struct xfs_extent),
-				0);
-	} else {
+	if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
+		efdp = kmem_zalloc(xfs_efd_item_sizeof(nextents), 0);
+	else
 		efdp = kmem_cache_zalloc(xfs_efd_zone,
 					GFP_KERNEL | __GFP_NOFAIL);
-	}
 
 	xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
 			  &xfs_efd_item_ops);
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index d2577d872de771..3bb62ef525f2e0 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -70,6 +70,12 @@ struct xfs_efd_log_item {
 	struct xfs_efd_log_format efd_format;
 };
 
+static inline int xfs_efd_item_sizeof(unsigned int nextents)
+{
+	return sizeof(struct xfs_efd_log_item) +
+		(nextents - 1) * sizeof(struct xfs_extent);
+}
+
 /*
  * Max number of extents in fast allocation path.
  */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index c93710cb5ce3f0..f7f70438d98703 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1953,10 +1953,8 @@ xfs_init_zones(void)
 		goto out_destroy_trans_zone;
 
 	xfs_efd_zone = kmem_cache_create("xfs_efd_item",
-					(sizeof(struct xfs_efd_log_item) +
-					(XFS_EFD_MAX_FAST_EXTENTS - 1) *
-					sizeof(struct xfs_extent)),
-					0, 0, NULL);
+			xfs_efd_item_sizeof(XFS_EFD_MAX_FAST_EXTENTS),
+			0, 0, NULL);
 	if (!xfs_efd_zone)
 		goto out_destroy_buf_item_zone;
 
-- 
2.30.1

Re: [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:28:03AM +0200, Christoph Hellwig wrote:
> Add a helper to calculate the size of an xfs_efd_log_item structure
> the specified number of extents.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Questions about function names notwithstanding,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_extfree_item.c | 10 +++-------
>  fs/xfs/xfs_extfree_item.h |  6 ++++++
>  fs/xfs/xfs_super.c        |  6 ++----
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index afd568d426c1f1..a2abdfd3d076bf 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -268,15 +268,11 @@ xfs_trans_get_efd(
>  	struct xfs_efd_log_item		*efdp;
>  
>  	ASSERT(nextents > 0);
> -
> -	if (nextents > XFS_EFD_MAX_FAST_EXTENTS) {
> -		efdp = kmem_zalloc(sizeof(struct xfs_efd_log_item) +
> -				(nextents - 1) * sizeof(struct xfs_extent),
> -				0);
> -	} else {
> +	if (nextents > XFS_EFD_MAX_FAST_EXTENTS)
> +		efdp = kmem_zalloc(xfs_efd_item_sizeof(nextents), 0);
> +	else
>  		efdp = kmem_cache_zalloc(xfs_efd_zone,
>  					GFP_KERNEL | __GFP_NOFAIL);
> -	}
>  
>  	xfs_log_item_init(tp->t_mountp, &efdp->efd_item, XFS_LI_EFD,
>  			  &xfs_efd_item_ops);
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index d2577d872de771..3bb62ef525f2e0 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -70,6 +70,12 @@ struct xfs_efd_log_item {
>  	struct xfs_efd_log_format efd_format;
>  };
>  
> +static inline int xfs_efd_item_sizeof(unsigned int nextents)
> +{
> +	return sizeof(struct xfs_efd_log_item) +
> +		(nextents - 1) * sizeof(struct xfs_extent);
> +}
> +
>  /*
>   * Max number of extents in fast allocation path.
>   */
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index c93710cb5ce3f0..f7f70438d98703 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1953,10 +1953,8 @@ xfs_init_zones(void)
>  		goto out_destroy_trans_zone;
>  
>  	xfs_efd_zone = kmem_cache_create("xfs_efd_item",
> -					(sizeof(struct xfs_efd_log_item) +
> -					(XFS_EFD_MAX_FAST_EXTENTS - 1) *
> -					sizeof(struct xfs_extent)),
> -					0, 0, NULL);
> +			xfs_efd_item_sizeof(XFS_EFD_MAX_FAST_EXTENTS),
> +			0, 0, NULL);
>  	if (!xfs_efd_zone)
>  		goto out_destroy_buf_item_zone;
>  
> -- 
> 2.30.1
> 

[PATCH 7/7] xfs: Replace one-element arrays with flexible-array members

[Christoph Hellwig]

From: "Gustavo A. R. Silva" <gustavoars@kernel.org>

There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].

Refactor the code according to the use of flexible-array members in
multiple structures, instead of one-element arrays. Also, make use of
the new struct_size() helper to properly calculate the size of some
structures that contain flexible-array members.

[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
[hch: rebased on top of the previous cleanups]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_log_format.h | 6 +++---
 fs/xfs/xfs_extfree_item.c      | 9 +++------
 fs/xfs/xfs_extfree_item.h      | 4 ++--
 fs/xfs/xfs_ondisk.h            | 6 +++---
 4 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 639035052b4f65..9b218c30659ad7 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -559,7 +559,7 @@ struct xfs_efi_log_format {
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	struct xfs_extent	efi_extents[1];	/* array of extents to free */
+	struct xfs_extent	efi_extents[];	/* array of extents to free */
 };
 
 /*
@@ -577,7 +577,7 @@ struct xfs_efi_log_format_32 {
 	uint16_t		efi_size;	/* size of this item */
 	uint32_t		efi_nextents;	/* # extents to free */
 	uint64_t		efi_id;		/* efi identifier */
-	struct xfs_extent_32	efi_extents[1];	/* array of extents to free */
+	struct xfs_extent_32	efi_extents[];	/* array of extents to free */
 } __attribute__((packed));
 
 /*
@@ -590,7 +590,7 @@ struct xfs_efd_log_format {
 	uint16_t		efd_size;	/* size of this item */
 	uint32_t		efd_nextents;	/* # of extents freed */
 	uint64_t		efd_efi_id;	/* id of corresponding efi */
-	struct xfs_extent	efd_extents[1];	/* array of extents freed */
+	struct xfs_extent	efd_extents[];	/* array of extents freed */
 };
 
 /*
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index a2abdfd3d076bf..8bea9c9ecf2042 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -73,8 +73,7 @@ static inline int
 xfs_efi_log_item_sizeof(
 	struct xfs_efi_log_format *elf)
 {
-	return sizeof(*elf) +
-	       (elf->efi_nextents - 1) * sizeof(struct xfs_extent);
+	return struct_size(elf, efi_extents, elf->efi_nextents);
 }
 
 STATIC void
@@ -194,8 +193,7 @@ static inline int
 xfs_efd_log_item_sizeof(
 	struct xfs_efd_log_format *elf)
 {
-	return sizeof(struct xfs_efd_log_format) +
-	       (elf->efd_nextents - 1) * sizeof(struct xfs_extent);
+	return struct_size(elf, efd_extents, elf->efd_nextents);
 }
 
 STATIC void
@@ -636,8 +634,7 @@ xfs_efi_copy_format_32(
 	struct xfs_efi_log_format_32	*src = buf->i_addr;
 	unsigned int			i;
 
-	if (buf->i_len != sizeof(*src) +
-	    (src->efi_nextents - 1) * sizeof(struct xfs_extent_32)) {
+	if (buf->i_len != struct_size(src, efi_extents, src->efi_nextents)) {
 		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
 		return -EFSCORRUPTED;
 	}
diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
index 3bb62ef525f2e0..a01ce86145bb64 100644
--- a/fs/xfs/xfs_extfree_item.h
+++ b/fs/xfs/xfs_extfree_item.h
@@ -55,7 +55,7 @@ struct xfs_efi_log_item {
 static inline int xfs_efi_item_sizeof(unsigned int nextents)
 {
 	return sizeof(struct xfs_efi_log_item) +
-		(nextents - 1) * sizeof(struct xfs_extent);
+		nextents * sizeof(struct xfs_extent);
 }
 
 /*
@@ -73,7 +73,7 @@ struct xfs_efd_log_item {
 static inline int xfs_efd_item_sizeof(unsigned int nextents)
 {
 	return sizeof(struct xfs_efd_log_item) +
-		(nextents - 1) * sizeof(struct xfs_extent);
+		nextents * sizeof(struct xfs_extent);
 }
 
 /*
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 739476f7dffa21..fa4b590671bf58 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -118,9 +118,9 @@ xfs_check_ondisk_structs(void)
 	/* log structures */
 	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	32);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	32);
-	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	16);
+	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent,		16);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
-- 
2.30.1

Re: [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members

[Darrick J. Wong]

On Mon, Apr 19, 2021 at 10:28:04AM +0200, Christoph Hellwig wrote:
> From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> 
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code according to the use of flexible-array members in
> multiple structures, instead of one-element arrays. Also, make use of
> the new struct_size() helper to properly calculate the size of some
> structures that contain flexible-array members.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> [hch: rebased on top of the previous cleanups]
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me besides the naming thing; with those resolved,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/libxfs/xfs_log_format.h | 6 +++---
>  fs/xfs/xfs_extfree_item.c      | 9 +++------
>  fs/xfs/xfs_extfree_item.h      | 4 ++--
>  fs/xfs/xfs_ondisk.h            | 6 +++---
>  4 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 639035052b4f65..9b218c30659ad7 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -559,7 +559,7 @@ struct xfs_efi_log_format {
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	struct xfs_extent	efi_extents[1];	/* array of extents to free */
> +	struct xfs_extent	efi_extents[];	/* array of extents to free */
>  };
>  
>  /*
> @@ -577,7 +577,7 @@ struct xfs_efi_log_format_32 {
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	struct xfs_extent_32	efi_extents[1];	/* array of extents to free */
> +	struct xfs_extent_32	efi_extents[];	/* array of extents to free */
>  } __attribute__((packed));
>  
>  /*
> @@ -590,7 +590,7 @@ struct xfs_efd_log_format {
>  	uint16_t		efd_size;	/* size of this item */
>  	uint32_t		efd_nextents;	/* # of extents freed */
>  	uint64_t		efd_efi_id;	/* id of corresponding efi */
> -	struct xfs_extent	efd_extents[1];	/* array of extents freed */
> +	struct xfs_extent	efd_extents[];	/* array of extents freed */
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index a2abdfd3d076bf..8bea9c9ecf2042 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -73,8 +73,7 @@ static inline int
>  xfs_efi_log_item_sizeof(
>  	struct xfs_efi_log_format *elf)
>  {
> -	return sizeof(*elf) +
> -	       (elf->efi_nextents - 1) * sizeof(struct xfs_extent);
> +	return struct_size(elf, efi_extents, elf->efi_nextents);
>  }
>  
>  STATIC void
> @@ -194,8 +193,7 @@ static inline int
>  xfs_efd_log_item_sizeof(
>  	struct xfs_efd_log_format *elf)
>  {
> -	return sizeof(struct xfs_efd_log_format) +
> -	       (elf->efd_nextents - 1) * sizeof(struct xfs_extent);
> +	return struct_size(elf, efd_extents, elf->efd_nextents);
>  }
>  
>  STATIC void
> @@ -636,8 +634,7 @@ xfs_efi_copy_format_32(
>  	struct xfs_efi_log_format_32	*src = buf->i_addr;
>  	unsigned int			i;
>  
> -	if (buf->i_len != sizeof(*src) +
> -	    (src->efi_nextents - 1) * sizeof(struct xfs_extent_32)) {
> +	if (buf->i_len != struct_size(src, efi_extents, src->efi_nextents)) {
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
>  		return -EFSCORRUPTED;
>  	}
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 3bb62ef525f2e0..a01ce86145bb64 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -55,7 +55,7 @@ struct xfs_efi_log_item {
>  static inline int xfs_efi_item_sizeof(unsigned int nextents)
>  {
>  	return sizeof(struct xfs_efi_log_item) +
> -		(nextents - 1) * sizeof(struct xfs_extent);
> +		nextents * sizeof(struct xfs_extent);
>  }
>  
>  /*
> @@ -73,7 +73,7 @@ struct xfs_efd_log_item {
>  static inline int xfs_efd_item_sizeof(unsigned int nextents)
>  {
>  	return sizeof(struct xfs_efd_log_item) +
> -		(nextents - 1) * sizeof(struct xfs_extent);
> +		nextents * sizeof(struct xfs_extent);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 739476f7dffa21..fa4b590671bf58 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -118,9 +118,9 @@ xfs_check_ondisk_structs(void)
>  	/* log structures */
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	32);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	32);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	16);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	16);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent,		16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
> -- 
> 2.30.1
> 

Re: [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members

[Gustavo A. R. Silva]

On 4/19/21 03:28, Christoph Hellwig wrote:
> From: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> 
> There is a regular need in the kernel to provide a way to declare having
> a dynamically sized set of trailing elements in a structure. Kernel code
> should always use “flexible array members”[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code according to the use of flexible-array members in
> multiple structures, instead of one-element arrays. Also, make use of
> the new struct_size() helper to properly calculate the size of some
> structures that contain flexible-array members.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> [hch: rebased on top of the previous cleanups]
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks, Christoph.
--
Gustavo

> ---
>  fs/xfs/libxfs/xfs_log_format.h | 6 +++---
>  fs/xfs/xfs_extfree_item.c      | 9 +++------
>  fs/xfs/xfs_extfree_item.h      | 4 ++--
>  fs/xfs/xfs_ondisk.h            | 6 +++---
>  4 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 639035052b4f65..9b218c30659ad7 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -559,7 +559,7 @@ struct xfs_efi_log_format {
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	struct xfs_extent	efi_extents[1];	/* array of extents to free */
> +	struct xfs_extent	efi_extents[];	/* array of extents to free */
>  };
>  
>  /*
> @@ -577,7 +577,7 @@ struct xfs_efi_log_format_32 {
>  	uint16_t		efi_size;	/* size of this item */
>  	uint32_t		efi_nextents;	/* # extents to free */
>  	uint64_t		efi_id;		/* efi identifier */
> -	struct xfs_extent_32	efi_extents[1];	/* array of extents to free */
> +	struct xfs_extent_32	efi_extents[];	/* array of extents to free */
>  } __attribute__((packed));
>  
>  /*
> @@ -590,7 +590,7 @@ struct xfs_efd_log_format {
>  	uint16_t		efd_size;	/* size of this item */
>  	uint32_t		efd_nextents;	/* # of extents freed */
>  	uint64_t		efd_efi_id;	/* id of corresponding efi */
> -	struct xfs_extent	efd_extents[1];	/* array of extents freed */
> +	struct xfs_extent	efd_extents[];	/* array of extents freed */
>  };
>  
>  /*
> diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> index a2abdfd3d076bf..8bea9c9ecf2042 100644
> --- a/fs/xfs/xfs_extfree_item.c
> +++ b/fs/xfs/xfs_extfree_item.c
> @@ -73,8 +73,7 @@ static inline int
>  xfs_efi_log_item_sizeof(
>  	struct xfs_efi_log_format *elf)
>  {
> -	return sizeof(*elf) +
> -	       (elf->efi_nextents - 1) * sizeof(struct xfs_extent);
> +	return struct_size(elf, efi_extents, elf->efi_nextents);
>  }
>  
>  STATIC void
> @@ -194,8 +193,7 @@ static inline int
>  xfs_efd_log_item_sizeof(
>  	struct xfs_efd_log_format *elf)
>  {
> -	return sizeof(struct xfs_efd_log_format) +
> -	       (elf->efd_nextents - 1) * sizeof(struct xfs_extent);
> +	return struct_size(elf, efd_extents, elf->efd_nextents);
>  }
>  
>  STATIC void
> @@ -636,8 +634,7 @@ xfs_efi_copy_format_32(
>  	struct xfs_efi_log_format_32	*src = buf->i_addr;
>  	unsigned int			i;
>  
> -	if (buf->i_len != sizeof(*src) +
> -	    (src->efi_nextents - 1) * sizeof(struct xfs_extent_32)) {
> +	if (buf->i_len != struct_size(src, efi_extents, src->efi_nextents)) {
>  		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, NULL);
>  		return -EFSCORRUPTED;
>  	}
> diff --git a/fs/xfs/xfs_extfree_item.h b/fs/xfs/xfs_extfree_item.h
> index 3bb62ef525f2e0..a01ce86145bb64 100644
> --- a/fs/xfs/xfs_extfree_item.h
> +++ b/fs/xfs/xfs_extfree_item.h
> @@ -55,7 +55,7 @@ struct xfs_efi_log_item {
>  static inline int xfs_efi_item_sizeof(unsigned int nextents)
>  {
>  	return sizeof(struct xfs_efi_log_item) +
> -		(nextents - 1) * sizeof(struct xfs_extent);
> +		nextents * sizeof(struct xfs_extent);
>  }
>  
>  /*
> @@ -73,7 +73,7 @@ struct xfs_efd_log_item {
>  static inline int xfs_efd_item_sizeof(unsigned int nextents)
>  {
>  	return sizeof(struct xfs_efd_log_item) +
> -		(nextents - 1) * sizeof(struct xfs_extent);
> +		nextents * sizeof(struct xfs_extent);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 739476f7dffa21..fa4b590671bf58 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -118,9 +118,9 @@ xfs_check_ondisk_structs(void)
>  	/* log structures */
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_buf_log_format,	88);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_dq_logformat,		24);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	32);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	32);
> -	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	28);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efd_log_format,	16);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format,	16);
> +	XFS_CHECK_STRUCT_SIZE(struct xfs_efi_log_format_32,	16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent,		16);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_extent_32,		12);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_log_dinode,		176);
>