xfs: fix various problems

From: Darrick J. Wong <[email protected]>

In xfs_qm_dqalloc, we join the locked quota inode to the transaction we use to allocate blocks. If the allocation or mapping fails, we're not allowed to unlock the inode because the transaction code is in charge of unlocking it for us. Therefore, remove the iunlock call to avoid blowing asserts about unbalanced locking + mount hang.

Found by corrupting the AGF and allocating space in the filesystem (quotacheck) immediately after mount. The upcoming agfl wrapping fixup test will trigger this scenario.

Signed-off-by Darrick J. Wong <[email protected]>

---

 fs/xfs/xfs_dquot.c |    2 --
 1 file changed, 2 deletions(-)

fs/xfs/xfs_dquot.c +0 -3

index 43572f8..2410acc 100644
@@ -394,8 +394,6 @@ xfs_qm_dqalloc(
 error1:
 	xfs_defer_cancel(&dfops);
 error0:
-	xfs_iunlock(quotip, XFS_ILOCK_EXCL);
-
 	return error;
 }
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Yet another round of playing whack-a-mole with directory code that asserts on corrupt on-disk metadata when it really should be returning -EFSCORRUPTED instead of ASSERTing. Found by a xfs/391 crash while lastbit fuzzing of ltail.bestcount.

Signed-off-by Darrick J. Wong <[email protected]>

---

 fs/xfs/libxfs/xfs_dir2_leaf.c |    3 ++-
 fs/xfs/libxfs/xfs_dir2_node.c |    5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

fs/xfs/libxfs/xfs_dir2_leaf.c +2 -1

index d7e630f..d61d52d 100644
@@ -1415,7 +1415,8 @@ xfs_dir2_leaf_removename(
 	oldbest = be16_to_cpu(bf[0].length);
 	ltp = xfs_dir2_leaf_tail_p(args->geo, leaf);
 	bestsp = xfs_dir2_leaf_bests_p(ltp);
-	ASSERT(be16_to_cpu(bestsp[db]) == oldbest);
+	if (be16_to_cpu(bestsp[db]) != oldbest)
+		return -EFSCORRUPTED;
 	/*
 	 * Mark the former data entry unused.
 	 */

fs/xfs/libxfs/xfs_dir2_node.c +3 -3

index 239d97a..0839ffe 100644
@@ -387,8 +387,9 @@ xfs_dir2_leaf_to_node(
 	dp->d_ops->free_hdr_from_disk(&freehdr, free);
 	leaf = lbp->b_addr;
 	ltp = xfs_dir2_leaf_tail_p(args->geo, leaf);
-	ASSERT(be32_to_cpu(ltp->bestcount) <=
-				(uint)dp->i_d.di_size / args->geo->blksize);
+	if (be32_to_cpu(ltp->bestcount) >
+				(uint)dp->i_d.di_size / args->geo->blksize)
+		return -EFSCORRUPTED;
 
 	/*
 	 * Copy freespace entries from the leaf block to the new block.
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

There's no point in allocating a transaction and locking the inode in preparation to clear cow blocks if there actually are any cow fork extents. Therefore, move the xfs_reflink_cancel_cow_range hunk to xfs_inactive and check the cow ifp first. This makes inode reclamation run faster.

Signed-off-by Darrick J. Wong <[email protected]>

---

 fs/xfs/xfs_inode.c |    5 +++++
 fs/xfs/xfs_super.c |    9 ---------
 2 files changed, 5 insertions(+), 9 deletions(-)

fs/xfs/xfs_inode.c +5 -0

index 604ee38..50fbbf5 100644
@@ -1872,6 +1872,7 @@ xfs_inactive(
 	xfs_inode_t	*ip)
 {
 	struct xfs_mount	*mp;
+	struct xfs_ifork	*cow_ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
 	int			error;
 	int			truncate = 0;
 

@@ -1892,6 +1893,10 @@ xfs_inactive(
 	if (mp->m_flags & XFS_MOUNT_RDONLY)
 		return;
 
+	/* Try to clean out the cow blocks if there are any. */
+	if (xfs_is_reflink_inode(ip) && cow_ifp->if_bytes > 0)
+		xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, true);
+
 	if (VFS_I(ip)->i_nlink != 0) {
 		/*
 		 * force is true because we are evicting an inode from the

fs/xfs/xfs_super.c +0 -10

index 7aba628..624a802 100644
@@ -970,7 +970,6 @@ xfs_fs_destroy_inode(
 	struct inode		*inode)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
-	int			error;
 
 	trace_xfs_destroy_inode(ip);
 

@@ -978,14 +977,6 @@ xfs_fs_destroy_inode(
 	XFS_STATS_INC(ip->i_mount, vn_rele);
 	XFS_STATS_INC(ip->i_mount, vn_remove);
 
-	if (xfs_is_reflink_inode(ip)) {
-		error = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, true);
-		if (error && !XFS_FORCED_SHUTDOWN(ip->i_mount))
-			xfs_warn(ip->i_mount,
-"Error %d while evicting CoW blocks for inode %llu.",
-					error, ip->i_ino);
-	}
-
 	xfs_inactive(ip);
 
 	ASSERT(XFS_FORCED_SHUTDOWN(ip->i_mount) || ip->i_delayed_blks == 0);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Dave Chinner <[email protected]>

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 <[email protected]>

[darrick: forward port to newer kernel, simplify the helper]

Signed-off-by Darrick J. Wong <[email protected]>

---

 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    |    6 +++---
 fs/xfs/xfs_fsops.c         |    2 +-
 5 files changed, 31 insertions(+), 23 deletions(-)

fs/xfs/libxfs/xfs_alloc.c +24 -7

index c02781a..36101e5 100644
@@ -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 ||

fs/xfs/libxfs/xfs_alloc.h +2 -0

index 65a0caf..a311a24 100644
@@ -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.
  */

fs/xfs/libxfs/xfs_format.h +1 -12

index 1acb584..42956d8 100644
@@ -803,24 +803,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)

fs/xfs/scrub/agheader.c +3 -3

index fd97552..fd383c5 100644
@@ -80,7 +80,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;

@@ -664,7 +664,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);
 

@@ -791,7 +791,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;
 	}

fs/xfs/xfs_fsops.c +1 -2

index 8b45456..5237927 100644
@@ -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);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +310 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"
+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_trans.h"
+#include "xfs_fixups.h"
+
+/*
+ * v5 AGFL padding defects
+ *
+ * When the v5 format was first introduced, there was a defect in the struct
+ * xfs_agfl definition that resulted in XFS_AGFL_SIZE returning different
+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.
+ *
+ * Therefore, we add two fixup functions -- the first runs at mount time to
+ * detect a short-wrapped AGFL and fix it; the second runs at unmount, freeze,
+ * or remount-ro time to move a wrapped AGFL to the beginning of the list.
+ * This reduces the likelihood of a screwup to the scenario where you have (a)
+ * a filesystem with no post-4.5 features (reflink, rmap), (b) the AGFL wraps,
+ * (c) the filesystem goes down leaving a dirty log, and (d) the dirty
+ * filesystem is mounted on an old kernel.
+ */
+
+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);
+
+	/* Make sure we're either spot on or off by 1. */
+	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);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;
+
+	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);
+
+	/* Empty AGFL?  Make sure we aren't pointing at the end. */
+	if (pag->pagf_flcount == 0) {
+		if (flfirst >= agfl_size || fllast >= agfl_size) {
+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+		}
+		return 0;
+	}
+
+	/* If we don't hit the end, we're done. */
+	if (flfirst < fllast && fllast != agfl_size - 1)
+		return 0;
+
+	/*
+	 * Move a start of a wrapped list towards the start of the agfl block.
+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	if (fllast == agfl_size - 1) {
+		/* Back the AGFL off from the end of the block. */
+		len = sizeof(xfs_agblock_t);
+		agfl_bno[flfirst - 1] = agfl_bno[agfl_size - 1];
+		offset = (char *)&agfl_bno[flfirst - 1] - (char *)agflbp->b_addr;
+		be32_add_cpu(&agf->agf_fllast, -1);
+		be32_add_cpu(&agf->agf_flfirst, -1);
+	} else {
+		/* Move the first part of the AGFL towards the front. */
+		len = (agfl_size - flfirst) * sizeof(xfs_agblock_t);
+		memcpy(&agfl_bno[fllast + 1], &agfl_bno[flfirst], len);
+		offset = (char *)&agfl_bno[fllast + 1] - (char *)agflbp->b_addr;
+		agf->agf_flfirst = 0;
+		agf->agf_fllast = cpu_to_be32(pag->pagf_flcount - 1);
+	}
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+
+	return 0;
+}
+
+typedef int (*xfs_agf_apply_fn_t)(struct xfs_trans *tp, struct xfs_buf *agfbp,
+		struct xfs_perag *pag);
+
+/* Apply something to every AGF. */
+STATIC int
+xfs_fixup_agf_apply(
+	struct xfs_mount	*mp,
+	xfs_agf_apply_fn_t	fn)
+{
+	struct xfs_trans	*tp;
+	struct xfs_perag	*pag;
+	struct xfs_buf		*agfbp;
+	xfs_agnumber_t		agno;
+	int			error;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	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 = fn(tp, agfbp, pag);
+		xfs_perag_put(pag);
+		xfs_trans_brelse(tp, agfbp);
+		if (error)
+			goto cancel;
+	}
+
+	return xfs_trans_commit(tp);
+cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
+
+/* Fix AGFL wrapping so we can use the filesystem. */
+int
+xfs_fixup_agfl_wrap_mount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_mount);
+}
+
+/* Fix AGFL wrapping so old kernels can use this filesystem. */
+int
+xfs_fixup_agfl_wrap_unmount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_unmount);
+}

