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