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

Darrick J. Wong
2020-05-18T16:53:13Z

On Mon, May 18, 2020 at 09:16:25AM -0400, Brian Foster wrote:
> On Thu, May 14, 2020 at 09:56:58AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> ...
> >
> > Fix this by changing the ondisk dquot initialization function to use
> > ordered buffers to write out fresh dquot blocks if it detects that we're
> > running quotacheck. If the system goes down before quotacheck can
> > complete, the CHKD flags will not be set in the superblock and the next
> > mount will run quotacheck again, which can fix uninitialized dquot
> > buffers. This requires amending the defer code to maintaine ordered
> > buffer state across defer rolls for the sake of the dquot allocation
> > code.
> >
> > For regular operations we preserve the current behavior since the dquot
> > items require properly initialized ondisk dquot records.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: rework the code comment explaining all this
> > ---
> > fs/xfs/libxfs/xfs_defer.c | 10 +++++++
> > fs/xfs/xfs_dquot.c | 62 ++++++++++++++++++++++++++++++++++++---------
> > 2 files changed, 58 insertions(+), 14 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index 52e0f7245afc..f60a8967f9d5 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> ...
> > @@ -238,11 +240,45 @@ xfs_qm_init_dquot_blk(
> ...
> > +
> > + /*
> > + * 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.
> > + *
> > + * If we crash before quotacheck completes, a subsequent quotacheck run
> > + * will re-allocate and re-initialize the dquot records as needed.
> > + */
>
> I took a stab at condensing the comment a bit, FWIW (diff below). LGTM
> either way. Thanks for the update.
>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
>
> > + if (!(mp->m_qflags & qflag))
> > + xfs_trans_ordered_buf(tp, bp);
> > + else
> > + xfs_trans_log_buf(tp, bp, 0, BBTOB(q->qi_dqchunklen) - 1);
> > }
> >
> > /*
> >
>
> diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> index f60a8967f9d5..55b95d45303b 100644
> --- a/fs/xfs/xfs_dquot.c
> +++ b/fs/xfs/xfs_dquot.c
> @@ -254,26 +254,20 @@ xfs_qm_init_dquot_blk(
> xfs_trans_dquot_buf(tp, bp, blftype);
>
> /*
> - * 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.
> + * quotacheck uses delayed writes to update all the dquots on disk in an
> + * efficient manner instead of logging the individual dquot changes as
> + * they are made. However if we log the buffer allocated here and crash
> + * after quotacheck while the logged initialisation is still in the
> + * active region of the log, log recovery can replay the dquot buffer
> + * initialisation over the top of the checked dquots and corrupt quota
> + * accounting.
> *
> - * 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.
> - *
> - * If we crash before quotacheck completes, a subsequent quotacheck run
> - * will re-allocate and re-initialize the dquot records as needed.
> + * To avoid this problem, quotacheck cannot log the initialised buffer.
> + * We must still dirty the buffer and write it back before the
> + * allocation transaction clears the log. Therefore, mark the buffer as
> + * ordered instead of logging it directly. This is safe for quotacheck
> + * because it detects and repairs allocated but initialized dquot blocks
> + * in the quota inodes.

I think I like your revised comment better. :)

--D

> */
> if (!(mp->m_qflags & qflag))
> xfs_trans_ordered_buf(tp, bp);
>