Re: [PATCH 10/19] xfs: verify dquot blocks as they are read from disk

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 10/19] xfs: verify dquot blocks as they are read from disk
Date: Fri, 12 Oct 2012 09:08:43 +1100	[thread overview]
Message-ID: <20121011220843.GI2739@dastard> (raw)
In-Reply-To: <20121011214805.GI6346@infradead.org>

On Thu, Oct 11, 2012 at 05:48:05PM -0400, Christoph Hellwig wrote:
> On Tue, Oct 09, 2012 at 02:51:01PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add a dquot buffer verify callback function and pass it into the
> > buffer read functions. This checks all the dquots in a buffer, but
> > cannot completely verify the dquot ids are correct. Also, errors
> > cannot be repaired, so an additional function is added to repair bad
> > dquots in the buffer if such an error is detected in a context where
> > repair is allowed.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> Looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But after the first half dozen of callback I have a question:
> 
> > +		if (error) {
> > +			XFS_CORRUPTION_ERROR("xfs_dquot_read_verify",
> > +					     XFS_ERRLEVEL_LOW, mp, d);
> > +			xfs_buf_ioerror(bp, EFSCORRUPTED);
> > +			break;
> > +		}
> > +	}
> > +	bp->b_iodone = NULL;
> > +	xfs_buf_ioend(bp, 0);
> 
> It seems we always call xfs_buf_ioerror on errors, and then always
> do a
> 
> 	bp->b_iodone = NULL;
> 	xfs_buf_ioend(bp, 0);
> 
> for each callback.  What is the reason we can't take these two into the
> core buffer cache code?

I could, but that
requires changing all the iodone callbacks to function
that way. There is no reason a b_iodone function can't set another
b_iodone function to be called (i.e. chained completions), and
moving the bp->b_iodone = NULL; into the xfs_buf_ioend() code will
essentially prevent that. So I'm simply avoiding having to do
unnecessary extra work and validation by doing it this way.

Further, the next set of patches that introduce write verifiers
factor the read verifier into a verify function and two wrappers:

verify()
{
}

write_verify()
{
	verify(bp);
}

read_verify()
{
	verify(bp);
	bp->b_pre_io = write_verify;
	bp->b_iodone = NULL;
	xfs_buf_ioend(bp, 0);
}

And when CRC validation is added, it becomes:

write_verify()
{
	verify(bp);
	calc_crc(bp, crc_offset);
}

read_verify()
{
	verify_crc(bp, crc_offset);
	verify();
	bp->b_pre_io = write_verify;
	bp->b_iodone = NULL;
	xfs_buf_ioend(bp, 0);
}

So you can see that there is different processing for the read and
write operations, so handing the iodone/ioend processing in the read
verifier isn't a big deal.

If we want to change how we execute iodone processing, then I think
it's better to wait till all this falls out and we can tailor the
solution with the full set of use cases in view....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs