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

[Dave Chinner <david@fromorbit.com>]

On Tue, May 12, 2020 at 05:25:48PM -0700, Darrick J. Wong wrote:
> 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.

*nod*

That was pretty much my thinking, but I haven't looked at the code
to determine if there was...

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

.... some other subtlety I hadn't thought of... :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com