Patchwork [15/23] jbd2: Change disk layout for metadata checksumming

login
register
mail settings
Submitter Darrick J. Wong
Date March 6, 2012, 8:49 p.m.
Message ID <20120306204941.1663.56283.stgit@elm3b70.beaverton.ibm.com>
Download mbox | patch
Permalink /patch/145012/
State Deferred
Delegated to: Theodore Ts'o
Headers show

Comments

Darrick J. Wong - March 6, 2012, 8:49 p.m.
Define flags and allocate space in on-disk journal structures to support
checksumming of journal metadata.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 include/linux/jbd2.h |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - April 28, 2012, 2:19 p.m.
On Tue, Mar 06, 2012 at 12:49:41PM -0800, Darrick J. Wong wrote:
> @@ -177,11 +189,17 @@ typedef struct journal_block_tag_s
>  	__be32		t_blocknr;	/* The on-disk block number */
>  	__be32		t_flags;	/* See below */
>  	__be32		t_blocknr_high; /* most-significant high 32bits. */
> +	__be32		t_checksum;	/* crc32c(uuid+seq+block) */
>  } journal_block_tag_t;
>  
>  #define JBD2_TAG_SIZE32 (offsetof(journal_block_tag_t, t_blocknr_high))
>  #define JBD2_TAG_SIZE64 (sizeof(journal_block_tag_t))

There's a problem with this patch here --- we are changing the size of
journal_block_tag_t, which is an on-disk data structure.  So for
64-bit journals, this represents a format change.  This means that if
you have a 64-bit file system that needs to have its journal
recovered, if the journal was written with an older kernel, and then
we try to recover it with a new kernel, things won't be good.
Similarly, for e2fsck's recovery code, it's not going to be able to
recover 64-bit file systems using current coding, since this patch
series changes the size of JBD2_TAG_SIZE64.

What we need to do is something like this:

#define JBD2_TAG_SIZE64 (offsetof(journal_block_tag_t, t_checksum))
#define JBD2_TAG_SIZE_CSUM (sizeof(journal_block_tag_t))

And then change the code appropriately in e2fsprogs and in the kernel
to use the correct tag size depending on the journal options.

       	   	       	    	      	     - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger - April 28, 2012, 10:58 p.m.
On 2012-04-28, at 8:19, Ted Ts'o <tytso@mit.edu> wrote:

> On Tue, Mar 06, 2012 at 12:49:41PM -0800, Darrick J. Wong wrote:
>> @@ -177,11 +189,17 @@ typedef struct journal_block_tag_s
>>    __be32        t_blocknr;    /* The on-disk block number */
>>    __be32        t_flags;    /* See below */
>>    __be32        t_blocknr_high; /* most-significant high 32bits. */
>> +    __be32        t_checksum;    /* crc32c(uuid+seq+block) */
>> } journal_block_tag_t;
>> 
>> #define JBD2_TAG_SIZE32 (offsetof(journal_block_tag_t, t_blocknr_high))
>> #define JBD2_TAG_SIZE64 (sizeof(journal_block_tag_t))
> 
> There's a problem with this patch here --- we are changing the size of
> journal_block_tag_t, which is an on-disk data structure.  So for
> 64-bit journals, this represents a format change.  This means that if
> you have a 64-bit file system that needs to have its journal
> recovered, if the journal was written with an older kernel, and then
> we try to recover it with a new kernel, things won't be good.
> Similarly, for e2fsck's recovery code, it's not going to be able to
> recover 64-bit file systems using current coding, since this patch
> series changes the size of JBD2_TAG_SIZE64.
> 
> What we need to do is something like this:
> 
> #define JBD2_TAG_SIZE64 (offsetof(journal_block_tag_t, t_checksum))
> #define JBD2_TAG_SIZE_CSUM (sizeof(journal_block_tag_t))
> 
> And then change the code appropriately in e2fsprogs and in the kernel
> to use the correct tag size depending on the journal options.

I thought we originally discussed using the high 16 bits of the t_flags field to store the checksum?  This would avoid the need to change the disk format. 

