On Thu, May 30, 2013 at 08:02:12AM -0400, Brian Foster wrote:
> On 05/29/2013 09:00 PM, Dave Chinner wrote:
> > On Wed, May 29, 2013 at 02:58:27PM -0400, Brian Foster wrote:
> >> On 05/27/2013 02:38 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>
....
> >>> @@ -853,8 +854,12 @@ xfs_qm_reset_dqcounts(
> >>> do_div(j, sizeof(xfs_dqblk_t));
> >>> ASSERT(mp->m_quotainfo->qi_dqperchunk == j);
> >>> #endif
> >>> - ddq = bp->b_addr;
> >>> + dqb = bp->b_addr;
> >>> for (j = 0; j < mp->m_quotainfo->qi_dqperchunk; j++) {
> >>> + struct xfs_disk_dquot *ddq;
> >>> +
> >>> + ddq = (struct xfs_disk_dquot *)&dqb[j];
> >>> +
> >>> /*
> >>> * Do a sanity check, and if needed, repair the dqblk. Don't
> >>> * output any warnings because it's perfectly possible to
> >>> @@ -871,7 +876,8 @@ xfs_qm_reset_dqcounts(
> >>> ddq->d_bwarns = 0;
> >>> ddq->d_iwarns = 0;
> >>> ddq->d_rtbwarns = 0;
> >>> - ddq = (xfs_disk_dquot_t *) ((xfs_dqblk_t *)ddq + 1);
> >>> + xfs_update_cksum((char *)&dqb[j], sizeof(struct xfs_dqblk),
> >>> + XFS_DQUOT_CRC_OFF);
> >>
> >> Nice cleanup on the cast ugliness even without the crc change. Is there
> >> a technical reason for the unconditional crc update here beyond that
> >> we're doing a reset? I'm wondering if there's any value in leaving those
> >> bits untouched for a filesystem that might have never enabled crc
> >> (quotacheck or not).
> >
> > The dquot might be zeroed and unused, but the buffer it sits in is
> > still allocated and valid. That means if we ever start using that
> > dquot again (either by quotacheck or a new uid/gid/prid), it will be
> > read straight out of the buffer rather than allocated, and hence the
> > constraint that allocated but unused dquots still need to have valid
> > CRCs.
> >
>
> The constraint makes sense when CRCs are enabled...
>
> > FWIW, the dquot buffer read validates the CRC on all dquots in the
> > buffer when it comes off disk as it has no way of knowing what
> > dquots contain valid data or not. Same with the xfs_qm_dqcheck()
> > call - an unused dquot still needs to be a valid dquot to pass those
> > checks...
> >
>
> Yeah, that part makes sense. I've followed through and grokked most of
> the dquot buffer read and dquot CRC validation code, I think.
>
> My question is more why is the code above (in xfs_qm_reset_dqcounts())
> not the following?
>
> if (xfs_sb_version_hascrc(&mp->m_sb))
> xfs_update_cksum(...);
Because I forgot as it really doesn't matter at all. It wasn't
clear to me that this is what you were asking about the first time
around....
Fixed.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
