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@xxxxxxxxxxxxx