Since there is still a whole transaction checksum, it isn't so critical that the per-block checksum be strong.

One idea is to do the crc32c for each block, then store the high 16 bits into t_flags, and checksum the full 32-bit per-block checksums to make the commit block checksum, to avoid having to do the block checksums twice. 

Cheers, Andreas--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - April 29, 2012, 7:39 p.m.
[ I've trimmed the cc line to avoid spamming lots of folks who might not
  care about the details of jbd2 checksumming. ]

On Sat, Apr 28, 2012 at 04:58:12PM -0600, Andreas Dilger wrote:
> 
> I thought we originally discussed using the high 16 bits of the
> t_flags field to store the checksum?  This would avoid the need to
> change the disk format.

I don't recall that suggestion, but I like it.  One thing that will
get subtle about this is that t_flags is stored big-endian (jbd/jbd2
data structures are stored be, but the data structures in ext4 proper
are stored le; sigh).   So we'd have to do something like this:

typedef struct journal_block_tag_s
{
	__u32		t_blocknr;	/* The on-disk block number */
	__u16		t_checksum;	/* 16-bit checksum */
	__u16		t_flags;	/* See below */
	__u32		t_blocknr_high; /* most-significant high 32bits. */
} journal_block_tag_t;

... and then make sure we change all of the places that access t_flags
using cpu_to_be32() and be32_to_cpu() get changed to the 16-bit
variant.

> Since there is still a whole transaction checksum, it isn't so
> critical that the per-block checksum be strong.
>
> One idea is to do the crc32c for each block, then store the high 16
> bits into t_flags, and checksum the full 32-bit per-block checksums
> to make the commit block checksum, to avoid having to do the block
> checksums twice.

It's not critical because the hard drive is doing its own ECC.  So I'm
not that worried about detecting a large burst of bit errors, which is
the main advantage of using a larger CRC.  I'm more worried about a
disk block getting written to the wrong place, or not getting written
at all.  So whether the chance of detecting a wrong block is 99.9985%
at all (w/ a 16-bit checksum) or 99.9999% (with a 32-bit checksum),
at all, either is fine.

I'm not even sure I would worry combining the per-block checksums into
the commit block checksum.  In the rare case where there is an error
not detected by the 16-bit checksum which is detected in the commit
checksum, what are we supposed to do?  Throw away the entire commit
again?  Just simply testing to see what we do in this rare case is
going to be interesting / painful.

   	     	     	      	 	     - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger - April 30, 2012, 4:08 a.m.
On 2012-04-29, at 1:39 PM, Ted Ts'o wrote:
> On Sat, Apr 28, 2012 at 04:58:12PM -0600, Andreas Dilger wrote:
>> 
>> I thought we originally discussed using the high 16 bits of the
>> t_flags field to store the checksum?  This would avoid the need to
>> change the disk format.
> 
> I don't recall that suggestion, but I like it.  One thing that will
> get subtle about this is that t_flags is stored big-endian (jbd/jbd2
> data structures are stored be, but the data structures in ext4 proper
> are stored le; sigh).   So we'd have to do something like this:
> 
> typedef struct journal_block_tag_s
> {
> 	__u32		t_blocknr;	/* The on-disk block number */
> 	__u16		t_checksum;	/* 16-bit checksum */
> 	__u16		t_flags;	/* See below */
> 	__u32		t_blocknr_high; /* most-significant high 32bits. */
> } journal_block_tag_t;
> 
> ... and then make sure we change all of the places that access t_flags
> using cpu_to_be32() and be32_to_cpu() get changed to the 16-bit
> variant.

Sigh...