fs/xfs/xfs_fixups.h new file +26 -0

new file mode 100644
index 0000000..fb52a96
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#ifndef	__XFS_FIXUPS_H__
+#define	__XFS_FIXUPS_H__
+
+int xfs_fixup_agfl_wrap_mount(struct xfs_mount *mp);
+int xfs_fixup_agfl_wrap_unmount(struct xfs_mount *mp);
+
+#endif /* __XFS_FIXUPS_H__ */

fs/xfs/xfs_mount.c +20 -1

index 98fd41c..eb284aa 100644
@@ -46,7 +46,7 @@
 #include "xfs_refcount_btree.h"
 #include "xfs_reflink.h"
 #include "xfs_extent_busy.h"
-
+#include "xfs_fixups.h"
 
 static DEFINE_MUTEX(xfs_uuid_table_mutex);
 static int xfs_uuid_table_size;

@@ -875,6 +875,16 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner.
+	 */
+	error = xfs_fixup_agfl_wrap_mount(mp);
+	if (error) {
+		xfs_warn(mp, "Failed to fix agfl wrapping.  Run xfs_repair.");
+		goto out_log_dealloc;
+	}
+
+	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */

@@ -1128,6 +1138,15 @@ xfs_unmountfs(
 	xfs_qm_unmount(mp);
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
+	/*
 	 * Unreserve any blocks we have so that when we unmount we don't account
 	 * the reserved free space as used. This is really only necessary for
 	 * lazy superblock counting because it trusts the incore superblock

fs/xfs/xfs_super.c +10 -1

index 624a802..d9aa39a 100644
@@ -50,6 +50,7 @@
 #include "xfs_refcount_item.h"
 #include "xfs_bmap_item.h"
 #include "xfs_reflink.h"
+#include "xfs_fixups.h"
 
 #include <linux/namei.h>
 #include <linux/dax.h>

@@ -1206,6 +1207,15 @@ xfs_quiesce_attr(
 	xfs_reclaim_inodes(mp, 0);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
 
+	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
 	/* Push the superblock and write an unmount record */
 	error = xfs_log_sbcount(mp);
 	if (error)
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +254 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"
+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_trans.h"
+#include "xfs_fixups.h"
+
+/*
+ * v5 AGFL padding defects
+ *
+ * When the v5 format was first introduced, there was a defect in the struct
+ * xfs_agfl definition that resulted in XFS_AGFL_SIZE returning different
+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.
+ *
+ * Therefore, we add two fixup functions -- the first runs at mount time to
+ * detect a short-wrapped AGFL and fix it; the second runs at unmount, freeze,
+ * or remount-ro time to move a wrapped AGFL to the beginning of the list.
+ * This reduces the likelihood of a screwup to the scenario where you have (a)
+ * a filesystem with no post-4.5 features (reflink, rmap), (b) the AGFL wraps,
+ * (c) the filesystem goes down leaving a dirty log, and (d) the dirty
+ * filesystem is mounted on an old kernel.
+ */
+
+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);
+
+	/* Make sure we're either spot on or off by 1. */
+	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);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;
+
+	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);
+
+	/* Empty AGFL?  Make sure we aren't pointing at the end. */
+	if (pag->pagf_flcount == 0) {
+		if (flfirst >= agfl_size || fllast >= agfl_size) {
+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+		}
+		return 0;
+	}
+
+	/* If we don't hit the end, we're done. */
+	if (flfirst < fllast && fllast != agfl_size - 1)
+		return 0;
+
+	/*
+	 * Move a start of a wrapped list towards the start of the agfl block.
+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	if (fllast == agfl_size - 1) {
+		/* Back the AGFL off from the end of the block. */
+		len = sizeof(xfs_agblock_t);
+		agfl_bno[flfirst - 1] = agfl_bno[agfl_size - 1];
+		offset = (char *)&agfl_bno[flfirst - 1] - (char *)agflbp->b_addr;
+		be32_add_cpu(&agf->agf_fllast, -1);
+		be32_add_cpu(&agf->agf_flfirst, -1);
+	} else {
+		/* Move the first part of the AGFL towards the front. */
+		len = (agfl_size - flfirst) * sizeof(xfs_agblock_t);
+		memcpy(&agfl_bno[fllast + 1], &agfl_bno[flfirst], len);
+		offset = (char *)&agfl_bno[fllast + 1] - (char *)agflbp->b_addr;
+		agf->agf_flfirst = 0;
+		agf->agf_fllast = cpu_to_be32(pag->pagf_flcount - 1);
+	}
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+
+	return 0;
+}
+
+typedef int (*xfs_agf_apply_fn_t)(struct xfs_trans *tp, struct xfs_buf *agfbp,
+		struct xfs_perag *pag);
+
+/* Apply something to every AGF. */
+STATIC int
+xfs_fixup_agf_apply(

fs/xfs/xfs_fixups.c new file +46 -0

+	if (error)
+		return error;
+
+	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 = fn(tp, agfbp, pag);
+		xfs_perag_put(pag);
+		xfs_trans_brelse(tp, agfbp);
+		if (error)
+			goto cancel;
+	}
+
+	return xfs_trans_commit(tp);
+cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
+
+/* Fix AGFL wrapping so we can use the filesystem. */
+int
+xfs_fixup_agfl_wrap_mount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_mount);
+}
+
+/* Fix AGFL wrapping so old kernels can use this filesystem. */
+int
+xfs_fixup_agfl_wrap_unmount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_unmount);
+}

fs/xfs/xfs_fixups.h new file +26 -0

new file mode 100644
index 0000000..fb52a96
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#ifndef	__XFS_FIXUPS_H__
+#define	__XFS_FIXUPS_H__
+
+int xfs_fixup_agfl_wrap_mount(struct xfs_mount *mp);
+int xfs_fixup_agfl_wrap_unmount(struct xfs_mount *mp);
+
+#endif /* __XFS_FIXUPS_H__ */

fs/xfs/xfs_mount.c +20 -1

index 98fd41c..eb284aa 100644
@@ -46,7 +46,7 @@
 #include "xfs_refcount_btree.h"
 #include "xfs_reflink.h"
 #include "xfs_extent_busy.h"
-
+#include "xfs_fixups.h"
 
 static DEFINE_MUTEX(xfs_uuid_table_mutex);
 static int xfs_uuid_table_size;

