Re: [PATCH 7/7] xfs: rework secondary superblock updates in growfs

[Dave Chinner <david@fromorbit.com>]

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:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > 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 <dchinner@redhat.com>
> > ---
> >  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
> ...
> > @@ -630,43 +653,27 @@ xfs_growfs_imaxpct(
> >  
> ...
> >  static int
> >  xfs_growfs_update_superblocks(
> ...
> >  	/* 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