>> Since there is still a whole transaction checksum, it isn't so
>> critical that the per-block checksum be strong.
>> 
>> One idea is to do the crc32c for each block, then store the high 16
>> bits into t_flags, and checksum the full 32-bit per-block checksums
>> to make the commit block checksum, to avoid having to do the block
>> checksums twice.
> 
> It's not critical because the hard drive is doing its own ECC.  So I'm
> not that worried about detecting a large burst of bit errors, which is
> the main advantage of using a larger CRC.  I'm more worried about a
> disk block getting written to the wrong place, or not getting written
> at all.  So whether the chance of detecting a wrong block is 99.9985%
> at all (w/ a 16-bit checksum) or 99.9999% (with a 32-bit checksum),
> at all, either is fine.
> 
> I'm not even sure I would worry combining the per-block checksums into
> the commit block checksum.  In the rare case where there is an error
> not detected by the 16-bit checksum which is detected in the commit
> checksum, what are we supposed to do?  Throw away the entire commit
> again?  Just simply testing to see what we do in this rare case is
> going to be interesting / painful.

The main reason to create the commit block checksum out of the
per-block checksums is to avoid having to checksum the data blocks
themselves twice.  That would be a significant overhead to compute,
say, 4096 * 4kB = 16MB of block data, while computing the commit
block checksum out of 4096 * 2 bytes = 8kB of the 16-bit checksums
is relatively minor.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong - April 30, 2012, 3:53 p.m.
On Sat, Apr 28, 2012 at 10:19:33AM -0400, Ted Ts'o wrote:
> On Tue, Mar 06, 2012 at 12:49:41PM -0800, Darrick J. Wong wrote:
> > @@ -177,11 +189,17 @@ typedef struct journal_block_tag_s
> >  	__be32		t_blocknr;	/* The on-disk block number */
> >  	__be32		t_flags;	/* See below */
> >  	__be32		t_blocknr_high; /* most-significant high 32bits. */
> > +	__be32		t_checksum;	/* crc32c(uuid+seq+block) */
> >  } journal_block_tag_t;
> >  
> >  #define JBD2_TAG_SIZE32 (offsetof(journal_block_tag_t, t_blocknr_high))
> >  #define JBD2_TAG_SIZE64 (sizeof(journal_block_tag_t))
> 
> There's a problem with this patch here --- we are changing the size of
> journal_block_tag_t, which is an on-disk data structure.  So for
> 64-bit journals, this represents a format change.  This means that if
> you have a 64-bit file system that needs to have its journal
> recovered, if the journal was written with an older kernel, and then
> we try to recover it with a new kernel, things won't be good.
> Similarly, for e2fsck's recovery code, it's not going to be able to
> recover 64-bit file systems using current coding, since this patch
> series changes the size of JBD2_TAG_SIZE64.
> 
> What we need to do is something like this:
> 
> #define JBD2_TAG_SIZE64 (offsetof(journal_block_tag_t, t_checksum))
> #define JBD2_TAG_SIZE_CSUM (sizeof(journal_block_tag_t))
> 
> And then change the code appropriately in e2fsprogs and in the kernel
> to use the correct tag size depending on the journal options.

Oops.  I forgot to update JBD2_TAG_SIZE64.

I have a question, though -- it looks as though the code that handles reading
and writing tags from raw disk blocks calls journal_tag_bytes() to determine
the tag size, and manually increments a pointer "tagp" to step through the
block.  This construction seems to be be sufficient to deal with possible
differences between sizeof(journal_block_tag_t) and the on-disk tag size, and
both increases over the 32bit tag size are gated on INCOMPAT_64BIT and
INCOMPAT_CSUM_V2.

Had I defined JBD2_TAG_SIZE64 with offsetof() as Ted did above, I think that
journal_tag_bytes() would return the correct on-disk tag size, which should fix
the scenario Ted outlined above.  The tag checksum set/verify functions would
also need to be taught where t_checksum is (in the space occupied by
t_blocknr_high) on a 32bit journal.  Could those two suggestions fix the
problem without causing us to discard half the checksum bits?



Well, not quite -- the calculation of tags per block in journal.c below the
comment "journal descriptor can store up to n blocks -bzzz" probably ought to
be using journal_tag_bytes(), not sizeof(journal_block_tag_t) to figure out how
many tags can be crammed into a disk block, since right now I think it
underreports the number of tags per block on a 32bit journal.
journal_tag_disk_size() is a more descriptive name for journal_tag_bytes().