@@ -875,6 +875,16 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner.
+	 */
+	error = xfs_fixup_agfl_wrap_mount(mp);
+	if (error) {
+		xfs_warn(mp, "Failed to fix agfl wrapping.  Run xfs_repair.");
+		goto out_log_dealloc;
+	}
+
+	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */

@@ -1128,6 +1138,15 @@ xfs_unmountfs(
 	xfs_qm_unmount(mp);
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
+	/*
 	 * Unreserve any blocks we have so that when we unmount we don't account
 	 * the reserved free space as used. This is really only necessary for
 	 * lazy superblock counting because it trusts the incore superblock

fs/xfs/xfs_super.c +10 -1

index 624a802..d9aa39a 100644
@@ -50,6 +50,7 @@
 #include "xfs_refcount_item.h"
 #include "xfs_bmap_item.h"
 #include "xfs_reflink.h"
+#include "xfs_fixups.h"
 
 #include <linux/namei.h>
 #include <linux/dax.h>

@@ -1206,6 +1207,15 @@ xfs_quiesce_attr(
 	xfs_reclaim_inodes(mp, 0);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
 
+	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
 	/* Push the superblock and write an unmount record */
 	error = xfs_log_sbcount(mp);
 	if (error)
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +26 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"

fs/xfs/xfs_fixups.c new file +16 -0

+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.

fs/xfs/xfs_fixups.c new file +57 -0

+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);

fs/xfs/xfs_fixups.c new file +41 -0

+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;

fs/xfs/xfs_fixups.c new file +4 -0

+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);

fs/xfs/xfs_fixups.c new file +9 -0

+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.

fs/xfs/xfs_fixups.c new file +73 -0

+	}
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+
+	return 0;
+}
+
+typedef int (*xfs_agf_apply_fn_t)(struct xfs_trans *tp, struct xfs_buf *agfbp,
+		struct xfs_perag *pag);
+
+/* Apply something to every AGF. */
+STATIC int
+xfs_fixup_agf_apply(
+	struct xfs_mount	*mp,
+	xfs_agf_apply_fn_t	fn)
+{
+	struct xfs_trans	*tp;
+	struct xfs_perag	*pag;
+	struct xfs_buf		*agfbp;
+	xfs_agnumber_t		agno;
+	int			error;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	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 = fn(tp, agfbp, pag);
+		xfs_perag_put(pag);
+		xfs_trans_brelse(tp, agfbp);
+		if (error)
+			goto cancel;
+	}
+
+	return xfs_trans_commit(tp);
+cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
+
+/* Fix AGFL wrapping so we can use the filesystem. */
+int
+xfs_fixup_agfl_wrap_mount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_mount);
+}
+
+/* Fix AGFL wrapping so old kernels can use this filesystem. */
+int
+xfs_fixup_agfl_wrap_unmount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_unmount);
+}

fs/xfs/xfs_fixups.h new file +26 -0

new file mode 100644
index 0000000..fb52a96
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#ifndef	__XFS_FIXUPS_H__
+#define	__XFS_FIXUPS_H__
+
+int xfs_fixup_agfl_wrap_mount(struct xfs_mount *mp);
+int xfs_fixup_agfl_wrap_unmount(struct xfs_mount *mp);
+
+#endif /* __XFS_FIXUPS_H__ */

fs/xfs/xfs_mount.c +20 -1

index 98fd41c..eb284aa 100644
@@ -46,7 +46,7 @@
 #include "xfs_refcount_btree.h"
 #include "xfs_reflink.h"
 #include "xfs_extent_busy.h"
-
+#include "xfs_fixups.h"
 
 static DEFINE_MUTEX(xfs_uuid_table_mutex);
 static int xfs_uuid_table_size;

@@ -875,6 +875,16 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner.
+	 */
+	error = xfs_fixup_agfl_wrap_mount(mp);
+	if (error) {
+		xfs_warn(mp, "Failed to fix agfl wrapping.  Run xfs_repair.");
+		goto out_log_dealloc;
+	}
+
+	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */

@@ -1128,6 +1138,15 @@ xfs_unmountfs(
 	xfs_qm_unmount(mp);
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
+	/*
 	 * Unreserve any blocks we have so that when we unmount we don't account
 	 * the reserved free space as used. This is really only necessary for
 	 * lazy superblock counting because it trusts the incore superblock

fs/xfs/xfs_super.c +10 -1

index 624a802..d9aa39a 100644
@@ -50,6 +50,7 @@
 #include "xfs_refcount_item.h"
 #include "xfs_bmap_item.h"
 #include "xfs_reflink.h"
+#include "xfs_fixups.h"
 
 #include <linux/namei.h>
 #include <linux/dax.h>

@@ -1206,6 +1207,15 @@ xfs_quiesce_attr(
 	xfs_reclaim_inodes(mp, 0);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
 
+	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
 	/* Push the superblock and write an unmount record */
 	error = xfs_log_sbcount(mp);
 	if (error)
 
--

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 63 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +31 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"
+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_trans.h"
+#include "xfs_fixups.h"
+

fs/xfs/xfs_fixups.c line 36

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +5 -0

+/*
+ * v5 AGFL padding defects
+ *
+ * When the v5 format was first introduced, there was a defect in the struct
+ * xfs_agfl definition that resulted in XFS_AGFL_SIZE returning different

XFS_AGFL_SIZE() no longer exists as of the previous patch. It might be better to size refer to "the agfl size" generically.

Quoted 16 lines

fs/xfs/xfs_fixups.c new file +16 -0

+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.

fs/xfs/xfs_fixups.c line 62

Quoted 10 lines

fs/xfs/xfs_fixups.c new file +10 -0

+ *
+ * Therefore, we add two fixup functions -- the first runs at mount time to
+ * detect a short-wrapped AGFL and fix it; the second runs at unmount, freeze,
+ * or remount-ro time to move a wrapped AGFL to the beginning of the list.
+ * This reduces the likelihood of a screwup to the scenario where you have (a)
+ * a filesystem with no post-4.5 features (reflink, rmap), (b) the AGFL wraps,
+ * (c) the filesystem goes down leaving a dirty log, and (d) the dirty
+ * filesystem is mounted on an old kernel.
+ */
+

While the mount vs. unmount time fixups serve different purposes and have different conditions, do we really need two separate fixup implementations? E.g., if we have to unwrap the AGFL at unmount time, why not just reuse that fixup at mount time (when needed) as well? I suspect the difference would only be the length of what we can consider valid in the agfl at the time.

Quoted 63 lines

fs/xfs/xfs_fixups.c new file +63 -0

+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);
+
+	/* Make sure we're either spot on or off by 1. */
+	active = fllast - flfirst + 1;
+	if (active <= 0)
+		active += agfl_size;
+	if (active == pag->pagf_flcount)

fs/xfs/xfs_fixups.c line 129

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		return 0;
+	else if (active != pag->pagf_flcount + 1)
+		return -EFSCORRUPTED;
+

So we're not attempting to cover the case where the agfl has 1 more block than the agfl size (i.e., the case where an fs goes back to a kernel with an unpacked header)?

I'm wondering whether using the unmount algorithm in both places (as noted above) would also facilitate this mechanism to work both ways (unpacked <-> packed), provided particular conditions are met. I suppose that could be considered regardless, but the less variance the better IMO.

fs/xfs/xfs_fixups.c line 130

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	/* Would this have even passed muster on an old system? */

Comment doesn't really explain what is going on here..?

fs/xfs/xfs_fixups.c line 134

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	if (flfirst >= agfl_size - 1 || fllast >= agfl_size - 1 ||
+	    pag->pagf_flcount > agfl_size - 1)
+		return -EFSCORRUPTED;
+

I take it these are checks for whether the agfl was previously corrupted based on the unpacked size? FWIW, it might be a bit more clear to use an old/unpacked_agfl_size variable to declare intent (here and in some of the comments and whatnot that follow).

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	/*
+	 * 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].

fs/xfs/xfs_fixups.c line 143

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +5 -0

+	 *
+	 * 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.
+	 */

Kind of a strange example to use given that it doesn't mention wrapping.. this only triggers if the agfl was previously wrapped, right?

Quoted 45 lines

fs/xfs/xfs_fixups.c new file +45 -0

