RFC: adding a crc field to xfs_buf_log_format_t
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. But this means for many modification that previous just required one or two chunks of a buffer logged (e.g. superblock counters or changing a single btree records) we would now need a second iovec. One idea to avoid this would be to put the crc into a modified version of xfs_buf_log_format_t. Any opinions wether this is worth the trouble?
Quoted 2 lines · Christoph Hellwig
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?
In the first case, I can't see how having that CRC in the transaction helps in recovery at all. Algorithmically, if all buffers have a crc32c in them, then the buffers should CRC to zero when you include the CRC value in the calculation. Hence during log recovery when we read a buffer in for the first time, we simply need to check that the buffer CRC is zero. Hence we can verify that we've read an uncorrupted buffer regardless of it's type or location of the crc value in the buffer.
In the second case, that means every transaction commit has to recalculate the CRC for every buffer modified to insert them into the transaction. That means we need to peak into the buffer type during transaction commit to determine where the CRC is and extract that. There's a *lot* of CPU overhead there, especially for heavily re-logged buffers, and once again I don't think it buys us anything because we still need to verify the CRC is correct before we write the buffer to disk at the end of log replay...
I note that from your previous patch set you make these comments:
Quoted 4 lines
Quoted 4 lines
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.....
Quoted 3 lines
Quoted 3 lines
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....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Quoted 42 lines · Dave Chinner
Quoted 2 lines · Christoph Hellwig
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?
In the first case, I can't see how having that CRC in the transaction helps in recovery at all. Algorithmically, if all buffers have a crc32c in them, then the buffers should CRC to zero when you include the CRC value in the calculation. Hence during log recovery when we read a buffer in for the first time, we simply need to check that the buffer CRC is zero. Hence we can verify that we've read an uncorrupted buffer regardless of it's type or location of the crc value in the buffer.
In the second case, that means every transaction commit has to recalculate the CRC for every buffer modified to insert them into the transaction. That means we need to peak into the buffer type during transaction commit to determine where the CRC is and extract that. There's a *lot* of CPU overhead there, especially for heavily re-logged buffers, and once again I don't think it buys us anything because we still need to verify the CRC is correct before we write the buffer to disk at the end of log replay...
I note that from your previous patch set you make these comments:
Quoted 4 lines
Quoted 4 lines
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.
Quoted 4 lines
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.....
Thanks for the clarification.
I haven't looked at the CRC of the transactions yet - need to find that patch.
But it seems to make sense to just apply CRC's to metadata or log data that is going to disk and keep things simple - as we are targetting corruption of on-disk meta data by outside things.
--Tim
Quoted 7 lines · Dave Chinner
Quoted 2 lines · Christoph Hellwig
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.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
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.