xfs
[Top] [All Lists]

Re: [PATCH 1/6] xfs: rework dquot CRCs

To: Dave Chinner <dchinner@xxxxxxxxxx>, Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 1/6] xfs: rework dquot CRCs
From: Ben Myers <bpm@xxxxxxx>
Date: Tue, 4 Jun 2013 16:46:12 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51ACDDF9.1040202@xxxxxxxxxx>
References: <1370237332-24757-1-git-send-email-david@xxxxxxxxxxxxx> <1370237332-24757-2-git-send-email-david@xxxxxxxxxxxxx> <51ACDDF9.1040202@xxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Mon, Jun 03, 2013 at 02:18:33PM -0400, Brian Foster wrote:
> On 06/03/2013 01:28 AM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > Calculating dquot CRCs when the backing buffer is written back just
> > doesn't work reliably. There are several places which manipulate
> > dquots directly in the buffers, and they don't calculate CRCs
> > appropriately, nor do they always set the buffer up to calculate
> > CRCs appropriately.
> > 
> > Firstly, if we log a dquot buffer (e.g. during allocation) it gets
> > logged without valid CRC, and so on recovery we end up with a dquot
> > that is not valid.
> > 
> > Secondly, if we recover/repair a dquot, we don't have a verifier
> > attached to the buffer and hence CRCs arenot calculate don the way
> > down to disk.
> > 
> > Thirdly, calculating the CRC after we've changed the contents means
> > that if we re-read the dquot from the buffer, we cannot verify the
> > contents of the dquot are valid, as the CRC is invalid.
> > 
> > So, to avoid all the dquot CRC errors that are being detected by the
> > read verifier, change to using the same model as for inodes. that
> > is, dquot CRCs are calculated and written to the backing buffer at
> > the time the dquot is flushed to the backing buffer. If we modify
> > the dquuot directly in the backing buffer, calculate the CRC
> > immediately after the modification is complete. Hence the dquot in
> > the on-disk buffer should always have a valid CRC.
> > 
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> 
> > ---
> >  fs/xfs/xfs_dquot.c       |   37 ++++++++++++++++---------------------
> >  fs/xfs/xfs_log_recover.c |   10 ++++++++++
> >  fs/xfs/xfs_qm.c          |   40 ++++++++++++++++++++++++++++++----------
> >  fs/xfs/xfs_quota.h       |    2 ++
> >  4 files changed, 58 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > index a41f8bf..044e97a 100644
> > --- a/fs/xfs/xfs_dquot.c
> > +++ b/fs/xfs/xfs_dquot.c
> > @@ -249,8 +249,11 @@ xfs_qm_init_dquot_blk(
> >             d->dd_diskdq.d_version = XFS_DQUOT_VERSION;
> >             d->dd_diskdq.d_id = cpu_to_be32(curid);
> >             d->dd_diskdq.d_flags = type;
> > -           if (xfs_sb_version_hascrc(&mp->m_sb))
> > +           if (xfs_sb_version_hascrc(&mp->m_sb)) {
> >                     uuid_copy(&d->dd_uuid, &mp->m_sb.sb_uuid);
> > +                   xfs_update_cksum((char *)d, sizeof(struct xfs_dqblk),
> > +                                    XFS_DQUOT_CRC_OFF);
> > +           }
> >     }
> >  
> >     xfs_trans_dquot_buf(tp, bp,

Huh.  Looks like this buffer will not be recovered.  See
xlog_recover_do_reg_buffer... I think he'll be skipped in recovery.  Goto next.

That's not an issue with this patch though.

Reviewed-by: Ben Myers <bpm@xxxxxxx>

<Prev in Thread] Current Thread [Next in Thread>