All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Darrick J. Wong" <djwong@us.ibm.com>
To: James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>,
	Theodore Tso <tytso@mit.edu>,
	Sunil Mushran <sunil.mushran@oracle.com>,
	Amir Goldstein <amir73il@gmail.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Andi Kleen <andi@firstfloor.org>, Mingming Cao <cmm@us.ibm.com>,
	Joel Becker <jlbec@evilplan.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-ext4@vger.kernel.org, Coly Li <colyli@gmail.com>
Subject: Re: [PATCH 08/16] ext4: Calculate and verify checksums for inode bitmaps
Date: Mon, 5 Sep 2011 15:12:26 -0700	[thread overview]
Message-ID: <20110905221226.GW12086@tux1.beaverton.ibm.com> (raw)
In-Reply-To: <1315251928.15087.74.camel@dabdike.int.hansenpartnership.com>

On Mon, Sep 05, 2011 at 02:45:28PM -0500, James Bottomley wrote:
> On Mon, 2011-09-05 at 11:22 -0700, Darrick J. Wong wrote:
> > On Fri, Sep 02, 2011 at 03:27:21PM -0600, Andreas Dilger wrote:
> > > On 2011-09-02, at 1:18 PM, Darrick J. Wong wrote:
> > > > On Wed, Aug 31, 2011 at 10:49:05PM -0600, Andreas Dilger wrote:
> > > >> On 2011-08-31, at 6:31 PM, Darrick J. Wong wrote:
> > > >>> Compute and verify the checksum of the inode bitmap; the checkum is stored in
> > > >>> the block group descriptor.
> > > >> 
> > > >> I would prefer if there was a 16-bit checksum for the (most common)
> > > >> 32-byte group descriptors, and this was extended to a 32-bit checksum
> > > >> for the (much less common) 64-byte+ group descriptors.  For filesystems
> > > >> that are newly formatted with the 64bit feature it makes no difference,
> > > >> but virtually all ext3/4 filesystems have only the smaller group descriptors.
> > > >> 
> > > >> Regardless of whether using half of the crc32c is better or worse than
> > > >> using crc16 for the bitmap blocks, storing _any_ checksum is better than
> > > >> storing nothing at all.  I would propose the following:
> > > > 
> > > > That's an interesting reframing of the argument that I hadn't considered.
> > > > I'd fallen into the idea of needing crc32c because of its bit error
> > > > guarantees (all corruptions of odd numbers of bits and all corruptions of
> > > > fewer than ...4? bits) that I hadn't quite realized that even if crc16
> > > > can't guarantee to find any corruption at all, it still _might_, and that's
> > > > better than nothing.
> > > > 
> > > > Ok, let's split the 32-bit fields and use crc16 for the case of 32-byte block
> > > > group descriptors.
> > > 
> > > I noticed the crc16 calculation is actually _slower_ than crc32c,
> > > probably because the CPU cannot use 32-bit values when computing the
> > > result, so it has to do a lot of word masking, per your table at
> > > https://ext4.wiki.kernel.org/index.php/Ext4_Metadata_Checksums.
> > > Also, there is the question of whether computing two different
> > > checksums is needlessly complicating the code, or if it is easier
> > > to just compute crc32c all the time and only make the storing of
> > > the high 16 bits conditional.
> > > 
> > > What I'm suggesting is always computing the crc32c, but for filesystems
> > > that are not formatted with the 64bit option just store the low 16 bits
> > > of the crc32c value into bg_{block,inode}_bitmap_csum_lo.  This is much
> > > better than not computing a checksum here at all.  The only open question
> > > is whether 1/2 of crc32c is substantially worse at detecting errors than
> > > crc16 or not?
> > 
> > All the literature I've read has suggested that crc16 can't guarantee any error
> > detection capability at all with data buffers longer than 256 bytes.
> 
> Um, so in a hashing algorithm that maps f:Z_m -> Z_n you can never
> guarantee error detection if m>n because of hash collisions.  All you
> can guarantee is that if f(a) != f(b) then a != b, so crc16 wouldn't be
> able to *guarantee* error detection in anything over 2 bytes.
> 
> All of the rest of the magic in hashing functions goes into making sure
> that the collision sets don't include common errors (like bit flipping).
> In theory, for the correct polynomial, CRC-16 should be able to detect
> single, double and triple bit flip errors in blocks of up to 8191
> bytes ... of course, if those aren't your common errors, then this
> analysis is useless ...

Sorry, I grossly misspoke.  Of course crc16 can't guarantee the ability to
detect all possible errors in any data block larger than 16 bits.  What I meant
to say is that I wasn't sure what is the maximum number of bit errors that
crc16 polynomials can detect given a message length of 32768+32+128 bits.

In particular, I remember reading on the wikipedia page[1] that for polynomials
with odd numbers of terms (such as ansi crc16), the period for 2-bit errors is
65535 bits as you say; but as I recall, those polynomials also can't detect all
errors involving odd numbers of bit flips.  For polynomials with even numbers
of terms (such as the t10dif one) the period in which it can detect 2-bit
errors is 32767 bits, but on the other hand they can detect odd numbers of
errors.

The people who study error rates on disk hardware at IBM tell me that bit flips
are more common than you'd like, though I was also looking for something that
can tell me if blocks are being written to the wrong places.

[1] http://en.wikipedia.org/wiki/Mathematics_of_CRC#Bitfilters