+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;
+
+	agfl_size = xfs_agfl_size(mp);
+	agf = XFS_BUF_TO_AGF(agfbp);
+	agno = be32_to_cpu(agf->agf_seqno);

fs/xfs/xfs_fixups.c line 194

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	flfirst = be32_to_cpu(agf->agf_flfirst);
+	fllast = be32_to_cpu(agf->agf_fllast);
+
+	/* Empty AGFL?  Make sure we aren't pointing at the end. */
+	if (pag->pagf_flcount == 0) {
+		if (flfirst >= agfl_size || fllast >= agfl_size) {

When would either field be >= agfl_size? Isn't that where they wrap?

Quoted 14 lines

fs/xfs/xfs_fixups.c new file +13 -0

+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+		}
+		return 0;
+	}
+
+	/* If we don't hit the end, we're done. */
+	if (flfirst < fllast && fllast != agfl_size - 1)
+		return 0;
+
+	/*

fs/xfs/xfs_fixups.c line 208

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	 * Move a start of a wrapped list towards the start of the agfl block.

FWIW, this and the subsequent comments kind of gave me the impression that the agfl would be "shifted" towards the start of the block. The code doesn't do that, but rather rotates the start/head of the agfl to the tail and adjusts the pointers (changing the effective order). That seems technically Ok, but I had to grok the code and read back to understand the comment rather than the other way around.

Quoted 13 lines

fs/xfs/xfs_fixups.c new file +13 -0

+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;

fs/xfs/xfs_fixups.c line 227

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	if (fllast == agfl_size - 1) {
+		/* Back the AGFL off from the end of the block. */
+		len = sizeof(xfs_agblock_t);
+		agfl_bno[flfirst - 1] = agfl_bno[agfl_size - 1];

What if the agfl is full (flfirst == 0)?

fs/xfs/xfs_fixups.c line 231

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		offset = (char *)&agfl_bno[flfirst - 1] - (char *)agflbp->b_addr;
+		be32_add_cpu(&agf->agf_fllast, -1);
+		be32_add_cpu(&agf->agf_flfirst, -1);
+	} else {

fs/xfs/xfs_fixups.c line 237

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+		/* Move the first part of the AGFL towards the front. */
+		len = (agfl_size - flfirst) * sizeof(xfs_agblock_t);
+		memcpy(&agfl_bno[fllast + 1], &agfl_bno[flfirst], len);
+		offset = (char *)&agfl_bno[fllast + 1] - (char *)agflbp->b_addr;
+		agf->agf_flfirst = 0;
+		agf->agf_fllast = cpu_to_be32(pag->pagf_flcount - 1);

Similar question here as above.. it looks like the copy may be a no-op, but we still reset flfirst/fllast (which may also be fine, but unnecessary).

The more interesting case here is flcount == 1 and flfirst == fllast, where it looks like we'd wrongly reset first/last (and perhaps copy more than we should..?).

Ugh. Remind me again why we can't just detect a mismatch, fail the mount and tell the user to repair? :P

Random thought... have we considered anything less invasive/more simple for this issue than low level buffer manipulation? For example, could we address the unpacked -> packed case by just allocating a block to fill the gap at the end of the wrapped agfl?

Perhaps that could even be done more discreetly in xfs_alloc_fix_freelist() when we already have a transaction set up, etc.

That wouldn't help going the other direction, but maybe we could consider similar logic to free an extraneous block and then worry about protecting users who won't upgrade broken kernels separately if that really continues to be a problem.

Brian

Quoted 181 lines

fs/xfs/xfs_fixups.c new file +73 -0

+	}
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+
+	return 0;
+}
+
+typedef int (*xfs_agf_apply_fn_t)(struct xfs_trans *tp, struct xfs_buf *agfbp,
+		struct xfs_perag *pag);
+
+/* Apply something to every AGF. */
+STATIC int
+xfs_fixup_agf_apply(
+	struct xfs_mount	*mp,
+	xfs_agf_apply_fn_t	fn)
+{
+	struct xfs_trans	*tp;
+	struct xfs_perag	*pag;
+	struct xfs_buf		*agfbp;
+	xfs_agnumber_t		agno;
+	int			error;
+
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_growdata, 0, 0, 0, &tp);
+	if (error)
+		return error;
+
+	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 = fn(tp, agfbp, pag);
+		xfs_perag_put(pag);
+		xfs_trans_brelse(tp, agfbp);
+		if (error)
+			goto cancel;
+	}
+
+	return xfs_trans_commit(tp);
+cancel:
+	xfs_trans_cancel(tp);
+	return error;
+}
+
+/* Fix AGFL wrapping so we can use the filesystem. */
+int
+xfs_fixup_agfl_wrap_mount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_mount);
+}
+
+/* Fix AGFL wrapping so old kernels can use this filesystem. */
+int
+xfs_fixup_agfl_wrap_unmount(
+	struct xfs_mount	*mp)
+{
+	if (!xfs_sb_version_needs_agfl_wrap_fixes(&mp->m_sb))
+		return 0;
+
+	return xfs_fixup_agf_apply(mp, xfs_fixup_freelist_wrap_unmount);
+}

fs/xfs/xfs_fixups.h new file +26 -0

new file mode 100644
index 0000000..fb52a96
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#ifndef	__XFS_FIXUPS_H__
+#define	__XFS_FIXUPS_H__
+
+int xfs_fixup_agfl_wrap_mount(struct xfs_mount *mp);
+int xfs_fixup_agfl_wrap_unmount(struct xfs_mount *mp);
+
+#endif /* __XFS_FIXUPS_H__ */

fs/xfs/xfs_mount.c +20 -1

index 98fd41c..eb284aa 100644
@@ -46,7 +46,7 @@
 #include "xfs_refcount_btree.h"
 #include "xfs_reflink.h"
 #include "xfs_extent_busy.h"
-
+#include "xfs_fixups.h"
 
 static DEFINE_MUTEX(xfs_uuid_table_mutex);
 static int xfs_uuid_table_size;

@@ -875,6 +875,16 @@ xfs_mountfs(
 	}
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner.
+	 */
+	error = xfs_fixup_agfl_wrap_mount(mp);
+	if (error) {
+		xfs_warn(mp, "Failed to fix agfl wrapping.  Run xfs_repair.");
+		goto out_log_dealloc;
+	}
+
+	/*
 	 * Get and sanity-check the root inode.
 	 * Save the pointer to it in the mount structure.
 	 */

