Re: [PATCH] xfs: use ordered buffers to initialize dquot buffers during quotacheck

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, May 13, 2020 at 10:06:28AM +1000, Dave Chinner wrote:
> On Tue, May 12, 2020 at 02:00:33PM -0700, Darrick J. Wong wrote:
> > @@ -277,11 +279,34 @@ xfs_qm_init_dquot_blk(
> >  		}
> >  	}
> >  
> > -	xfs_trans_dquot_buf(tp, bp,
> > -			    (type & XFS_DQ_USER ? XFS_BLF_UDQUOT_BUF :
> > -			    ((type & XFS_DQ_PROJ) ? XFS_BLF_PDQUOT_BUF :
> > -			     XFS_BLF_GDQUOT_BUF)));
> > -	xfs_trans_log_buf(tp, bp, 0, BBTOB(q->qi_dqchunklen) - 1);
> > +	if (type & XFS_DQ_USER) {
> > +		qflag = XFS_UQUOTA_CHKD;
> > +		blftype = XFS_BLF_UDQUOT_BUF;
> > +	} else if (type & XFS_DQ_PROJ) {
> > +		qflag = XFS_PQUOTA_CHKD;
> > +		blftype = XFS_BLF_PDQUOT_BUF;
> > +	} else {
> > +		qflag = XFS_GQUOTA_CHKD;
> > +		blftype = XFS_BLF_GDQUOT_BUF;
> > +	}
> > +
> > +	xfs_trans_dquot_buf(tp, bp, blftype);
> > +
> > +	/*
> > +	 * If the CHKD flag isn't set, we're running quotacheck and need to use
> > +	 * ordered buffers so that the logged initialization buffer does not
> > +	 * get replayed over the delwritten quotacheck buffer.  If we crash
> > +	 * before the end of quotacheck, the CHKD flags will not be set in the
> > +	 * superblock and we'll re-run quotacheck at next mount.
> > +	 *
> > +	 * Outside of quotacheck, dquot updates are logged via dquot items and
> > +	 * we must use the regular buffer logging mechanisms to ensure that the
> > +	 * initial buffer state is recovered before dquot items.
> > +	 */
> > +	if (mp->m_qflags & qflag)
> > +		xfs_trans_log_buf(tp, bp, 0, BBTOB(q->qi_dqchunklen) - 1);
> > +	else
> > +		xfs_trans_ordered_buf(tp, bp);
> >  }
> 
> That comment is ... difficult to understand. It conflates what we
> are currently doing with what might happen in future if we did
> something differently at the current time. IIUC, what you actually
> mean is this:
> 
> 	/*
> 	 * When quotacheck runs, we use delayed writes to update all the dquots
> 	 * on disk in an efficient manner instead of logging the individual
> 	 * dquot changes as they are made.
> 	 *
> 	 * Hence if we log the buffer that we allocate here, then crash
> 	 * post-quotacheck while the logged initialisation is still in the
> 	 * active region of the log, we can lose the information quotacheck
> 	 * wrote directly to the buffer. That is, log recovery will replay the
> 	 * dquot buffer initialisation over the top of whatever information
> 	 * quotacheck had written to the buffer.
> 	 *
> 	 * To avoid this problem, dquot allocation during quotacheck needs to
> 	 * avoid logging the initialised buffer, but we still need to have
> 	 * writeback of the buffer pin the tail of the log so that it is
> 	 * initialised on disk before we remove the allocation transaction from
> 	 * the active region of the log. Marking the buffer as ordered instead
> 	 * of logging it provides this behaviour.
> 	 */

That's certainly a /lot/ clearer. :)

> Also, does this mean quotacheck completion should force the log and push the AIL
> to ensure that all the allocations are completed and removed from the log before
> marking the quota as CHKD?

I need to think about this more, but I think the answer is that we don't
need to force/push the log because the delwri means we've persisted the
new dquot contents before we set CHKD, and if we crash before we set
CHKD, on the next mount attempt we'll re-run quotacheck, which can
reallocate or reinitialize the ondisk dquots.

But I dunno, maybe there's some other subtlety I haven't thought of...

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com