Re: [PATCH 16/16] jbd2: Support CRC32C transaction checksums

["Darrick J. Wong" <djwong@us.ibm.com>]

On Thu, Sep 01, 2011 at 02:11:26AM -0600, Andreas Dilger wrote:
> On 2011-08-31, at 6:32 PM, Darrick J. Wong wrote:
> > The CRC32c polynomial provides better error detection and can be hardware
> > accelerated on certain machines.  To that end, add support for it to jbd2.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> > fs/jbd2/Kconfig      |    1 +
> > fs/jbd2/commit.c     |    6 +++---
> > fs/jbd2/recovery.c   |   20 +++++++++++++++++---
> > include/linux/jbd2.h |    1 +
> > 4 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > 
> > diff --git a/fs/jbd2/Kconfig b/fs/jbd2/Kconfig
> > index f32f346..40a126b 100644
> > --- a/fs/jbd2/Kconfig
> > +++ b/fs/jbd2/Kconfig
> > @@ -1,6 +1,7 @@
> > config JBD2
> > 	tristate
> > 	select CRC32
> > +	select LIBCRC32C
> > 	help
> > 	  This is a generic journaling layer for block devices that support
> > 	  both 32-bit and 64-bit block numbers.  It is currently used by
> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > index eef6979..00387be 100644
> > --- a/fs/jbd2/commit.c
> > +++ b/fs/jbd2/commit.c
> > @@ -21,7 +21,7 @@
> > #include <linux/mm.h>
> > #include <linux/pagemap.h>
> > #include <linux/jiffies.h>
> > -#include <linux/crc32.h>
> > +#include <linux/crc32c.h>
> > #include <linux/writeback.h>
> > #include <linux/backing-dev.h>
> > #include <linux/bio.h>
> > @@ -125,7 +125,7 @@ static int journal_submit_commit_record(journal_t *journal,
> > 
> > 	if (JBD2_HAS_COMPAT_FEATURE(journal,
> > 				    JBD2_FEATURE_COMPAT_CHECKSUM)) {
> > -		tmp->h_chksum_type 	= JBD2_CRC32_CHKSUM;
> > +		tmp->h_chksum_type	= JBD2_CRC32C_CHKSUM;
> > 		tmp->h_chksum_size 	= JBD2_CRC32_CHKSUM_SIZE;
> > 		tmp->h_chksum[0] 	= cpu_to_be32(crc32_sum);
> > 	}
> > @@ -287,7 +287,7 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
> > 	__u32 checksum;
> > 
> > 	addr = kmap_atomic(page, KM_USER0);
> > -	checksum = crc32_be(crc32_sum,
> > +	checksum = crc32c_le(crc32_sum,
> > 		(void *)(addr + offset_in_page(bh->b_data)), bh->b_size);
> > 	kunmap_atomic(addr, KM_USER0);
> > 
> > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> > index 1cad869..4bab4dd 100644
> > --- a/fs/jbd2/recovery.c
> > +++ b/fs/jbd2/recovery.c
> > @@ -21,6 +21,7 @@
> > #include <linux/jbd2.h>
> > #include <linux/errno.h>
> > #include <linux/crc32.h>
> > +#include <linux/crc32c.h>
> > #endif
> > 
> > /*
> > @@ -323,7 +324,8 @@ static inline unsigned long long read_tag_block(int tag_bytes, journal_block_tag
> >  * descriptor block.
> >  */
> > static int calc_chksums(journal_t *journal, struct buffer_head *bh,
> > -			unsigned long *next_log_block, __u32 *crc32_sum)
> > +			unsigned long *next_log_block, __u32 *crc32_sum,
> > +			__u32 *crc32c_sum)
> > {
> > 	int i, num_blks, err;
> > 	unsigned long io_block;
> > @@ -332,6 +334,7 @@ static int calc_chksums(journal_t *journal, struct buffer_head *bh,
> > 	num_blks = count_tags(journal, bh);
> > 	/* Calculate checksum of the descriptor block. */
> > 	*crc32_sum = crc32_be(*crc32_sum, (void *)bh->b_data, bh->b_size);
> > +	*crc32c_sum = crc32c_le(*crc32c_sum, (void *)bh->b_data, bh->b_size);
> 
> We definitely do not want to compute both the crc32 and crc32c for every
> block written to the journal.  That would be needlessly expensive.
> 
> It looks like the missing factor is not knowing the checksum type before
> the commit block is accessed.  This can be fixed by storing the checksum
> type in the journal superblock (JBD2_CKSUM_TYPE_CRC32 = 0 for compatibility).
> 
> During recovery, the journal superblock s_chksum_type should remain as
> set by the previous kernel (so the existing commit block checksums can
> be verified) and as soon as the journal recovery has completed and a new
> block written to the journal the checksum type can be updated to the latest
> default value.

Okay, that does sound better than my current weird approach. :)  I admit to
being a bit timid about messing with jbd2 after proposing a lot of changes to
ext4.

> > 	for (i = 0; i < num_blks; i++) {
> > 		io_block = (*next_log_block)++;
> > @@ -344,6 +347,9 @@ static int calc_chksums(journal_t *journal, struct buffer_head *bh,
> > 		} else {
> > 			*crc32_sum = crc32_be(*crc32_sum, (void *)obh->b_data,
> > 				     obh->b_size);
> > +			*crc32c_sum = crc32c_le(*crc32c_sum,
> > +				     (void *)obh->b_data,
> > +				     obh->b_size);
> 
> (style) the indenting here is funky.  Continued lines should follow the
>        '(' on the previous line and not be gratuitously wrapped.

Yeah, I wondered about that.

--D

> > @@ -617,7 +625,12 @@ static int do_one_pass(journal_t *journal,
> > 				    cbh->h_chksum_type == JBD2_CRC32_CHKSUM &&
> > 				    cbh->h_chksum_size ==
> > 						JBD2_CRC32_CHKSUM_SIZE)
> > -				       chksum_seen = 1;
> > +					chksum_seen = 1;
> > +				else if (crc32c_sum == found_chksum &&
> > +				    cbh->h_chksum_type == JBD2_CRC32C_CHKSUM &&
> > +				    cbh->h_chksum_size ==
> > +						JBD2_CRC32_CHKSUM_SIZE)
> > +					chksum_seen = 1;
> > 				else if (!(cbh->h_chksum_type == 0 &&
> > 					     cbh->h_chksum_size == 0 &&
> > 					     found_chksum == 0 &&
> > @@ -646,6 +659,7 @@ static int do_one_pass(journal_t *journal,
> > 					}
> > 				}
> > 				crc32_sum = ~0;
> > +				crc32c_sum = ~0;
> > 			}
> > 			brelse(bh);
> > 			next_commit_ID++;
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index 38f307b..de3ec23 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -147,6 +147,7 @@ typedef struct journal_header_s
> > #define JBD2_CRC32_CHKSUM   1
> > #define JBD2_MD5_CHKSUM     2
> > #define JBD2_SHA1_CHKSUM    3
> > +#define JBD2_CRC32C_CHKSUM  4
> > 
> > #define JBD2_CRC32_CHKSUM_SIZE 4
> > 
> > 
>