As for putting half the checksum into the upper 16 bits of the flags field --
is journal space at such a premium that we need to overload the field and
reduce the strength of the checksum?  Enabling journal checksums on a 4k block
filesystem causes tags_per_block to decrease from 512 to 341 on a 32bit journal
and from 341 to 256 on a 64bit journal.  Do transactions typically have that
many blocks?  I didn't think most transactions had 1-2MB of dirty data.

--D
> 
>        	   	       	    	      	     - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger - April 30, 2012, 4:51 p.m.
On 2012-04-30, at 9:53 AM, djwong wrote:
> As for putting half the checksum into the upper 16 bits of the flags
> field -- is journal space at such a premium that we need to overload
> the field and reduce the strength of the checksum?  Enabling journal
> checksums on a 4k block filesystem causes tags_per_block to decrease
> from 512 to 341 on a 32bit journal and from 341 to 256 on a 64bit
> journal.  Do transactions typically have that many blocks?  I didn't
> think most transactions had 1-2MB of dirty data.

I think on a busy filesystem there can be many thousands of blocks in
a single transaction.  We run Lustre with 400MB journals, and under
metadata-intensive workloads we can hit the 100MB transaction size
limit easily.  However, this doesn't mean there are 25k blocks in
each transaction, since most of these blocks are reserved for the
worst case, but not used.

As for the impact of reducing the number of tags in each block, for a
4096-block transaction this would currently mean 8 32-bit tag blocks,
and it would grow to 12 or 13, which isn't significant in the end.

My suggestion was mostly to avoid problems with the disk format change.
If this can be handled in another manner, AND it doesn't break journal
recovery on older kernels/e2fsprogs, then I'm OK with the cleaner
approach.  Please ensure that this is tested.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong - May 18, 2012, 10:51 p.m.
On Sun, Apr 29, 2012 at 03:39:21PM -0400, Ted Ts'o wrote:
> [ I've trimmed the cc line to avoid spamming lots of folks who might not
>   care about the details of jbd2 checksumming. ]
> 
> On Sat, Apr 28, 2012 at 04:58:12PM -0600, Andreas Dilger wrote:
> > 
> > I thought we originally discussed using the high 16 bits of the
> > t_flags field to store the checksum?  This would avoid the need to
> > change the disk format.
> 
> I don't recall that suggestion, but I like it.  One thing that will
> get subtle about this is that t_flags is stored big-endian (jbd/jbd2
> data structures are stored be, but the data structures in ext4 proper
> are stored le; sigh).   So we'd have to do something like this:
> 
> typedef struct journal_block_tag_s
> {
> 	__u32		t_blocknr;	/* The on-disk block number */
> 	__u16		t_checksum;	/* 16-bit checksum */
> 	__u16		t_flags;	/* See below */
> 	__u32		t_blocknr_high; /* most-significant high 32bits. */
> } journal_block_tag_t;
> 
> ... and then make sure we change all of the places that access t_flags
> using cpu_to_be32() and be32_to_cpu() get changed to the 16-bit
> variant.
> 
> > Since there is still a whole transaction checksum, it isn't so
> > critical that the per-block checksum be strong.
> >
> > One idea is to do the crc32c for each block, then store the high 16
> > bits into t_flags, and checksum the full 32-bit per-block checksums
> > to make the commit block checksum, to avoid having to do the block
> > checksums twice.
> 
> It's not critical because the hard drive is doing its own ECC.  So I'm
> not that worried about detecting a large burst of bit errors, which is
> the main advantage of using a larger CRC.  I'm more worried about a
> disk block getting written to the wrong place, or not getting written
> at all.  So whether the chance of detecting a wrong block is 99.9985%
> at all (w/ a 16-bit checksum) or 99.9999% (with a 32-bit checksum),
> at all, either is fine.

Hmmm, what about 64k block filesystems?

