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@xxxxxxxxxx>
> > ---
> > 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
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