--D
> 
> James
> 
> 

  reply	other threads:[~2011-09-05 22:12 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-01  0:30 [PATCH v1 00/16] ext4: Add metadata checksumming Darrick J. Wong
2011-09-01  0:30 ` [PATCH 01/16] ext4: ext4_dx_add_entry should dirty directory metadata with the directory inode Darrick J. Wong
2011-09-01  0:30 ` [PATCH 02/16] ext4: ext4_rename should dirty dir_bh with the correct directory Darrick J. Wong
2011-09-01  0:30 ` [PATCH 03/16] ext4: ext4_mkdir should dirty dir_block with the parent inode Darrick J. Wong
2011-09-01  0:30 ` [PATCH 04/16] ext4: Create a new BH_Verified flag to avoid unnecessary metadata validation Darrick J. Wong
2011-09-01  0:31 ` [PATCH 05/16] ext4: Create a rocompat flag for extended metadata checksumming Darrick J. Wong
2011-09-01  0:31 ` [PATCH 06/16] ext4: Calculate and verify inode checksums Darrick J. Wong
2011-09-01  2:30   ` Andreas Dilger
2011-09-02 19:32     ` Darrick J. Wong
2011-09-02 22:02       ` Andreas Dilger
2011-09-05 17:57         ` Darrick J. Wong
2011-09-01  0:31 ` [PATCH 07/16] ext4: Create bitmap checksum helper functions Darrick J. Wong
2011-09-01  0:31 ` [PATCH 08/16] ext4: Calculate and verify checksums for inode bitmaps Darrick J. Wong
2011-09-01  4:49   ` Andreas Dilger
2011-09-02 19:18     ` Darrick J. Wong
2011-09-02 21:27       ` Andreas Dilger
2011-09-05 18:22         ` Darrick J. Wong
2011-09-05 18:27           ` Andi Kleen
2011-09-05 19:45           ` James Bottomley
2011-09-05 22:12             ` Darrick J. Wong [this message]
2011-09-01  0:31 ` [PATCH 09/16] ext4: Calculate and verify block bitmap checksum Darrick J. Wong
     [not found]   ` <2492E720-3316-4561-8C9C-BBC6E8670EAD@dilger.ca>
2011-09-02 19:08     ` Darrick J. Wong
2011-09-02 21:06       ` Andreas Dilger
2011-09-01  0:31 ` [PATCH 10/16] ext4: Verify and calculate checksums for extent tree blocks Darrick J. Wong
     [not found]   ` <26584BE9-B716-40D5-B3B4-8C5912869648@dilger.ca>
2011-09-02 19:02     ` Darrick J. Wong
2011-09-01  0:31 ` [PATCH 11/16] ext4: Calculate and verify checksums for htree nodes Darrick J. Wong
2011-09-01  0:31 ` [PATCH 12/16] ext4: Calculate and verify checksums of directory leaf blocks Darrick J. Wong
     [not found]   ` <4D5D8FB2-0D8A-4D30-B6D8-51158395C1C9@dilger.ca>
2011-09-02 18:57     ` Darrick J. Wong
2011-09-02 20:52       ` Andreas Dilger
2011-09-05 18:30         ` Darrick J. Wong
2011-09-01  0:32 ` [PATCH 13/16] ext4: Calculate and verify checksums of extended attribute blocks Darrick J. Wong
     [not found]   ` <F4A42DCE-F91C-419B-9153-65A7EA91D241@dilger.ca>
2011-09-02 18:43     ` Darrick J. Wong
2011-09-01  0:32 ` [PATCH 14/16] ext4: Make inode checksum cover empty space Darrick J. Wong
     [not found]   ` <4540D5DB-53D9-4C33-BC6B-868870D42AF3@dilger.ca>
2011-09-02 18:42     ` Darrick J. Wong
2011-09-01  0:32 ` [PATCH 15/16] ext4: Calculate and verify superblock checksum Darrick J. Wong
     [not found]   ` <2882FBB2-797C-4D27-8569-B6826DD34F68@dilger.ca>
2011-09-02 18:40     ` Darrick J. Wong
2011-09-01  0:32 ` [PATCH 16/16] jbd2: Support CRC32C transaction checksums Darrick J. Wong
     [not found]   ` <99019900-30B1-450A-9620-E94371A30CC6@dilger.ca>
2011-09-02 18:31     ` Darrick J. Wong
2011-09-02 14:15 ` [PATCH v1 00/16] ext4: Add metadata checksumming Greg Freemyer
2011-09-02 14:15   ` Greg Freemyer
2011-09-02 18:22   ` Darrick J. Wong
2011-09-02 18:22     ` Darrick J. Wong
2011-09-04 11:41     ` Martin K. Petersen
2011-09-04 16:54       ` Andi Kleen
2011-09-04 17:17         ` Martin K. Petersen
2011-09-04 17:44           ` Andi Kleen
2011-09-04 22:19             ` Martin K. Petersen
2011-09-05 18:55               ` Darrick J. Wong
2011-09-05 18:45       ` Darrick J. Wong
2011-09-06 12:59         ` Martin K. Petersen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110905221226.GW12086@tux1.beaverton.ibm.com \
    --to=djwong@us.ibm.com \
    --cc=James.Bottomley@hansenpartnership.com \
    --cc=adilger.kernel@dilger.ca \
    --cc=amir73il@gmail.com \
    --cc=andi@firstfloor.org \
    --cc=cmm@us.ibm.com \
    --cc=colyli@gmail.com \
    --cc=jlbec@evilplan.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sunil.mushran@oracle.com \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.