Anyway, I revised two of the patches quite a while ago and apparently forgot to
send them. :(  They simply enlarge the journal tag struct and adjust the code
to use journal_tag_bytes() instead of the constants.

I was going to send them out, but I rebased off e2fsprogs head and 3.4-rc7 just
today and saw new regressions about group descriptor checksums.  Oh well.

--D

> 
> I'm not even sure I would worry combining the per-block checksums into
> the commit block checksum.  In the rare case where there is an error
> not detected by the 16-bit checksum which is detected in the commit
> checksum, what are we supposed to do?  Throw away the entire commit
> again?  Just simply testing to see what we do in this rare case is
> going to be interesting / painful.
> 
>    	     	     	      	 	     - Ted
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 5557bae..c286153 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -147,12 +147,24 @@  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
 
 #define JBD2_CHECKSUM_BYTES (32 / sizeof(u32))
 /*
  * Commit block header for storing transactional checksums:
+ *
+ * NOTE: If FEATURE_COMPAT_CHECKSUM (checksum v1) is set, the h_chksum*
+ * fields are used to store a checksum of the descriptor and data blocks.
+ *
+ * If FEATURE_INCOMPAT_CSUM_V2 (checksum v2) is set, then the h_chksum
+ * field is used to store crc32c(uuid+commit_block).  Each journal metadata
+ * block gets its own checksum, and data block checksums are stored in
+ * journal_block_tag (in the descriptor).  The other h_chksum* fields are
+ * not used.
+ *
+ * Checksum v1 and v2 are mutually exclusive features.
  */
 struct commit_header {
 	__be32		h_magic;
@@ -177,11 +189,17 @@  typedef struct journal_block_tag_s
 	__be32		t_blocknr;	/* The on-disk block number */
 	__be32		t_flags;	/* See below */
 	__be32		t_blocknr_high; /* most-significant high 32bits. */
+	__be32		t_checksum;	/* crc32c(uuid+seq+block) */
 } journal_block_tag_t;
 
 #define JBD2_TAG_SIZE32 (offsetof(journal_block_tag_t, t_blocknr_high))
 #define JBD2_TAG_SIZE64 (sizeof(journal_block_tag_t))
 
+/* Tail of descriptor block, for checksumming */
+struct jbd2_journal_block_tail {
+	__be32		t_checksum;	/* crc32c(uuid+descr_block) */
+};
+
 /*
  * The revoke descriptor: used on disk to describe a series of blocks to
  * be revoked from the log
@@ -192,6 +210,10 @@  typedef struct jbd2_journal_revoke_header_s
 	__be32		 r_count;	/* Count of bytes used in the block */
 } jbd2_journal_revoke_header_t;
 
+/* Tail of revoke block, for checksumming */
+struct jbd2_journal_revoke_tail {
+	__be32		r_checksum;	/* crc32c(uuid+revoke_block) */
+};
 
 /* Definitions for the journal tag flags word: */
 #define JBD2_FLAG_ESCAPE		1	/* on-disk block is escaped */
@@ -241,7 +263,10 @@  typedef struct journal_superblock_s
 	__be32	s_max_trans_data;	/* Limit of data blocks per trans. */
 
 /* 0x0050 */
-	__u32	s_padding[44];
+	__u8	s_checksum_type;	/* checksum type */
+	__u8	s_padding2[3];
+	__u32	s_padding[42];
+	__be32	s_checksum;		/* crc32c(superblock) */
 
 /* 0x0100 */
 	__u8	s_users[16*48];		/* ids of all fs'es sharing the log */
@@ -263,6 +288,7 @@  typedef struct journal_superblock_s
 #define JBD2_FEATURE_INCOMPAT_REVOKE		0x00000001
 #define JBD2_FEATURE_INCOMPAT_64BIT		0x00000002
 #define JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT	0x00000004
+#define JBD2_FEATURE_INCOMPAT_CSUM_V2		0x00000008
 
 /* Features known to this kernel version: */
 #define JBD2_KNOWN_COMPAT_FEATURES	JBD2_FEATURE_COMPAT_CHECKSUM