* cleanup the EFI/EFD definitions
@ 2021-04-19 8:27 Christoph Hellwig
2021-04-19 8:27 ` [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2 Christoph Hellwig
` (6 more replies)
0 siblings, 7 replies; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:27 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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(-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
@ 2021-04-19 8:27 ` Christoph Hellwig
2021-04-20 16:26 ` Darrick J. Wong
2021-04-19 8:27 ` [PATCH 2/7] xfs: clean up the EFI and EFD log format handling Christoph Hellwig
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:27 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/7] xfs: clean up the EFI and EFD log format handling
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
2021-04-19 8:27 ` [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2 Christoph Hellwig
@ 2021-04-19 8:27 ` Christoph Hellwig
2021-04-20 17:05 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof Christoph Hellwig
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:27 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
2021-04-19 8:27 ` [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2 Christoph Hellwig
2021-04-19 8:27 ` [PATCH 2/7] xfs: clean up the EFI and EFD log format handling Christoph Hellwig
@ 2021-04-19 8:28 ` Christoph Hellwig
2021-04-20 17:09 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof Christoph Hellwig
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:28 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
` (2 preceding siblings ...)
2021-04-19 8:28 ` [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof Christoph Hellwig
@ 2021-04-19 8:28 ` Christoph Hellwig
2021-04-20 17:10 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper Christoph Hellwig
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:28 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
` (3 preceding siblings ...)
2021-04-19 8:28 ` [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof Christoph Hellwig
@ 2021-04-19 8:28 ` Christoph Hellwig
2021-04-20 17:11 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper Christoph Hellwig
2021-04-19 8:28 ` [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members Christoph Hellwig
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:28 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
` (4 preceding siblings ...)
2021-04-19 8:28 ` [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper Christoph Hellwig
@ 2021-04-19 8:28 ` Christoph Hellwig
2021-04-20 17:12 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members Christoph Hellwig
6 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:28 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
` (5 preceding siblings ...)
2021-04-19 8:28 ` [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper Christoph Hellwig
@ 2021-04-19 8:28 ` Christoph Hellwig
2021-04-20 17:15 ` Darrick J. Wong
2021-04-20 22:16 ` Gustavo A. R. Silva
6 siblings, 2 replies; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-19 8:28 UTC (permalink / raw)
To: linux-xfs; +Cc: Gustavo A . R . Silva
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2
2021-04-19 8:27 ` [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2 Christoph Hellwig
@ 2021-04-20 16:26 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 16:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling
2021-04-19 8:27 ` [PATCH 2/7] xfs: clean up the EFI and EFD log format handling Christoph Hellwig
@ 2021-04-20 17:05 ` Darrick J. Wong
2021-04-21 5:55 ` Christoph Hellwig
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof
2021-04-19 8:28 ` [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof Christoph Hellwig
@ 2021-04-20 17:09 ` Darrick J. Wong
2021-04-21 5:56 ` Christoph Hellwig
0 siblings, 1 reply; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof
2021-04-19 8:28 ` [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof Christoph Hellwig
@ 2021-04-20 17:10 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper
2021-04-19 8:28 ` [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper Christoph Hellwig
@ 2021-04-20 17:11 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:11 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper
2021-04-19 8:28 ` [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper Christoph Hellwig
@ 2021-04-20 17:12 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:12 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members
2021-04-19 8:28 ` [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members Christoph Hellwig
@ 2021-04-20 17:15 ` Darrick J. Wong
2021-04-20 22:16 ` Gustavo A. R. Silva
1 sibling, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-20 17:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members
2021-04-19 8:28 ` [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members Christoph Hellwig
2021-04-20 17:15 ` Darrick J. Wong
@ 2021-04-20 22:16 ` Gustavo A. R. Silva
1 sibling, 0 replies; 20+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-20 22:16 UTC (permalink / raw)
To: Christoph Hellwig, linux-xfs; +Cc: 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);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling
2021-04-20 17:05 ` Darrick J. Wong
@ 2021-04-21 5:55 ` Christoph Hellwig
2021-04-22 0:19 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-21 5:55 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, Gustavo A . R . Silva
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?
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof
2021-04-20 17:09 ` Darrick J. Wong
@ 2021-04-21 5:56 ` Christoph Hellwig
2021-04-22 0:29 ` Darrick J. Wong
0 siblings, 1 reply; 20+ messages in thread
From: Christoph Hellwig @ 2021-04-21 5:56 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs, Gustavo A . R . Silva
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.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/7] xfs: clean up the EFI and EFD log format handling
2021-04-21 5:55 ` Christoph Hellwig
@ 2021-04-22 0:19 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-22 0:19 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof
2021-04-21 5:56 ` Christoph Hellwig
@ 2021-04-22 0:29 ` Darrick J. Wong
0 siblings, 0 replies; 20+ messages in thread
From: Darrick J. Wong @ 2021-04-22 0:29 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs, Gustavo A . R . Silva
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
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2021-04-22 0:29 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 8:27 cleanup the EFI/EFD definitions Christoph Hellwig
2021-04-19 8:27 ` [PATCH 1/7] xfs: remove the EFD size asserts in xlog_recover_efd_commit_pass2 Christoph Hellwig
2021-04-20 16:26 ` Darrick J. Wong
2021-04-19 8:27 ` [PATCH 2/7] xfs: clean up the EFI and EFD log format handling Christoph Hellwig
2021-04-20 17:05 ` Darrick J. Wong
2021-04-21 5:55 ` Christoph Hellwig
2021-04-22 0:19 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof Christoph Hellwig
2021-04-20 17:09 ` Darrick J. Wong
2021-04-21 5:56 ` Christoph Hellwig
2021-04-22 0:29 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 4/7] xfs: pass a xfs_efd_log_item to xfs_efd_item_sizeof Christoph Hellwig
2021-04-20 17:10 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 5/7] xfs: add a xfs_efi_item_sizeof helper Christoph Hellwig
2021-04-20 17:11 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 6/7] xfs: add a xfs_efd_item_sizeof helper Christoph Hellwig
2021-04-20 17:12 ` Darrick J. Wong
2021-04-19 8:28 ` [PATCH 7/7] xfs: Replace one-element arrays with flexible-array members Christoph Hellwig
2021-04-20 17:15 ` Darrick J. Wong
2021-04-20 22:16 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.