[RFC 0/5] xfs-4.17: fix v5 AGFL wrapping

[RFC 0/5] xfs-4.17: fix v5 AGFL wrapping

[Darrick J. Wong]

Hi all,

Here's a bunch of patches fixing the AGFL padding problem once and for
all.  When the v5 disk format was rolled out, the AGFL header definition
had a different padding size on 32-bit vs 64-bit systems, with the
result that XFS_AGFL_SIZE reports different maximum lengths depending on
the compiler.  In Linux 4.5 we fixed the structure definition, but this
has lead to sporadic corruption reports on filesystems that were
unmounted with a pre-4.5 kernel and a wrapped AGFL and then remounted on
a 4.5+ kernel.

To deal with these corruption problems, we introduce a new ROCOMPAT
feature bit to indicate that the AGFL has been scanned and guaranteed
not to wrap.  We then amend the mounting code to find broken wrapping,
fix the wrapping, and (unconditionally) set the new ROCOMPAT flag.  This
totally blows since old kernels will no longer mount(!) the filesystem
after we do this, so either we need to have a massive upstream/LTS
kernel/xfsprogs flag day or find a less crappy solution.

No, this is NOT going into 4.16.

--D

[PATCH 1/5] xfs: convert XFS_AGFL_SIZE to a helper function

[Darrick J. Wong]

From: Dave Chinner <dchinner@redhat.com>

The AGFL size calculation is about to get more complex, so lets turn
the macro into a function first and remove the macro.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
[darrick: forward port to newer kernel, simplify the helper]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c  |   31 ++++++++++++++++++++++++-------
 fs/xfs/libxfs/xfs_alloc.h  |    2 ++
 fs/xfs/libxfs/xfs_format.h |   13 +------------
 fs/xfs/scrub/agheader.c    |   10 +++++-----
 fs/xfs/scrub/repair.c      |    2 +-
 fs/xfs/xfs_fsops.c         |    2 +-
 6 files changed, 34 insertions(+), 26 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 6883a76..f3f17a1 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -53,6 +53,23 @@ STATIC int xfs_alloc_ag_vextent_size(xfs_alloc_arg_t *);
 STATIC int xfs_alloc_ag_vextent_small(xfs_alloc_arg_t *,
 		xfs_btree_cur_t *, xfs_agblock_t *, xfs_extlen_t *, int *);
 
+/*
+ * Size of the AGFL.  For CRC-enabled filesystes we steal a couple of slots in
+ * the beginning of the block for a proper header with the location information
+ * and CRC.
+ */
+unsigned int
+xfs_agfl_size(
+	struct xfs_mount	*mp)
+{
+	unsigned int		size = mp->m_sb.sb_sectsize;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb))
+		size -= sizeof(struct xfs_agfl);
+
+	return size / sizeof(xfs_agblock_t);
+}
+
 unsigned int
 xfs_refc_block(
 	struct xfs_mount	*mp)
@@ -550,7 +567,7 @@ xfs_agfl_verify(
 	if (bp->b_pag && be32_to_cpu(agfl->agfl_seqno) != bp->b_pag->pag_agno)
 		return __this_address;
 
-	for (i = 0; i < XFS_AGFL_SIZE(mp); i++) {
+	for (i = 0; i < xfs_agfl_size(mp); i++) {
 		if (be32_to_cpu(agfl->agfl_bno[i]) != NULLAGBLOCK &&
 		    be32_to_cpu(agfl->agfl_bno[i]) >= mp->m_sb.sb_agblocks)
 			return __this_address;
@@ -2266,7 +2283,7 @@ xfs_alloc_get_freelist(
 	bno = be32_to_cpu(agfl_bno[be32_to_cpu(agf->agf_flfirst)]);
 	be32_add_cpu(&agf->agf_flfirst, 1);
 	xfs_trans_brelse(tp, agflbp);
-	if (be32_to_cpu(agf->agf_flfirst) == XFS_AGFL_SIZE(mp))
+	if (be32_to_cpu(agf->agf_flfirst) == xfs_agfl_size(mp))
 		agf->agf_flfirst = 0;
 
 	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
@@ -2377,7 +2394,7 @@ xfs_alloc_put_freelist(
 			be32_to_cpu(agf->agf_seqno), &agflbp)))
 		return error;
 	be32_add_cpu(&agf->agf_fllast, 1);
-	if (be32_to_cpu(agf->agf_fllast) == XFS_AGFL_SIZE(mp))
+	if (be32_to_cpu(agf->agf_fllast) == xfs_agfl_size(mp))
 		agf->agf_fllast = 0;
 
 	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
@@ -2395,7 +2412,7 @@ xfs_alloc_put_freelist(
 
 	xfs_alloc_log_agf(tp, agbp, logflags);
 
-	ASSERT(be32_to_cpu(agf->agf_flcount) <= XFS_AGFL_SIZE(mp));
+	ASSERT(be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp));
 
 	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
 	blockp = &agfl_bno[be32_to_cpu(agf->agf_fllast)];
@@ -2428,9 +2445,9 @@ xfs_agf_verify(
 	if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) &&
 	      XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
 	      be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
-	      be32_to_cpu(agf->agf_flfirst) < XFS_AGFL_SIZE(mp) &&
-	      be32_to_cpu(agf->agf_fllast) < XFS_AGFL_SIZE(mp) &&
-	      be32_to_cpu(agf->agf_flcount) <= XFS_AGFL_SIZE(mp)))
+	      be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
+	      be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
+	      be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp)))
 		return __this_address;
 
 	if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) < 1 ||
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 65a0caf..a311a24 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -26,6 +26,8 @@ struct xfs_trans;
 
 extern struct workqueue_struct *xfs_alloc_wq;
 
+unsigned int xfs_agfl_size(struct xfs_mount *mp);
+
 /*
  * Freespace allocation types.  Argument to xfs_alloc_[v]extent.
  */
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 826296f..bb42a11 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -810,24 +810,13 @@ typedef struct xfs_agi {
 		&(XFS_BUF_TO_AGFL(bp)->agfl_bno[0]) : \
 		(__be32 *)(bp)->b_addr)
 
-/*
- * Size of the AGFL.  For CRC-enabled filesystes we steal a couple of
- * slots in the beginning of the block for a proper header with the
- * location information and CRC.
- */
-#define XFS_AGFL_SIZE(mp) \
-	(((mp)->m_sb.sb_sectsize - \
-	 (xfs_sb_version_hascrc(&((mp)->m_sb)) ? \
-		sizeof(struct xfs_agfl) : 0)) / \
-	  sizeof(xfs_agblock_t))
-
 typedef struct xfs_agfl {
 	__be32		agfl_magicnum;
 	__be32		agfl_seqno;
 	uuid_t		agfl_uuid;
 	__be64		agfl_lsn;
 	__be32		agfl_crc;
-	__be32		agfl_bno[];	/* actually XFS_AGFL_SIZE(mp) */
+	__be32		agfl_bno[];	/* actually xfs_agfl_size(mp) */
 } __attribute__((packed)) xfs_agfl_t;
 
 #define XFS_AGFL_CRC_OFF	offsetof(struct xfs_agfl, agfl_crc)
diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index c2cec24..fbd1744 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -86,7 +86,7 @@ xfs_scrub_walk_agfl(
 	}
 
 	/* first to the end */