@@ -1128,6 +1138,15 @@ xfs_unmountfs(
 	xfs_qm_unmount(mp);
 
 	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
+	/*
 	 * Unreserve any blocks we have so that when we unmount we don't account
 	 * the reserved free space as used. This is really only necessary for
 	 * lazy superblock counting because it trusts the incore superblock

fs/xfs/xfs_super.c +10 -1

index 624a802..d9aa39a 100644
@@ -50,6 +50,7 @@
 #include "xfs_refcount_item.h"
 #include "xfs_bmap_item.h"
 #include "xfs_reflink.h"
+#include "xfs_fixups.h"
 
 #include <linux/namei.h>
 #include <linux/dax.h>

@@ -1206,6 +1207,15 @@ xfs_quiesce_attr(
 	xfs_reclaim_inodes(mp, 0);
 	xfs_reclaim_inodes(mp, SYNC_WAIT);
 
+	/*
+	 * Make sure our AGFL counters do not wrap the end of the block
+	 * in a troublesome manner for old kernels.
+	 */
+	error = xfs_fixup_agfl_wrap_unmount(mp);
+	if (error)
+		xfs_warn(mp, "Unable to fix agfl wrapping.  "
+				"This may cause problems on next mount.");
+
 	/* Push the superblock and write an unmount record */
 	error = xfs_log_sbcount(mp);
 	if (error)
 
--

Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 64 lines · Brian Foster

Quoted 63 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +31 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"
+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_trans.h"
+#include "xfs_fixups.h"
+

Quoted 8 lines

fs/xfs/xfs_fixups.c line 36

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +5 -0

+/*
+ * v5 AGFL padding defects
+ *
+ * When the v5 format was first introduced, there was a defect in the struct
+ * xfs_agfl definition that resulted in XFS_AGFL_SIZE returning different

XFS_AGFL_SIZE() no longer exists as of the previous patch. It might be better to size refer to "the agfl size" generically.

How about:

"...the agfl length calculation formula (at the time known as XFS_AGFL_SIZE)..."

Quoted 27 lines

Quoted 16 lines

fs/xfs/xfs_fixups.c new file +16 -0

+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.

fs/xfs/xfs_fixups.c line 62

Quoted 10 lines

fs/xfs/xfs_fixups.c new file +10 -0

+ *
+ * Therefore, we add two fixup functions -- the first runs at mount time to
+ * detect a short-wrapped AGFL and fix it; the second runs at unmount, freeze,
+ * or remount-ro time to move a wrapped AGFL to the beginning of the list.
+ * This reduces the likelihood of a screwup to the scenario where you have (a)
+ * a filesystem with no post-4.5 features (reflink, rmap), (b) the AGFL wraps,
+ * (c) the filesystem goes down leaving a dirty log, and (d) the dirty
+ * filesystem is mounted on an old kernel.
+ */
+

Quoted 6 lines

While the mount vs. unmount time fixups serve different purposes and have different conditions, do we really need two separate fixup implementations? E.g., if we have to unwrap the AGFL at unmount time, why not just reuse that fixup at mount time (when needed) as well? I suspect the difference would only be the length of what we can consider valid in the agfl at the time.

They're doing two different things. I will use ASCII art to demonstrate the mount-time function:

Let's say the AGFL block has space for 10 items. It looks like this:

         0   1   2   3   4   5   6   7   8   9

Header | | | | | | | | | | |

When the fs is freshly formatted or repaired, the AGFL will look like:

         0   1   2   3   4   5   6   7   8   9

Header | A | B | C | D | E | F | | | | |
^-- flfirst ^-- fllast

Due to the padding problems prior to 4.5 ("4.5 agfl padding fix"), XFS_AGFL_SIZE would return 10 on 32-bit systems and 9 on 64-bit systems. Therefore, if the AGFL wrapped on a 64-bit kernel we would end up with:

         0   1   2   3   4   5   6   7   8   9

Header | D | E | F | | | | A | B | C | ? |
^-- fllast ^-- flfirst

Note that block 9's contents are undefined because we didn't write anything there. Now the "4.5 agfl padding fix" has corrected XFS_AGFL_SIZE to return 10 in all cases, this looks like an AGF corruption because flcount is 6 but the distance between flfirst and fllast is 7. So long as the list wraps and the mismatch is exactly one block we can fix this. The mount time fixer in this patch does this by moving the records between flfirst and xfs_agfl_size (A, B, and C) to the end of the block to close the gap:

         0   1   2   3   4   5   6   7   8   9

Header | D | E | F | | | | | A | B | C |
^-- fllast ^-- flfirst

If the count is off by more than 1 then the AGF is truly corrupt and we bail out.

Now let's say that we run for a while and want to unmount, but our AGFL wraps like this:

         0   1   2   3   4   5   6   7   8   9

Header | T | U | V | | | | | Q | R | S |
^-- fllast ^-- flfirst

We don't know that the next kernel to mount this filesystem will have the "4.5 agfl padding fix" applied to it; if it does not, it will flag the AGF as corrupted because flcount is 6 but in its view the distance between flfirst and fllast (which omits bno[9]) is 5. We don't want it to choke on that, so the unmount fixer moves all the records between flfirst and the end (Q, R, and S) towards the start of the block and resets flfirst/fllast:

         0   1   2   3   4   5   6   7   8   9

Header | T | U | V | Q | R | S | | | | |
^-- flfirst ^-- fllast

Since the AGFL no longer wraps at all, it doesn't matter if the next kernel to mount this filesystem has the "4.5 agfl padding fix" applied; all kernels can handle this correctly.

Quoted 63 lines

Quoted 63 lines

fs/xfs/xfs_fixups.c new file +63 -0

+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);
+
+	/* Make sure we're either spot on or off by 1. */
+	active = fllast - flfirst + 1;
+	if (active <= 0)
+		active += agfl_size;
+	if (active == pag->pagf_flcount)

Quoted 8 lines

fs/xfs/xfs_fixups.c line 129

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		return 0;
+	else if (active != pag->pagf_flcount + 1)
+		return -EFSCORRUPTED;
+

So we're not attempting to cover the case where the agfl has 1 more block than the agfl size (i.e., the case where an fs goes back to a kernel with an unpacked header)?

We don't know how the next kernel to touch this filesystem will define XFS_AGFL_SIZE -- it could be a 4.5+ kernel (same agfl size), a 32-bit pre-4.5 kernel (same agfl size), or a 64-bit pre-4.5 kernel (small agfl size).

Quoted 5 lines

I'm wondering whether using the unmount algorithm in both places (as noted above) would also facilitate this mechanism to work both ways (unpacked <-> packed), provided particular conditions are met. I suppose that could be considered regardless, but the less variance the better IMO.

In principle I suppose we could have a single function to handle all the rearranging, but I'd rather have two functions to handle the two cases. I'm open to refactoring the common parts out of the xfs_fixup_freelist_wrap_*mount functions.

Quoted 3 lines

fs/xfs/xfs_fixups.c line 130

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	/* Would this have even passed muster on an old system? */

Comment doesn't really explain what is going on here..?

/*
* If the distance between flfirst and fllast mismatches flcount by more
* than 1, then there's more wrong with this agfl than just the padding
* problem. Bail out completely, which will force the admin to run
* xfs_repair.
*/

Quoted 10 lines

fs/xfs/xfs_fixups.c line 134

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	if (flfirst >= agfl_size - 1 || fllast >= agfl_size - 1 ||
+	    pag->pagf_flcount > agfl_size - 1)
+		return -EFSCORRUPTED;
+

I take it these are checks for whether the agfl was previously corrupted based on the unpacked size? FWIW, it might be a bit more clear to use an old/unpacked_agfl_size variable to declare intent (here and in some of the comments and whatnot that follow).

Ok.

Quoted 4 lines

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	/*
+	 * 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].

Quoted 8 lines

fs/xfs/xfs_fixups.c line 143

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +5 -0

+	 *
+	 * 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.
+	 */

Kind of a strange example to use given that it doesn't mention wrapping.. this only triggers if the agfl was previously wrapped, right?

Right. I used to have a check that if the list didn't wrap then we would just exit because no fixing is required... but it must've fallen out. I agree that we don't want to rely on a subtlety here to stay out of here if the agfl wasn't initially wrapped.

Quoted 45 lines

Quoted 45 lines

fs/xfs/xfs_fixups.c new file +45 -0

+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;
+
+	agfl_size = xfs_agfl_size(mp);
+	agf = XFS_BUF_TO_AGF(agfbp);
+	agno = be32_to_cpu(agf->agf_seqno);

Quoted 8 lines

fs/xfs/xfs_fixups.c line 194

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	flfirst = be32_to_cpu(agf->agf_flfirst);
+	fllast = be32_to_cpu(agf->agf_fllast);
+
+	/* Empty AGFL?  Make sure we aren't pointing at the end. */
+	if (pag->pagf_flcount == 0) {
+		if (flfirst >= agfl_size || fllast >= agfl_size) {

When would either field be >= agfl_size? Isn't that where they wrap?

Oops, you're right, that should be flfirst >= agfl_size - 1.

Or really,

old_agfl_size = agfl_size - 1
if flfirst >= old_agfl_size or fllast >= old_agfl_size:
blahblahblah

Quoted 14 lines

Quoted 14 lines

fs/xfs/xfs_fixups.c new file +13 -0

+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+		}
+		return 0;
+	}
+
+	/* If we don't hit the end, we're done. */
+	if (flfirst < fllast && fllast != agfl_size - 1)
+		return 0;
+
+	/*

Quoted 8 lines

fs/xfs/xfs_fixups.c line 208

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	 * Move a start of a wrapped list towards the start of the agfl block.

FWIW, this and the subsequent comments kind of gave me the impression that the agfl would be "shifted" towards the start of the block. The code doesn't do that, but rather rotates the start/head of the agfl to the tail and adjusts the pointers (changing the effective order). That seems technically Ok, but I had to grok the code and read back to understand the comment rather than the other way around.

Yeah. Sorry about the confusion, maybe it would be better if I took out these confusing comments and replaced it all with the ascii art above?

Quoted 13 lines

Quoted 13 lines

fs/xfs/xfs_fixups.c new file +13 -0

+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;

Quoted 8 lines

fs/xfs/xfs_fixups.c line 227

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	if (fllast == agfl_size - 1) {
+		/* Back the AGFL off from the end of the block. */
+		len = sizeof(xfs_agblock_t);
+		agfl_bno[flfirst - 1] = agfl_bno[agfl_size - 1];

