On Fri, Feb 09, 2018 at 11:12:41AM -0500, Brian Foster wrote:

On Thu, Feb 01, 2018 at 05:42:02PM +1100, Dave Chinner wrote:

quoted

From: Dave Chinner <redacted>

Right now we wait until we've committed changes to the primary
superblock before we initialise any of the new secondary
superblocks. This means that if we have any write errors for new
secondary superblocks we end up with garbage in place rather than
zeros or even an "in progress" superblock to indicate a grow
operation is being done.

To ensure we can write the secondary superblocks, initialise them
earlier in the same loop that initialises the AG headers. We stamp
the new secondary superblocks here with the old geometry, but set
the "sb_inprogress" field to indicate that updates are being done to
the superblock so they cannot be used.  This will result in the
secondary superblock fields being updated or triggering errors that
will abort the grow before we commit any permanent changes.

This also means we can change the update mechanism of the secondary
superblocks.  We know that we are going to wholly overwrite the
information in the struct xfs_sb in the buffer, so there's no point
reading it from disk. Just allocate an uncached buffer, zero it in
memory, stamp the new superblock structure in it and write it out.
If we fail to write it out, then we'll leave the existing sb (old or
new w/ inprogress) on disk for repair to deal with later.

Signed-Off-By: Dave Chinner <redacted>
---
 fs/xfs/xfs_fsops.c | 92 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 55 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 113be7dbdc81..7318cebb591d 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c

...

quoted

@@ -630,43 +653,27 @@ xfs_growfs_imaxpct(
 

...

quoted

 static int
 xfs_growfs_update_superblocks(

...

quoted

 	/* update secondary superblocks. */
 	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
-		error = 0;
-		/*
-		 * new secondary superblocks need to be zeroed, not read from
-		 * disk as the contents of the new area we are growing into is
-		 * completely unknown.
-		 */
-		if (agno < oagcount) {
-			error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
-				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-				  XFS_FSS_TO_BB(mp, 1), 0, &bp,
-				  &xfs_sb_buf_ops);
-		} else {
-			bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
-				  XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
-				  XFS_FSS_TO_BB(mp, 1), 0);
-			if (bp) {
-				bp->b_ops = &xfs_sb_buf_ops;
-				xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
-			} else
-				error = -ENOMEM;
-		}
+		struct xfs_buf		*bp;
 
+		bp = xfs_growfs_get_hdr_buf(mp,
+				XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
+				XFS_FSS_TO_BB(mp, 1), 0, &xfs_sb_buf_ops);

This all seems fine to me up until the point where we use uncached
buffers for pre-existing secondary superblocks. This may all be fine now
if nothing else happens to access/use secondary supers, but it seems
like this essentially enforces that going forward.

Hmm, I see that scrub does appear to look at secondary superblocks via
cached buffers. Shouldn't we expect this path to maintain coherency with
an sb buffer that may have been read/cached from there?

Good catch! I wrote this before scrub started looking at secondary
superblocks. As a general rulle, we don't want to cache secondary
superblocks as they should never be used by the kernel except in
exceptional situations like grow or scrub.

I'll have a look at making this use cached buffers that get freed
immediately after we release them (i.e. don't go onto the LRU) and
that should solve the problem.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com