-	for (i = flfirst; i < XFS_AGFL_SIZE(mp); i++) {
+	for (i = flfirst; i < xfs_agfl_size(mp); i++) {
 		error = fn(sc, be32_to_cpu(agfl_bno[i]), priv);
 		if (error)
 			return error;
@@ -705,7 +705,7 @@ xfs_scrub_agf(
 	if (agfl_last > agfl_first)
 		fl_count = agfl_last - agfl_first + 1;
 	else
-		fl_count = XFS_AGFL_SIZE(mp) - agfl_first + agfl_last + 1;
+		fl_count = xfs_agfl_size(mp) - agfl_first + agfl_last + 1;
 	if (agfl_count != 0 && fl_count != agfl_count)
 		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
 
@@ -770,7 +770,7 @@ xfs_repair_agf_check_agfl(
 	}
 
 	/* first to the end */
-	for (i = flfirst; i < XFS_AGFL_SIZE(mp); i++) {
+	for (i = flfirst; i < xfs_agfl_size(mp); i++) {
 		bno = be32_to_cpu(agfl_bno[i]);
 		if (!xfs_verify_agbno(mp, sc->sa.agno, bno))
 			return -EFSCORRUPTED;
@@ -1079,7 +1079,7 @@ xfs_scrub_agfl(
 	/* Allocate buffer to ensure uniqueness of AGFL entries. */
 	agf = XFS_BUF_TO_AGF(sc->sa.agf_bp);
 	agflcount = be32_to_cpu(agf->agf_flcount);
-	if (agflcount > XFS_AGFL_SIZE(sc->mp)) {
+	if (agflcount > xfs_agfl_size(sc->mp)) {
 		xfs_scrub_block_set_corrupt(sc, sc->sa.agf_bp);
 		goto out;
 	}
@@ -1286,7 +1286,7 @@ xfs_repair_agfl(
 		trace_xfs_repair_agfl_insert(mp, sc->sa.agno, agbno, rbe->len);
 
 		for (bno = 0; bno < rbe->len; bno++) {
-			if (flcount >= XFS_AGFL_SIZE(mp) - 1)
+			if (flcount >= xfs_agfl_size(mp) - 1)
 				break;
 			agfl_bno[flcount + 1] = cpu_to_be32(agbno + bno);
 			flcount++;
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index bc13aa5..c8d699b 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -604,7 +604,7 @@ xfs_repair_is_block_in_agfl(
 	}
 
 	/* first to the end */
-	for (i = flfirst; i < XFS_AGFL_SIZE(mp); i++) {
+	for (i = flfirst; i < xfs_agfl_size(mp); i++) {
 		if (be32_to_cpu(agfl_bno[i]) == agbno)
 			return true;
 	}
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 84d7383..b1e6292 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -217,7 +217,7 @@ xfs_growfs_data_private(
 		}
 
 		agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, bp);
-		for (bucket = 0; bucket < XFS_AGFL_SIZE(mp); bucket++)
+		for (bucket = 0; bucket < xfs_agfl_size(mp); bucket++)
 			agfl_bno[bucket] = cpu_to_be32(NULLAGBLOCK);
 
 		error = xfs_bwrite(bp);

[PATCH 2/5] xfs: introduce 'fixed agfl' feature

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Introduce a new rocompat flag "FIXED_AGFL" to indicate that we've
scanned and verified that the AGFL does not hit the troublesome last
element of the AGFL array, and update xfs_agfl_size to its new
definition based on this feature.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_format.h |    7 +++++++
 1 file changed, 7 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index bb42a11..3befc92f 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -464,6 +464,7 @@ xfs_sb_has_compat_feature(
 #define XFS_SB_FEAT_RO_COMPAT_FINOBT   (1 << 0)		/* free inode btree */
 #define XFS_SB_FEAT_RO_COMPAT_RMAPBT   (1 << 1)		/* reverse map btree */
 #define XFS_SB_FEAT_RO_COMPAT_REFLINK  (1 << 2)		/* reflinked files */
+#define XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL (1 << 3)	/* fixed agfl */
 #define XFS_SB_FEAT_RO_COMPAT_ALL \
 		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
 		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
@@ -566,6 +567,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
 		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
 }
 
+static inline bool xfs_sb_version_hasfixedagfl(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
+		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL);
+}
+
 /*
  * end of superblock version macros
  */

[PATCH 3/5] xfs: fix agfl wrapping on v5 filesystems

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Introduce helper functions to scan and fix the AGFLs at mount time.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc.c  |  147 ++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_alloc.h  |    3 +
 fs/xfs/libxfs/xfs_format.h |    5 +
 fs/xfs/xfs_mount.c         |   10 +++
 fs/xfs/xfs_super.c         |   10 +++
 5 files changed, 175 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index f3f17a1..9564e34 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -70,6 +70,153 @@ xfs_agfl_size(
 	return size / sizeof(xfs_agblock_t);
 }
 
+/*
+ * Detect and fixup a screwup in the struct xfs_agfl definition that results in
+ * different free list sizes depending on the compiler padding added to the
+ * xfs_agfl. This will only matter on v5 filesystems that have the freelist
+ * wrapping around the end of the AGFL.  If we fix the problem, we log the new
+ * AGF immediately; if the caller fixes all the AGFLs, then it should set the
+ * superblock flag to say we've fixed everything.
+ */
+int
+xfs_agf_fixup_freelist_count(
+	struct xfs_trans	*tp,
+	struct xfs_buf		*agfbp,
+	struct xfs_perag	*pag)
+{
+	struct xfs_mount	*mp = tp->t_mountp;
+	struct xfs_agf		*agf;
+	struct xfs_buf		*agflbp;
+	__be32			*agfl_bno;
+	xfs_agnumber_t		agno;
+	uint32_t		agfl_size;
+	uint32_t		flfirst;
+	uint32_t		fllast;
+	int32_t			active;
+	int			offset;
+	int			error;
+
+	/* v4 or fixed-agfl fs doesn't need fixing */
+	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
+	    xfs_sb_version_hasfixedagfl(&mp->m_sb))
+		return 0;
+
+	agfl_size = xfs_agfl_size(mp);
+
+	agf = XFS_BUF_TO_AGF(agfbp);
+	agno = be32_to_cpu(agf->agf_seqno);
+	flfirst = be32_to_cpu(agf->agf_flfirst);
+	fllast = be32_to_cpu(agf->agf_fllast);
+
+	/*
+	 * In the original v5 code, we had a structure padding error in struct
+	 * xfs_agfl such that XFS_AGFL_SIZE thought the structure size was 36
+	 * on 32-bit and 40 on 64-bit.  On a 512b sector fs this lead to
+	 * ((512 - 36) / 4) = 119 agfl entries on 32bit, whereas on 64-bit
+	 * ((512 - 40) / 4) = 118 agfl entries.  This could lead to problems
+	 * if you migrated filesystems between 32bit and 64bit machines.
+	 *
+	 * In Linux 4.5 we fixed the padding problem such that the structure
+	 * size is always 36 bytes (119 entries in the above example), but
+	 * this still leads to verifier problems because flfirst/fllast have
+	 * to match flcount even if they wrap around the end.  This was also
+	 * problematic because there were v5 filesystems already deployed, and
+	 * some of them have the 40-byte padding (i.e. 118 entries).
+	 *
+	 * Now we're introducing a FIXED_AGFL feature that means that we've
+	 * fixed any wrapping problems and we'll never do that again.  Better
+	 * yet, old kernels won't be touching the AGFL because it's a rocompat
+	 * feature.  We retain the 36-byte padding.
+	 */
+	active = fllast - flfirst + 1;
+	if (active <= 0)
+		active += agfl_size;
+	if (active == pag->pagf_flcount)
+		return 0;
+	else if (active != pag->pagf_flcount + 1)
+		return -EFSCORRUPTED;
+
+	/* Would this have even passed muster on an old system? */
+	if (flfirst >= agfl_size - 1 || fllast >= agfl_size - 1 ||
+	    pag->pagf_flcount > agfl_size - 1)
+		return -EFSCORRUPTED;
+
+	/*
+	 * Convert a 40-byte-padded agfl into a 36-byte-padded AGFL.
+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 2] to bno[flfirst + 1...agfl_size - 1].
+	 *
+	 * Reusing the example above, if we had flfirst == 116, we need
+	 * to move bno[116] and bno[117] into bno[117] and bno[118],
+	 * respectively, and then increment flfirst.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst],
+			(agfl_size - flfirst - 1) * sizeof(xfs_agblock_t));
+	agfl_bno[flfirst] = cpu_to_be32(NULLAGBLOCK);
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	offset = (char *)&agfl_bno[flfirst] - (char *)agflbp->b_addr;
+	xfs_trans_log_buf(tp, agflbp, offset, BBTOB(agflbp->b_length) - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+
+	be32_add_cpu(&agf->agf_flfirst, 1);
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/* Fix all the AGFL counters. */
+int
+xfs_agf_fixup_freelist_counts(
+	struct xfs_mount	*mp)
+{
+	struct xfs_trans	*tp;
+	struct xfs_perag	*pag;
+	struct xfs_buf		*agfbp;
+	xfs_agnumber_t		agno;
+	int			error;
+
+	if (!xfs_sb_version_hascrc(&mp->m_sb) ||
+	    xfs_sb_version_hasfixedagfl(&mp->m_sb))
+		return 0;
+
+	/* Need to be able to log all AGFs + primary super */
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	/* Make sure all the AGFLs do not go off the end of the array. */
+	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
+		error = xfs_alloc_read_agf(mp, tp, agno, 0, &agfbp);
+		if (error)
+			goto cancel;
+		if (!agfbp) {
+			error = -ENOMEM;
+			goto cancel;
+		}
+		pag = xfs_perag_get(mp, agno);
+		error = xfs_agf_fixup_freelist_count(tp, agfbp, pag);
+		xfs_perag_put(pag);
+		xfs_trans_brelse(tp, agfbp);
+		if (error)
+			goto cancel;
+	}
+
+	/* Add the feature flag so we don't have to do this again. */
+	xfs_sb_version_addfixedagfl(&mp->m_sb);
+	xfs_log_sb(tp);
+
+	return xfs_trans_commit(tp);
+
+cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
+
 unsigned int
 xfs_refc_block(
 	struct xfs_mount	*mp)
diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index a311a24..2c79ea4e 100644
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -27,6 +27,9 @@ struct xfs_trans;
 extern struct workqueue_struct *xfs_alloc_wq;
 
 unsigned int xfs_agfl_size(struct xfs_mount *mp);
+int xfs_agf_fixup_freelist_count(struct xfs_trans *tp, struct xfs_buf *agfbp,
+		struct xfs_perag *pag);
+int xfs_agf_fixup_freelist_counts(struct xfs_mount *mp);
 
 /*
  * Freespace allocation types.  Argument to xfs_alloc_[v]extent.
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 3befc92f..e3fdc66 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -573,6 +573,11 @@ static inline bool xfs_sb_version_hasfixedagfl(struct xfs_sb *sbp)
 		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL);
 }
 
+static inline void xfs_sb_version_addfixedagfl(struct xfs_sb *sbp)
+{
+	sbp->sb_features_ro_compat |= XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL;
+}
+
 /*
  * end of superblock version macros
  */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 7f66b96..38d19db 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -874,6 +874,16 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block in a
+	 * troublesome manner.  Don't bother if we're readonly.
+	 */
+	if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
+		error = xfs_agf_fixup_freelist_counts(mp);
+		if (error)
+			goto out_log_dealloc;
+	}
+
+	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index ee75d93..57a0cb1 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1345,6 +1345,16 @@ xfs_fs_remount(
 		}
 
 		/*
+		 * Make sure our AGFL counters do not wrap the end of the block
+		 * in a troublesome manner.
+		 */
+		error = xfs_agf_fixup_freelist_counts(mp);
+		if (error) {
+			xfs_warn(mp, "failed to set FIXED_AGFL feature");
+			return error;
+		}
+
+		/*
 		 * Fill out the reserve pool if it is empty. Use the stashed
 		 * value if it is non-zero, otherwise go with the default.
 		 */

[PATCH 4/5] xfs: enable fixed agfl feature

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Now that we have all the FIXED AGFL bits ready, enable the feature flag.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_format.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index e3fdc66..7cdf66e 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -468,7 +468,8 @@ xfs_sb_has_compat_feature(
 #define XFS_SB_FEAT_RO_COMPAT_ALL \
 		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
 		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
-		 XFS_SB_FEAT_RO_COMPAT_REFLINK)
+		 XFS_SB_FEAT_RO_COMPAT_REFLINK | \
+		 XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL)
 #define XFS_SB_FEAT_RO_COMPAT_UNKNOWN	~XFS_SB_FEAT_RO_COMPAT_ALL
 static inline bool
 xfs_sb_has_ro_compat_feature(

[PATCH 5/5] xfs: scrub should flag (and repair) unfixed agfls

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If somehow we let a fs with an unfixed agfl slip by, we should flag it
and try to repair it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/agheader.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index fbd1744..d548656 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -160,8 +160,13 @@ xfs_scrub_superblock(
 	__be16				vernum_mask;
 
 	agno = sc->sm->sm_agno;
-	if (agno == 0)
+	if (agno == 0) {
+		/* If we somehow don't have a fixed agfl, preen... */
+		if (xfs_sb_version_hascrc(&mp->m_sb) &&
+		    !xfs_sb_version_hasfixedagfl(&mp->m_sb))
+			xfs_scrub_block_set_preen(sc, mp->m_sb_bp);
 		return 0;
+	}
 
 	error = xfs_trans_read_buf(mp, sc->tp, mp->m_ddev_targp,
 		  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
@@ -444,8 +449,13 @@ xfs_repair_superblock(
 
 	/* Don't try to repair AG 0's sb; let xfs_repair deal with it. */
 	agno = sc->sm->sm_agno;
-	if (agno == 0)
+	if (agno == 0) {
+		/* Try to fix the AGFLs if we don't have the feature set */
+		if ((sc->sm->sm_flags & XFS_SCRUB_OFLAG_PREEN) &&
+		    !(sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
+			return xfs_agf_fixup_freelist_counts(mp);
 		return -EOPNOTSUPP;
+	}
 
 	error = xfs_trans_read_buf(mp, sc->tp, mp->m_ddev_targp,
 		  XFS_AG_DADDR(mp, agno, XFS_SB_BLOCK(mp)),

Re: [PATCH 2/5] xfs: introduce 'fixed agfl' feature

[Darrick J. Wong]

On Wed, Jan 17, 2018 at 04:07:23PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Introduce a new rocompat flag "FIXED_AGFL" to indicate that we've
> scanned and verified that the AGFL does not hit the troublesome last
> element of the AGFL array, and update xfs_agfl_size to its new
> definition based on this feature.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index bb42a11..3befc92f 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -464,6 +464,7 @@ xfs_sb_has_compat_feature(
>  #define XFS_SB_FEAT_RO_COMPAT_FINOBT   (1 << 0)		/* free inode btree */
>  #define XFS_SB_FEAT_RO_COMPAT_RMAPBT   (1 << 1)		/* reverse map btree */
>  #define XFS_SB_FEAT_RO_COMPAT_REFLINK  (1 << 2)		/* reflinked files */
> +#define XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL (1 << 3)	/* fixed agfl */

(echoing comments made on irc by myself & Dave)

It suddenly occurred to me that we could do without this rocompat
feature if we simply don't allow wrapped AGFLs anymore.  As soon as
fllast points to that last agfl_bno slot, we simply move the whole list
back to the start of the agfl block (i.e. flfirst = 0) and relog the
whole agfl.  We still have problems if the admin mounts with an
unpatched kernel, gets the agfl to wrap, and then then moves to another
unpatched kernel, but at least the up-to-date kernels can just fix the
problem and move on.

I guess the nice thing about the rocompat feature is that the unpatched
kernels won't mount, instead of blowing up randomly some time in the
future and the user's antiquarian xfsprogs also refuses to touch it.

We could also only set the rocompat flag if we actually had to fix
something (vs. now where we always set it) though I wonder if that just
makes the "I reverted kernels and now it blew up wtf?!" even more
sporadic?  Hm.  Maybe we want that, since either is a rude awakening &
making the rude wakeup less frequent isn't a bad thing.

Ugh.  Maybe bikeshed now? :)

--D

>  #define XFS_SB_FEAT_RO_COMPAT_ALL \
>  		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
>  		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
> @@ -566,6 +567,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
>  		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
>  }
>  
> +static inline bool xfs_sb_version_hasfixedagfl(struct xfs_sb *sbp)
> +{
> +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> +		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL);
> +}
> +
>  /*
>   * end of superblock version macros
>   */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] xfs: introduce 'fixed agfl' feature

[Brian Foster]

On Wed, Jan 17, 2018 at 04:29:40PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 17, 2018 at 04:07:23PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Introduce a new rocompat flag "FIXED_AGFL" to indicate that we've
> > scanned and verified that the AGFL does not hit the troublesome last
> > element of the AGFL array, and update xfs_agfl_size to its new
> > definition based on this feature.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_format.h |    7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index bb42a11..3befc92f 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -464,6 +464,7 @@ xfs_sb_has_compat_feature(
> >  #define XFS_SB_FEAT_RO_COMPAT_FINOBT   (1 << 0)		/* free inode btree */
> >  #define XFS_SB_FEAT_RO_COMPAT_RMAPBT   (1 << 1)		/* reverse map btree */
> >  #define XFS_SB_FEAT_RO_COMPAT_REFLINK  (1 << 2)		/* reflinked files */
> > +#define XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL (1 << 3)	/* fixed agfl */
> 
> (echoing comments made on irc by myself & Dave)
> 
> It suddenly occurred to me that we could do without this rocompat
> feature if we simply don't allow wrapped AGFLs anymore.  As soon as
> fllast points to that last agfl_bno slot, we simply move the whole list
> back to the start of the agfl block (i.e. flfirst = 0) and relog the
> whole agfl.  We still have problems if the admin mounts with an
> unpatched kernel, gets the agfl to wrap, and then then moves to another
> unpatched kernel, but at least the up-to-date kernels can just fix the
> problem and move on.
> 

Eh, I'm not a huge fan of modifying AGFL behavior to deal with a padding
bug. It strikes me as overkill and undue risk (depending on how the code
would have to look I suppose). 

> I guess the nice thing about the rocompat feature is that the unpatched
> kernels won't mount, instead of blowing up randomly some time in the
> future and the user's antiquarian xfsprogs also refuses to touch it.
> 
> We could also only set the rocompat flag if we actually had to fix
> something (vs. now where we always set it) though I wonder if that just
> makes the "I reverted kernels and now it blew up wtf?!" even more
> sporadic?  Hm.  Maybe we want that, since either is a rude awakening &
> making the rude wakeup less frequent isn't a bad thing.
> 
> Ugh.  Maybe bikeshed now? :)
> 

Any reason we couldn't do something like force a read-only mount for
affected filesystems with a notice/msg to run xfs_repair (I assume we've
already backported fixes to repair/stable kernels to help prevent
reoccurence of this..)? Then we'd at least only affect users who are
affected by the problem, and IIUC it sounds like a corruption error is
imminent for those particular users as it is.

Brian

> --D
> 
> >  #define XFS_SB_FEAT_RO_COMPAT_ALL \
> >  		(XFS_SB_FEAT_RO_COMPAT_FINOBT | \
> >  		 XFS_SB_FEAT_RO_COMPAT_RMAPBT | \
> > @@ -566,6 +567,12 @@ static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
> >  		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
> >  }
> >  
> > +static inline bool xfs_sb_version_hasfixedagfl(struct xfs_sb *sbp)
> > +{
> > +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> > +		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FIXED_AGFL);
> > +}
> > +
> >  /*
> >   * end of superblock version macros
> >   */
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html