What if the agfl is full (flfirst == 0)?

I don't think we ever unmount with a full agfl -- we're guaranteed at least 118 items in the AGFL, and the maximum size the agfl will ever need is 8 blocks for each of bnobt, cntbt, rmapbt.

However, we could just detect a full agfl and free the end block.

Quoted 4 lines

fs/xfs/xfs_fixups.c line 231

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		offset = (char *)&agfl_bno[flfirst - 1] - (char *)agflbp->b_addr;
+		be32_add_cpu(&agf->agf_fllast, -1);
+		be32_add_cpu(&agf->agf_flfirst, -1);
+	} else {

This needs to check for flfirst > fllast.

Quoted 11 lines

fs/xfs/xfs_fixups.c line 237

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+		/* Move the first part of the AGFL towards the front. */
+		len = (agfl_size - flfirst) * sizeof(xfs_agblock_t);
+		memcpy(&agfl_bno[fllast + 1], &agfl_bno[flfirst], len);
+		offset = (char *)&agfl_bno[fllast + 1] - (char *)agflbp->b_addr;
+		agf->agf_flfirst = 0;
+		agf->agf_fllast = cpu_to_be32(pag->pagf_flcount - 1);

Similar question here as above.. it looks like the copy may be a no-op, but we still reset flfirst/fllast (which may also be fine, but unnecessary).

Quoted 6 lines

The more interesting case here is flcount == 1 and flfirst == fllast, where it looks like we'd wrongly reset first/last (and perhaps copy more than we should..?).

Ugh. Remind me again why we can't just detect a mismatch, fail the mount and tell the user to repair? :P

Not all the distros included xfs_repair in the initrd or the logic to call it (as opposed to "fsck -y") if the root fs fails to mount, which means that if this wrapping problem hits a root filesystem then the administrator will have to reboot with a recovery iso and run xfs_repair from there. That's going to generate a /lot/ of support work compared to us figuring out how to repair the filesystems automatically.

Quoted 4 lines

Random thought... have we considered anything less invasive/more simple for this issue than low level buffer manipulation? For example, could we address the unpacked -> packed case by just allocating a block to fill the gap at the end of the wrapped agfl?

The allocation could fail, in which case we'd be forced to resort to list manipulation.

Quoted 3 lines

Perhaps that could even be done more discreetly in xfs_alloc_fix_freelist() when we already have a transaction set up, etc.

Possible, though this will add more logic to every hot allocation call for something that only needs fixing at mount and at unmount. I think there's a stronger argument for doing the unmount cleanup on every fix_freelist since that would eliminate the small chance that a new kernel crashes and gets recovered on an old kernel.

Quoted 4 lines

That wouldn't help going the other direction, but maybe we could consider similar logic to free an extraneous block and then worry about protecting users who won't upgrade broken kernels separately if that really continues to be a problem.

It is already a problem here.

Quoted 74 lines · Darrick J. Wong

Quoted 72 lines · Brian Foster

Quoted 58 lines · Darrick J. Wong

From: Darrick J. Wong <[email protected]>

Signed-off-by Darrick J. Wong <[email protected]>

---
 fs/xfs/Makefile     |    1 
 fs/xfs/xfs_fixups.c |  310 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fixups.h |   26 ++++
 fs/xfs/xfs_mount.c  |   21 +++
 fs/xfs/xfs_super.c  |   10 ++
 5 files changed, 367 insertions(+), 1 deletion(-)
 create mode 100644 fs/xfs/xfs_fixups.c
 create mode 100644 fs/xfs/xfs_fixups.h

fs/xfs/Makefile +1 -0

index b03c77e..f88368a 100644
@@ -86,6 +86,7 @@ xfs-y				+= xfs_aops.o \
 				   xfs_extent_busy.o \
 				   xfs_file.o \
 				   xfs_filestream.o \
+				   xfs_fixups.o \
 				   xfs_fsmap.o \
 				   xfs_fsops.o \
 				   xfs_globals.o \

fs/xfs/xfs_fixups.c new file +26 -0

new file mode 100644
index 0000000..0cad7bb
@@ -0,0 +1,310 @@
+/*
+ * Copyright (C) 2018 Oracle.  All Rights Reserved.
+ *
+ * Author: Darrick J. Wong <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_shared.h"
+#include "xfs_format.h"
+#include "xfs_log_format.h"
+#include "xfs_trans_resv.h"
+#include "xfs_sb.h"

fs/xfs/xfs_fixups.c line 36

Quoted 10 lines

fs/xfs/xfs_fixups.c new file +10 -0

+#include "xfs_mount.h"
+#include "xfs_alloc.h"
+#include "xfs_trans.h"
+#include "xfs_fixups.h"
+
+/*
+ * v5 AGFL padding defects
+ *
+ * When the v5 format was first introduced, there was a defect in the struct
+ * xfs_agfl definition that resulted in XFS_AGFL_SIZE returning different

XFS_AGFL_SIZE() no longer exists as of the previous patch. It might be better to size refer to "the agfl size" generically.

Quoted 5 lines

How about:

"...the agfl length calculation formula (at the time known as XFS_AGFL_SIZE)..."

The formula formerly known as..? ;) Heh, sounds fine.

Quoted 34 lines

Quoted 27 lines

Quoted 16 lines

fs/xfs/xfs_fixups.c new file +16 -0

+ * values depending on the compiler padding.  On a fs with 512-byte sectors,
+ * this meant that XFS_AGFL_SIZE was 119 on i386, but 118 on x64.  Commit
+ * 96f859d52bcb1 ("libxfs: pack the agfl header structure so XFS_AGFL_SIZE is
+ * correct") changed the definition to disable padding the end of the
+ * structure, and was accepted into Linux 4.5.  Since then, the AGFL has
+ * always used the larger size (e.g. 119 entries on a 512b sector fs).
+ *
+ * Unfortunately, pre-4.5 kernels can produce filesystems with AGFLs that wrap
+ * at the smaller size, and those kernels are not prepared to handle the
+ * longer size.  This typically manifests itself as an AGF verifier corruption
+ * error followed by a filesystem shutdown.  While we encourage admins to stay
+ * current with software, we would like to avoid this intermittent breakage.
+ *
+ * Any v5 filesystem which has a feature bit set for a feature that was
+ * introduced after Linux 4.5 will not have this problem, as such kernels
+ * cannot be mounted on older kernels.  v4 filesystems are also unaffected.

fs/xfs/xfs_fixups.c line 62

Quoted 10 lines

fs/xfs/xfs_fixups.c new file +10 -0

+ *
+ * Therefore, we add two fixup functions -- the first runs at mount time to
+ * detect a short-wrapped AGFL and fix it; the second runs at unmount, freeze,
+ * or remount-ro time to move a wrapped AGFL to the beginning of the list.
+ * This reduces the likelihood of a screwup to the scenario where you have (a)
+ * a filesystem with no post-4.5 features (reflink, rmap), (b) the AGFL wraps,
+ * (c) the filesystem goes down leaving a dirty log, and (d) the dirty
+ * filesystem is mounted on an old kernel.
+ */
+

Quoted 6 lines

While the mount vs. unmount time fixups serve different purposes and have different conditions, do we really need two separate fixup implementations? E.g., if we have to unwrap the AGFL at unmount time, why not just reuse that fixup at mount time (when needed) as well? I suspect the difference would only be the length of what we can consider valid in the agfl at the time.

Quoted 3 lines

They're doing two different things. I will use ASCII art to demonstrate the mount-time function:

Yes, the question was whether we need to do two different things. For example...

Quoted 51 lines

Let's say the AGFL block has space for 10 items. It looks like this:

         0   1   2   3   4   5   6   7   8   9

Header | | | | | | | | | | |

When the fs is freshly formatted or repaired, the AGFL will look like:

         0   1   2   3   4   5   6   7   8   9

Header | A | B | C | D | E | F | | | | |
^-- flfirst ^-- fllast

Due to the padding problems prior to 4.5 ("4.5 agfl padding fix"), XFS_AGFL_SIZE would return 10 on 32-bit systems and 9 on 64-bit systems. Therefore, if the AGFL wrapped on a 64-bit kernel we would end up with:

         0   1   2   3   4   5   6   7   8   9

Header | D | E | F | | | | A | B | C | ? |
^-- fllast ^-- flfirst

Note that block 9's contents are undefined because we didn't write anything there. Now the "4.5 agfl padding fix" has corrected XFS_AGFL_SIZE to return 10 in all cases, this looks like an AGF corruption because flcount is 6 but the distance between flfirst and fllast is 7. So long as the list wraps and the mismatch is exactly one block we can fix this. The mount time fixer in this patch does this by moving the records between flfirst and xfs_agfl_size (A, B, and C) to the end of the block to close the gap:

         0   1   2   3   4   5   6   7   8   9

Header | D | E | F | | | | | A | B | C |
^-- fllast ^-- flfirst

If the count is off by more than 1 then the AGF is truly corrupt and we bail out.

Now let's say that we run for a while and want to unmount, but our AGFL wraps like this:

         0   1   2   3   4   5   6   7   8   9

Header | T | U | V | | | | | Q | R | S |
^-- fllast ^-- flfirst

We don't know that the next kernel to mount this filesystem will have the "4.5 agfl padding fix" applied to it; if it does not, it will flag the AGF as corrupted because flcount is 6 but in its view the distance between flfirst and fllast (which omits bno[9]) is 5. We don't want it to choke on that, so the unmount fixer moves all the records between flfirst and the end (Q, R, and S) towards the start of the block and resets flfirst/fllast:

Quoted 4 lines

         0   1   2   3   4   5   6   7   8   9

Header | T | U | V | Q | R | S | | | | |
^-- flfirst ^-- fllast

Could we use this basic algorithm at mount time as well? I know it wouldn't be _exactly_ the same operation at mount time as it is for unmount since slot 9 is a gap in the former case, but afaict the only difference from an algorithmic perspective is the length of the shift.

IOW, if we were to parameterize the length of the shift and have the mount fixup call the unmount algorithm, would it not also address the problem?

Quoted 76 lines

Since the AGFL no longer wraps at all, it doesn't matter if the next kernel to mount this filesystem has the "4.5 agfl padding fix" applied; all kernels can handle this correctly.

Quoted 63 lines

Quoted 63 lines

fs/xfs/xfs_fixups.c new file +63 -0

+/*
+ * Decide if we need to have the agfl wrapping fixes applied.  This only
+ * affects v5 filesystems that do not have any features enabled that did not
+ * exist when the agfl padding fix went in.
+ *
+ * Features already present when the fix went in were finobt, ftype, spinodes.
+ * If we see something new (e.g. reflink) then don't bother.
+ */
+#define XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_RO_COMPAT_FINOBT))
+#define XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED \
+		(~(XFS_SB_FEAT_INCOMPAT_FTYPE | \
+		   XFS_SB_FEAT_INCOMPAT_SPINODES))
+#define XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED \
+		(~0)
+static inline bool xfs_sb_version_needs_agfl_wrap_fixes(struct xfs_sb *sbp)
+{
+	return xfs_sb_version_hascrc(sbp) &&
+		!xfs_sb_has_incompat_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_ro_compat_feature(sbp,
+			XFS_SB_FEAT_RO_COMPAT_AGFL_WRAP_ALREADY_FIXED) &&
+		!xfs_sb_has_incompat_log_feature(sbp,
+			XFS_SB_FEAT_INCOMPAT_LOG_AGFL_WRAP_ALREADY_FIXED);
+}
+
+/*
+ * Fix an AGFL wrapping that falls short of the end of the block by filling the
+ * gap at the end of the block.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_mount(
+	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			len;
+	int			error;
+
+	if (pag->pagf_flcount == 0)
+		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);
+
+	/* Make sure we're either spot on or off by 1. */
+	active = fllast - flfirst + 1;
+	if (active <= 0)
+		active += agfl_size;
+	if (active == pag->pagf_flcount)

Quoted 8 lines

fs/xfs/xfs_fixups.c line 129

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		return 0;
+	else if (active != pag->pagf_flcount + 1)
+		return -EFSCORRUPTED;
+

So we're not attempting to cover the case where the agfl has 1 more block than the agfl size (i.e., the case where an fs goes back to a kernel with an unpacked header)?

Quoted 5 lines

We don't know how the next kernel to touch this filesystem will define XFS_AGFL_SIZE -- it could be a 4.5+ kernel (same agfl size), a 32-bit pre-4.5 kernel (same agfl size), or a 64-bit pre-4.5 kernel (small agfl size).

I don't think I was clear.. I'm envisioning whether we could come up with a patch that would generically fix up the agfl on disk to be sane relative to the current kernel. This patch covers the case of a packed agfl kernel mounting an unpacked on-disk agfl. It would be nice if we could implement something that also handled a packed on-disk agfl to an unpacked agfl kernel (for easier backport to unpacked kernels, for e.g.).

Quoted 11 lines

Quoted 5 lines

I'm wondering whether using the unmount algorithm in both places (as noted above) would also facilitate this mechanism to work both ways (unpacked <-> packed), provided particular conditions are met. I suppose that could be considered regardless, but the less variance the better IMO.

In principle I suppose we could have a single function to handle all the rearranging, but I'd rather have two functions to handle the two cases. I'm open to refactoring the common parts out of the xfs_fixup_freelist_wrap_*mount functions.

As noted above, I'm not really commenting on the function factoring here as much as the use of multiple algorithms (using one would certainly facilitate some mechanical refactoring).

Quoted 11 lines

Quoted 3 lines

fs/xfs/xfs_fixups.c line 130

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	/* Would this have even passed muster on an old system? */

Comment doesn't really explain what is going on here..?

/*
* If the distance between flfirst and fllast mismatches flcount by more
* than 1, then there's more wrong with this agfl than just the padding
* problem. Bail out completely, which will force the admin to run
* xfs_repair.
*/

*nod*

Quoted 116 lines

Quoted 10 lines

fs/xfs/xfs_fixups.c line 134

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	if (flfirst >= agfl_size - 1 || fllast >= agfl_size - 1 ||
+	    pag->pagf_flcount > agfl_size - 1)
+		return -EFSCORRUPTED;
+

I take it these are checks for whether the agfl was previously corrupted based on the unpacked size? FWIW, it might be a bit more clear to use an old/unpacked_agfl_size variable to declare intent (here and in some of the comments and whatnot that follow).

Ok.

Quoted 4 lines

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+	/*
+	 * 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].

Quoted 8 lines

fs/xfs/xfs_fixups.c line 143

Quoted 5 lines

fs/xfs/xfs_fixups.c new file +5 -0

+	 *
+	 * 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.
+	 */

Kind of a strange example to use given that it doesn't mention wrapping.. this only triggers if the agfl was previously wrapped, right?

Right. I used to have a check that if the list didn't wrap then we would just exit because no fixing is required... but it must've fallen out. I agree that we don't want to rely on a subtlety here to stay out of here if the agfl wasn't initially wrapped.

Quoted 45 lines

Quoted 45 lines

fs/xfs/xfs_fixups.c new file +45 -0

+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;
+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	len = (agfl_size - flfirst - 1) * sizeof(xfs_agblock_t);
+	memmove(&agfl_bno[flfirst + 1], &agfl_bno[flfirst], len);
+	offset = (char *)&agfl_bno[flfirst + 1] - (char *)agflbp->b_addr;
+	be32_add_cpu(&agf->agf_flfirst, 1);
+
+	xfs_trans_buf_set_type(tp, agflbp, XFS_BLFT_AGFL_BUF);
+	xfs_trans_log_buf(tp, agflbp, offset, offset + len - 1);
+	xfs_trans_brelse(tp, agflbp);
+	agflbp = NULL;
+	xfs_alloc_log_agf(tp, agfbp, XFS_AGF_FLFIRST);
+
+	return 0;
+}
+
+/*
+ * Fix an AGFL that touches the end of the block by moving the first or last
+ * part of the list elsewhere in the AGFL so that old kernels don't trip over
+ * wrapping issues.
+ */
+STATIC int
+xfs_fixup_freelist_wrap_unmount(
+	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;
+	int			offset;
+	int			len;
+	int			error;
+
+	agfl_size = xfs_agfl_size(mp);
+	agf = XFS_BUF_TO_AGF(agfbp);
+	agno = be32_to_cpu(agf->agf_seqno);

Quoted 8 lines

fs/xfs/xfs_fixups.c line 194

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	flfirst = be32_to_cpu(agf->agf_flfirst);
+	fllast = be32_to_cpu(agf->agf_fllast);
+
+	/* Empty AGFL?  Make sure we aren't pointing at the end. */
+	if (pag->pagf_flcount == 0) {
+		if (flfirst >= agfl_size || fllast >= agfl_size) {

When would either field be >= agfl_size? Isn't that where they wrap?

Oops, you're right, that should be flfirst >= agfl_size - 1.

Or really,

old_agfl_size = agfl_size - 1
if flfirst >= old_agfl_size or fllast >= old_agfl_size:
blahblahblah

Quoted 14 lines

Quoted 14 lines

fs/xfs/xfs_fixups.c new file +13 -0

+			agf->agf_flfirst = cpu_to_be32(1);
+			agf->agf_fllast = 0;
+			xfs_alloc_log_agf(tp, agfbp,
+					XFS_AGF_FLFIRST | XFS_AGF_FLLAST);
+		}
+		return 0;
+	}
+
+	/* If we don't hit the end, we're done. */
+	if (flfirst < fllast && fllast != agfl_size - 1)
+		return 0;
+
+	/*

Quoted 8 lines

fs/xfs/xfs_fixups.c line 208

Quoted 1 line

fs/xfs/xfs_fixups.c new file +1 -0

+	 * Move a start of a wrapped list towards the start of the agfl block.

FWIW, this and the subsequent comments kind of gave me the impression that the agfl would be "shifted" towards the start of the block. The code doesn't do that, but rather rotates the start/head of the agfl to the tail and adjusts the pointers (changing the effective order). That seems technically Ok, but I had to grok the code and read back to understand the comment rather than the other way around.

Quoted 3 lines

Yeah. Sorry about the confusion, maybe it would be better if I took out these confusing comments and replaced it all with the ascii art above?

I'm still hoping we can do something a bit more simple and flexible in general, but that aside, the ascii art describes the associated algorithms much better.

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

fs/xfs/xfs_fixups.c new file +13 -0

+	 * Therefore, we need to move the AGFL blocks
+	 * bno[flfirst..agfl_size - 1] to bno[fllast + 1...agfl_size - flfirst].
+	 * Then we reset flfirst and fllast appropriately.
+	 *
+	 * Reusing the example above, if we had flfirst == 117 and fllast == 4,
+	 * we need to move bno[117] and bno[118] into bno[5] and bno[6],
+	 * respectively, and then reset flfirst and fllast.
+	 *
+	 * If it's just the last block that touches the end, only move that.
+	 */
+	error = xfs_alloc_read_agfl(mp, tp, agno, &agflbp);
+	if (error)
+		return error;

Quoted 8 lines

fs/xfs/xfs_fixups.c line 227

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
+
+	if (fllast == agfl_size - 1) {
+		/* Back the AGFL off from the end of the block. */
+		len = sizeof(xfs_agblock_t);
+		agfl_bno[flfirst - 1] = agfl_bno[agfl_size - 1];

What if the agfl is full (flfirst == 0)?

Quoted 6 lines

I don't think we ever unmount with a full agfl -- we're guaranteed at least 118 items in the AGFL, and the maximum size the agfl will ever need is 8 blocks for each of bnobt, cntbt, rmapbt.

However, we could just detect a full agfl and free the end block.

I suppose it's unlikely we'll ever see corner cases such as a completely full or empty agfl. That said, I think the code needs to handle those cases sanely. That could mean explicit checks and skipping the fixup for all I care, just as long as we don't have memory corruption vectors and whatnot if (when?) assumptions prove wrong.

Quoted 25 lines

Quoted 4 lines

fs/xfs/xfs_fixups.c line 231

Quoted 4 lines

fs/xfs/xfs_fixups.c new file +4 -0

+		offset = (char *)&agfl_bno[flfirst - 1] - (char *)agflbp->b_addr;
+		be32_add_cpu(&agf->agf_fllast, -1);
+		be32_add_cpu(&agf->agf_flfirst, -1);
+	} else {

This needs to check for flfirst > fllast.

Quoted 11 lines

fs/xfs/xfs_fixups.c line 237

Quoted 6 lines

fs/xfs/xfs_fixups.c new file +6 -0

+		/* Move the first part of the AGFL towards the front. */
+		len = (agfl_size - flfirst) * sizeof(xfs_agblock_t);
+		memcpy(&agfl_bno[fllast + 1], &agfl_bno[flfirst], len);
+		offset = (char *)&agfl_bno[fllast + 1] - (char *)agflbp->b_addr;
+		agf->agf_flfirst = 0;
+		agf->agf_fllast = cpu_to_be32(pag->pagf_flcount - 1);

Similar question here as above.. it looks like the copy may be a no-op, but we still reset flfirst/fllast (which may also be fine, but unnecessary).

Quoted 6 lines

The more interesting case here is flcount == 1 and flfirst == fllast, where it looks like we'd wrongly reset first/last (and perhaps copy more than we should..?).

Ugh. Remind me again why we can't just detect a mismatch, fail the mount and tell the user to repair? :P

Quoted 7 lines

Not all the distros included xfs_repair in the initrd or the logic to call it (as opposed to "fsck -y") if the root fs fails to mount, which means that if this wrapping problem hits a root filesystem then the administrator will have to reboot with a recovery iso and run xfs_repair from there. That's going to generate a /lot/ of support work compared to us figuring out how to repair the filesystems automatically.

Yeah, sorry. Good argument for the unpacked -> packed fixup.

Quoted 8 lines

Quoted 4 lines

Random thought... have we considered anything less invasive/more simple for this issue than low level buffer manipulation? For example, could we address the unpacked -> packed case by just allocating a block to fill the gap at the end of the wrapped agfl?

The allocation could fail, in which case we'd be forced to resort to list manipulation.

Yeah.. obvious tradeoff, but how likely is that? It seems a reasonable tradeoff to me given there's still the repair path. Also note that we don't have to necessarily allocate from the btrees.

(FWIW, I hacked around on this a bit and ended up with something slightly different[1].)

Quoted 10 lines

Quoted 3 lines

Perhaps that could even be done more discreetly in xfs_alloc_fix_freelist() when we already have a transaction set up, etc.

Possible, though this will add more logic to every hot allocation call for something that only needs fixing at mount and at unmount. I think there's a stronger argument for doing the unmount cleanup on every fix_freelist since that would eliminate the small chance that a new kernel crashes and gets recovered on an old kernel.

Eh, I don't think that's anything we couldn't address if need be. E.g., amortize the padding fixup against actual allocations, set/test a pag bit to track the padding fixup, or check it on agf read and set a "needs fixup" bit, etc.

Quoted 7 lines

Quoted 4 lines

That wouldn't help going the other direction, but maybe we could consider similar logic to free an extraneous block and then worry about protecting users who won't upgrade broken kernels separately if that really continues to be a problem.

It is already a problem here.

Then it should be able to stand alone as an independent patch (with independent commit log that explains/justifies it, etc.).

Brian