xfs
[Top] [All Lists]

Re: RFC: adding a crc field to xfs_buf_log_format_t

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Re: RFC: adding a crc field to xfs_buf_log_format_t
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Wed, 24 Sep 2008 13:29:20 -0400
In-reply-to: <20080924010553.GC13705@disturbed>
References: <20080923172800.GA22047@xxxxxxxxxxxxx> <20080924010553.GC13705@disturbed>
User-agent: Mutt/1.5.18 (2008-05-17)
On Wed, Sep 24, 2008 at 11:05:53AM +1000, Dave Chinner wrote:
> On Tue, Sep 23, 2008 at 01:28:00PM -0400, Christoph Hellwig wrote:
> > With adding CRC to xfs metadata structures we face an interesting
> > problem.  As we want all the CRCs logged we always have to log the CRC.
> 
> What version of the CRC are you wanting to log? The one that is
> currently in the buffer (i.e. the one we last wrote to disk), or a
> new CRC that covers the changes we just made to the buffer?

The CRC of the block after the just applied changes.

> >> Note that we currently do not log the crc of the block, but
> >> re-created it during log recovery.  With the pending patch to
> >> also checksum the log this should be safe against filesystem
> >> corruption but doesn't really follow the end to end argument.
> 
> The CRC is protecting what is on disk, not what is being changed in
> memory. The model for protection is "write-IO to read-IO", not
> "in-memory change to in-memory change".  That is, the CRC is not
> protecting every single change that is made - it is simply there to
> validate what is on disk is *what we wrote*, and with the current
> re-logging model of the transaction subsystem that means each update
> of the CRC is an "aggregate change" of the object.
> 
> Hence I think that CRC'd log transactions are more than sufficient
> to protect against corruption of the delta changes that get applied
> to CRC protected objects.....

Still feeling a little un-easy about it, but I guess you're right.
Having one proper checksum for the log buffer should be enough, and
we should not worry about the end-to-end argument.  Certainly makes the
implementation a lot simpler, and the operations faster.

> >> Also poking into the buffer to find out whether this is a btree
> >> buffer during log recovery is not a very clean way to implement
> >> this.
> 
> Add the type of buffer to the buffer format structure, that way we
> can poke the buffer to _verify_ it's type rather than having to rely
> on what came off disk. Recording that type will also enable us to
> easily set up the buffer correctly for calculating the CRC at
> writeback at the end of log replay....

We can easily use blf_type for the type of buffer, that's what I had
changed the btree patch to after posting it.

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