[PATCH 0/2] Add inode checksum support to ext4

[PATCH 0/2] Add inode checksum support to ext4

[Darrick J. Wong]

Hi all,

I spent last week analyzing a client's corrupted ext3 image to see if I could
determine what had gone wrong and caused the filesystem to blow apart.  As best
as I could tell, a data block got miswritten into a different sector ... which
happened to be an indirect block.  Some time later the indirect block, which
now pointed at one of the inode tables (among other things that shouldn't ever
become file data) was loaded as part of a file write, which caused that inode
table to be blown to smithereens.  Just for fun I tried reading from one of
these busted-inode files and ... failed to encounter any errors.  Somehow, they
didn't find it funny that ext3 would read block numbers from a table with the
contents "ibm.com" with a straight face.  Fortunately there were backups. :)

The client at this point asked if ext4 would do a better job of sanity
checking, which got me to wonder why ext4 checksums block groups but not
inodes.  It's on Ted's todo list, but apparently nobody wrote any patch, so I
did.  The following two patches are a first draft of adding inode checksum
support to both the kernel driver and to the various e2fsprogs.

If you have an existing ext* fs with 256-byte inodes, you ought to be able to
"tune2fs -O inode_csum /dev/XXX", fsck /dev/XXX, and mount the filesystem with
checksumming enabled.  It seems to work for me (i386/x86-64), but I'm looking
for comments for improvement and perhaps some more testing (ppc64 is still
building).  This inode checksum feature is not enabled by default.

--D

[PATCH 1/2] ext4: Calculate and verify inode checksums

[Darrick J. Wong]

This patch introduces to ext4 the ability to calculate and verify inode
checksums.  This requires the use of a new ro compatibility flag and some
accompanying e2fsprogs patches to provide the relevant features in tune2fs and
e2fsck.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 fs/ext4/ext4.h  |    6 ++++--
 fs/ext4/inode.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 4daaf2b..8815928 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -617,7 +617,7 @@ struct ext4_inode {
 		} masix2;
 	} osd2;				/* OS dependent 2 */
 	__le16	i_extra_isize;
-	__le16	i_pad1;
+	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
 	__le32  i_ctime_extra;  /* extra Change time      (nsec << 2 | epoch) */
 	__le32  i_mtime_extra;  /* extra Modification time(nsec << 2 | epoch) */
 	__le32  i_atime_extra;  /* extra Access time      (nsec << 2 | epoch) */
@@ -1338,6 +1338,7 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
 #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
 #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
 #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+#define EXT4_FEATURE_RO_COMPAT_INODE_CSUM	0x0400
 
 #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
 #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
@@ -1364,7 +1365,8 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
 					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
 					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
 					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
-					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
+					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
+					 EXT4_FEATURE_RO_COMPAT_INODE_CSUM)
 
 /*
  * Default values for user and/or group using reserved blocks
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 1a86282..dc76f19 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -42,6 +42,7 @@
 #include <linux/printk.h>
 #include <linux/slab.h>
 #include <linux/ratelimit.h>
+#include <linux/crc16.h>
 
 #include "ext4_jbd2.h"
 #include "xattr.h"
@@ -52,6 +53,40 @@
 
 #define MPAGE_DA_EXTENT_TAIL 0x01
 
+static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	struct ext4_inode_info *ei = EXT4_I(inode);
+	int offset = offsetof(struct ext4_inode, i_checksum);
+	__le32 inum = cpu_to_le32(inode->i_ino);
+	__u16 crc = 0;
+
+	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
+	    le16_to_cpu(raw->i_extra_isize) >= 4) {
+		crc = crc16(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
+		crc = crc16(crc, (__u8 *)&inum, sizeof(inum));
+		crc = crc16(crc, (__u8 *)raw, offset);
+		offset += sizeof(raw->i_checksum); /* skip checksum */
+		/* for checksum of struct ext4_inode do the rest...*/
+		if (ei->i_extra_isize > 4)
+			crc = crc16(crc, (__u8 *)raw + offset,
+				    ei->i_extra_isize - 4);
+	}
+
+	return cpu_to_le16(crc);
+}
+
+static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw)
+{
+	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
+	    (raw->i_checksum != ext4_inode_csum(inode, raw)))
+		return 0;
+
+	return 1;
+}
+
 static inline int ext4_begin_ordered_truncate(struct inode *inode,
 					      loff_t new_size)
 {
@@ -4802,7 +4837,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	struct ext4_inode *raw_inode;
 	struct ext4_inode_info *ei;
 	struct inode *inode;
-	journal_t *journal = EXT4_SB(sb)->s_journal;
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	journal_t *journal = sbi->s_journal;
 	long ret;
 	int block;
 
@@ -4916,6 +4952,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	} else
 		ei->i_extra_isize = 0;
 
+	if (!ext4_inode_csum_verify(inode, raw_inode)) {
+		EXT4_ERROR_INODE(inode, "checksum invalid (%u != %u)",
+		       le16_to_cpu(ext4_inode_csum(inode, raw_inode)),
+		       le16_to_cpu(raw_inode->i_checksum));
+		ret = -EIO;
+		goto bad_inode;
+	}
+
 	EXT4_INODE_GET_XTIME(i_ctime, inode, raw_inode);
 	EXT4_INODE_GET_XTIME(i_mtime, inode, raw_inode);
 	EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
@@ -5138,6 +5182,12 @@ static int ext4_do_update_inode(handle_t *handle,
 			raw_inode->i_version_hi =
 			cpu_to_le32(inode->i_version >> 32);
 		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
+
+		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
+			EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
+		    EXT4_FITS_IN_INODE(raw_inode, ei, i_checksum))
+			raw_inode->i_checksum =
+				ext4_inode_csum(inode, raw_inode);
 	}
 
 	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Sunil Mushran]

On 04/06/2011 03:45 PM, Darrick J. Wong wrote:
> This patch introduces to ext4 the ability to calculate and verify inode
> checksums.  This requires the use of a new ro compatibility flag and some
> accompanying e2fsprogs patches to provide the relevant features in tune2fs and
> e2fsck.
>
> Signed-off-by: Darrick J. Wong<djwong@us.ibm.com>
> ---
>
>   fs/ext4/ext4.h  |    6 ++++--
>   fs/ext4/inode.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4daaf2b..8815928 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -617,7 +617,7 @@ struct ext4_inode {
>   		} masix2;
>   	} osd2;				/* OS dependent 2 */
>   	__le16	i_extra_isize;
> -	__le16	i_pad1;
> +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
>   	__le32  i_ctime_extra;  /* extra Change time      (nsec<<  2 | epoch) */
>   	__le32  i_mtime_extra;  /* extra Modification time(nsec<<  2 | epoch) */
>   	__le32  i_atime_extra;  /* extra Access time      (nsec<<  2 | epoch) */
> @@ -1338,6 +1338,7 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
>   #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
>   #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
>   #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
> +#define EXT4_FEATURE_RO_COMPAT_INODE_CSUM	0x0400
>
>   #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
>   #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
> @@ -1364,7 +1365,8 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
>   					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
>   					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
>   					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
> -					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
> +					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
> +					 EXT4_FEATURE_RO_COMPAT_INODE_CSUM)
>
>   /*
>    * Default values for user and/or group using reserved blocks
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 1a86282..dc76f19 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -42,6 +42,7 @@
>   #include<linux/printk.h>
>   #include<linux/slab.h>
>   #include<linux/ratelimit.h>
> +#include<linux/crc16.h>
>
>   #include "ext4_jbd2.h"
>   #include "xattr.h"
> @@ -52,6 +53,40 @@
>
>   #define MPAGE_DA_EXTENT_TAIL 0x01
>
> +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	int offset = offsetof(struct ext4_inode, i_checksum);
> +	__le32 inum = cpu_to_le32(inode->i_ino);
> +	__u16 crc = 0;
> +
> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
> +	    le16_to_cpu(raw->i_extra_isize)>= 4) {
> +		crc = crc16(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
> +		crc = crc16(crc, (__u8 *)&inum, sizeof(inum));
> +		crc = crc16(crc, (__u8 *)raw, offset);
> +		offset += sizeof(raw->i_checksum); /* skip checksum */
> +		/* for checksum of struct ext4_inode do the rest...*/
> +		if (ei->i_extra_isize>  4)
> +			crc = crc16(crc, (__u8 *)raw + offset,
> +				    ei->i_extra_isize - 4);
> +	}
> +
> +	return cpu_to_le16(crc);
> +}
> +
> +static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw)
> +{
> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
> +	    (raw->i_checksum != ext4_inode_csum(inode, raw)))
> +		return 0;
> +
> +	return 1;
> +}
> +
>   static inline int ext4_begin_ordered_truncate(struct inode *inode,
>   					      loff_t new_size)
>   {
> @@ -4802,7 +4837,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>   	struct ext4_inode *raw_inode;
>   	struct ext4_inode_info *ei;
>   	struct inode *inode;
> -	journal_t *journal = EXT4_SB(sb)->s_journal;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	journal_t *journal = sbi->s_journal;
>   	long ret;
>   	int block;
>
> @@ -4916,6 +4952,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>   	} else
>   		ei->i_extra_isize = 0;
>
> +	if (!ext4_inode_csum_verify(inode, raw_inode)) {
> +		EXT4_ERROR_INODE(inode, "checksum invalid (%u != %u)",
> +		       le16_to_cpu(ext4_inode_csum(inode, raw_inode)),
> +		       le16_to_cpu(raw_inode->i_checksum));
> +		ret = -EIO;
> +		goto bad_inode;
> +	}
> +
>   	EXT4_INODE_GET_XTIME(i_ctime, inode, raw_inode);
>   	EXT4_INODE_GET_XTIME(i_mtime, inode, raw_inode);
>   	EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
> @@ -5138,6 +5182,12 @@ static int ext4_do_update_inode(handle_t *handle,
>   			raw_inode->i_version_hi =
>   			cpu_to_le32(inode->i_version>>  32);
>   		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> +
> +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +			EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
> +		    EXT4_FITS_IN_INODE(raw_inode, ei, i_checksum))
> +			raw_inode->i_checksum =
> +				ext4_inode_csum(inode, raw_inode);
>   	}
>
>   	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");

You may want to look into jbd2 buffer triggers. struct jbd2_buffer_trigger_type
Instead of computing checksum on every update, it allows one to compute it
just before the journal write. More efficient.

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Darrick J. Wong]

On Wed, Apr 06, 2011 at 05:52:43PM -0700, Sunil Mushran wrote:
> On 04/06/2011 03:45 PM, Darrick J. Wong wrote:
>> This patch introduces to ext4 the ability to calculate and verify inode
>> checksums.  This requires the use of a new ro compatibility flag and some
>> accompanying e2fsprogs patches to provide the relevant features in tune2fs and
>> e2fsck.
>>
>> Signed-off-by: Darrick J. Wong<djwong@us.ibm.com>
>> ---
>>
>>   fs/ext4/ext4.h  |    6 ++++--
>>   fs/ext4/inode.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 55 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 4daaf2b..8815928 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -617,7 +617,7 @@ struct ext4_inode {
>>   		} masix2;
>>   	} osd2;				/* OS dependent 2 */
>>   	__le16	i_extra_isize;
>> -	__le16	i_pad1;
>> +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
>>   	__le32  i_ctime_extra;  /* extra Change time      (nsec<<  2 | epoch) */
>>   	__le32  i_mtime_extra;  /* extra Modification time(nsec<<  2 | epoch) */
>>   	__le32  i_atime_extra;  /* extra Access time      (nsec<<  2 | epoch) */
>> @@ -1338,6 +1338,7 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
>>   #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
>>   #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
>>   #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
>> +#define EXT4_FEATURE_RO_COMPAT_INODE_CSUM	0x0400
>>
>>   #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
>>   #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
>> @@ -1364,7 +1365,8 @@ static inline void ext4_clear_state_flags(struct ext4_inode_info *ei)
>>   					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
>>   					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
>>   					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
>> -					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
>> +					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
>> +					 EXT4_FEATURE_RO_COMPAT_INODE_CSUM)
>>
>>   /*
>>    * Default values for user and/or group using reserved blocks
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 1a86282..dc76f19 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -42,6 +42,7 @@
>>   #include<linux/printk.h>
>>   #include<linux/slab.h>
>>   #include<linux/ratelimit.h>
>> +#include<linux/crc16.h>
>>
>>   #include "ext4_jbd2.h"
>>   #include "xattr.h"
>> @@ -52,6 +53,40 @@
>>
>>   #define MPAGE_DA_EXTENT_TAIL 0x01
>>
>> +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
>> +{
>> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>> +	struct ext4_inode_info *ei = EXT4_I(inode);
>> +	int offset = offsetof(struct ext4_inode, i_checksum);
>> +	__le32 inum = cpu_to_le32(inode->i_ino);
>> +	__u16 crc = 0;
>> +
>> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
>> +	    le16_to_cpu(raw->i_extra_isize)>= 4) {
>> +		crc = crc16(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
>> +		crc = crc16(crc, (__u8 *)&inum, sizeof(inum));
>> +		crc = crc16(crc, (__u8 *)raw, offset);
>> +		offset += sizeof(raw->i_checksum); /* skip checksum */
>> +		/* for checksum of struct ext4_inode do the rest...*/
>> +		if (ei->i_extra_isize>  4)
>> +			crc = crc16(crc, (__u8 *)raw + offset,
>> +				    ei->i_extra_isize - 4);
>> +	}
>> +
>> +	return cpu_to_le16(crc);
>> +}
>> +
>> +static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw)
>> +{
>> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
>> +	    (raw->i_checksum != ext4_inode_csum(inode, raw)))
>> +		return 0;
>> +
>> +	return 1;
>> +}
>> +
>>   static inline int ext4_begin_ordered_truncate(struct inode *inode,
>>   					      loff_t new_size)
>>   {
>> @@ -4802,7 +4837,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>>   	struct ext4_inode *raw_inode;
>>   	struct ext4_inode_info *ei;
>>   	struct inode *inode;
>> -	journal_t *journal = EXT4_SB(sb)->s_journal;
>> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> +	journal_t *journal = sbi->s_journal;
>>   	long ret;
>>   	int block;
>>
>> @@ -4916,6 +4952,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>>   	} else
>>   		ei->i_extra_isize = 0;
>>
>> +	if (!ext4_inode_csum_verify(inode, raw_inode)) {
>> +		EXT4_ERROR_INODE(inode, "checksum invalid (%u != %u)",
>> +		       le16_to_cpu(ext4_inode_csum(inode, raw_inode)),
>> +		       le16_to_cpu(raw_inode->i_checksum));
>> +		ret = -EIO;
>> +		goto bad_inode;
>> +	}
>> +
>>   	EXT4_INODE_GET_XTIME(i_ctime, inode, raw_inode);
>>   	EXT4_INODE_GET_XTIME(i_mtime, inode, raw_inode);
>>   	EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
>> @@ -5138,6 +5182,12 @@ static int ext4_do_update_inode(handle_t *handle,
>>   			raw_inode->i_version_hi =
>>   			cpu_to_le32(inode->i_version>>  32);
>>   		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
>> +
>> +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>> +			EXT4_FEATURE_RO_COMPAT_INODE_CSUM)&&
>> +		    EXT4_FITS_IN_INODE(raw_inode, ei, i_checksum))
>> +			raw_inode->i_checksum =
>> +				ext4_inode_csum(inode, raw_inode);
>>   	}
>>
>>   	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
>
> You may want to look into jbd2 buffer triggers. struct jbd2_buffer_trigger_type
> Instead of computing checksum on every update, it allows one to compute it
> just before the journal write. More efficient.

Yes, I see that jbd2 has triggers, looks like a nifty feature.  I suppose if I
went with that approach I'd still have to calculate the checksum in
ext4_do_update_inode in the nojournal case, and in the journal case I'd write a
trigger that would figure out which inodes in a given buffer are actually dirty
and compute their checksums.

That said, I haven't really quantified the performance impact of this naive
approach yet, so I wonder -- did you see a similar scenario with ocfs2, and
what kind of performance increase did you get by adapting the code to use the
jbd2 trigger?  If there's potentially a large increase, it would be interesting
to apply the same conversion to the group descriptor checksumming code too.

--D

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Sunil Mushran]

On 04/07/2011 09:40 AM, Darrick J. Wong wrote:
> Yes, I see that jbd2 has triggers, looks like a nifty feature.  I suppose if I
> went with that approach I'd still have to calculate the checksum in
> ext4_do_update_inode in the nojournal case, and in the journal case I'd write a
> trigger that would figure out which inodes in a given buffer are actually dirty
> and compute their checksums.

Yes, you could use a in-mem flag (set in update, cleared in trigger) to
identify dirty inodes. The discussion on stable pages could be relevant for
the nojournal case.

>
> That said, I haven't really quantified the performance impact of this naive
> approach yet, so I wonder -- did you see a similar scenario with ocfs2, and
> what kind of performance increase did you get by adapting the code to use the
> jbd2 trigger?  If there's potentially a large increase, it would be interesting
> to apply the same conversion to the group descriptor checksumming code too.

Joel Becker may remember the overhead. He wrote the patch. That said we have few
differences. ocfs2 has larger (blocksized) inodes. Also, it computes ECC. The code
is in fs/ocfs2/blockcheck.c.

As in, you may want to generate ext4 specific numbers.

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Joel Becker]

On Thu, Apr 07, 2011 at 10:10:52AM -0700, Sunil Mushran wrote:
> On 04/07/2011 09:40 AM, Darrick J. Wong wrote:
> >That said, I haven't really quantified the performance impact of this naive
> >approach yet, so I wonder -- did you see a similar scenario with ocfs2, and
> >what kind of performance increase did you get by adapting the code to use the
> >jbd2 trigger?  If there's potentially a large increase, it would be interesting
> >to apply the same conversion to the group descriptor checksumming code too.
> 
> Joel Becker may remember the overhead. He wrote the patch. That said we have few
> differences. ocfs2 has larger (blocksized) inodes. Also, it computes ECC. The code
> is in fs/ocfs2/blockcheck.c.

	ocfs2 does the journal access/journal dirty cycle a lot more
than extN.  I think you'd want to generate your own numbers.

Joel


-- 

"Conservative, n.  A statesman who is enamoured of existing evils,
 as distinguished from the Liberal, who wishes to replace them
 with others."
	- Ambrose Bierce, The Devil's Dictionary

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Darrick J. Wong]

On Fri, Apr 08, 2011 at 11:50:13AM -0700, Joel Becker wrote:
> On Thu, Apr 07, 2011 at 10:10:52AM -0700, Sunil Mushran wrote:
> > On 04/07/2011 09:40 AM, Darrick J. Wong wrote:
> > >That said, I haven't really quantified the performance impact of this naive
> > >approach yet, so I wonder -- did you see a similar scenario with ocfs2, and
> > >what kind of performance increase did you get by adapting the code to use the
> > >jbd2 trigger?  If there's potentially a large increase, it would be interesting
> > >to apply the same conversion to the group descriptor checksumming code too.
> > 
> > Joel Becker may remember the overhead. He wrote the patch. That said we have few
> > differences. ocfs2 has larger (blocksized) inodes. Also, it computes ECC. The code
> > is in fs/ocfs2/blockcheck.c.

Heh, yes, ext4 uses a fairly simple crc16 and the inodes are (most likely) not
block sized.

> 	ocfs2 does the journal access/journal dirty cycle a lot more
> than extN.  I think you'd want to generate your own numbers.

Ok, I ran both the mailserver ffsb profile and a quick-and-dumb test that tried
to dirty inodes as fast as it could.  On both a regular disk, an SSD, and a
loopmounted ext4 on a tmpfs I couldn't really see much of a performance
difference at all.  I'll see about giving this a try once I get the field
location and e2fsck behavior more firmly resolved, though I suspect I won't see
much gain.

--D

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Andreas Dilger]

On 2011-04-06, at 4:45 PM, Darrick J. Wong wrote:
> This patch introduces to ext4 the ability to calculate and verify inode
> checksums.  This requires the use of a new ro compatibility flag and some
> accompanying e2fsprogs patches to provide the relevant features in tune2fs and
> e2fsck.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
> fs/ext4/ext4.h  |    6 ++++--
> fs/ext4/inode.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4daaf2b..8815928 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -617,7 +617,7 @@ struct ext4_inode {
> 		} masix2;
> 	} osd2;				/* OS dependent 2 */
> 	__le16	i_extra_isize;
> -	__le16	i_pad1;
> +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */

In previous discussions, we thought about using part/all of the l_i_reserved2 field to store the inode checksum.  The inode checksum is important enough to warrant a field in the core inode, so that it also works on upgraded filesystems.

> +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +	struct ext4_inode_info *ei = EXT4_I(inode);
> +	int offset = offsetof(struct ext4_inode, i_checksum);
> +	__le32 inum = cpu_to_le32(inode->i_ino);
> +	__u16 crc = 0;
> +
> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
> +	    le16_to_cpu(raw->i_extra_isize) >= 4) {

If this field remains in the large part of the inode, then this check is incorrect.  i_extra_isize is itself only valid if (s_inode_size is > EXT4_GOOD_OLD_INODE_SIZE), otherwise it points at the next inode's i_mode.

Also, instead of hard-coding ">= 4" here, it would be better to use EXT4_FITS_IN_INODE(raw, EXT4_I(inode), i_checksum).  Probably no reason to put "ei" on the stack at all.

> +		crc = crc16(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
> +		crc = crc16(crc, (__u8 *)&inum, sizeof(inum));
> +		crc = crc16(crc, (__u8 *)raw, offset);
> +		offset += sizeof(raw->i_checksum); /* skip checksum */
> +		/* for checksum of struct ext4_inode do the rest...*/
> +		if (ei->i_extra_isize > 4)
> +			crc = crc16(crc, (__u8 *)raw + offset,
> +				    ei->i_extra_isize - 4);


> +	}
> +
> +	return cpu_to_le16(crc);
> +}
> +
> +static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw)
> +{
> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
> +	    (raw->i_checksum != ext4_inode_csum(inode, raw)))
> +		return 0;
> +
> +	return 1;
> +}
> +
> static inline int ext4_begin_ordered_truncate(struct inode *inode,
> 					      loff_t new_size)
> {
> @@ -4802,7 +4837,8 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 	struct ext4_inode *raw_inode;
> 	struct ext4_inode_info *ei;
> 	struct inode *inode;
> -	journal_t *journal = EXT4_SB(sb)->s_journal;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	journal_t *journal = sbi->s_journal;
> 	long ret;
> 	int block;
> 
> @@ -4916,6 +4952,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
> 	} else
> 		ei->i_extra_isize = 0;
> 
> +	if (!ext4_inode_csum_verify(inode, raw_inode)) {
> +		EXT4_ERROR_INODE(inode, "checksum invalid (%u != %u)",
> +		       le16_to_cpu(ext4_inode_csum(inode, raw_inode)),
> +		       le16_to_cpu(raw_inode->i_checksum));
> +		ret = -EIO;
> +		goto bad_inode;
> +	}
> +
> 	EXT4_INODE_GET_XTIME(i_ctime, inode, raw_inode);
> 	EXT4_INODE_GET_XTIME(i_mtime, inode, raw_inode);
> 	EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
> @@ -5138,6 +5182,12 @@ static int ext4_do_update_inode(handle_t *handle,
> 			raw_inode->i_version_hi =
> 			cpu_to_le32(inode->i_version >> 32);
> 		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> +
> +		if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +			EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
> +		    EXT4_FITS_IN_INODE(raw_inode, ei, i_checksum))
> +			raw_inode->i_checksum =
> +				ext4_inode_csum(inode, raw_inode);
> 	}
> 
> 	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");


Cheers, Andreas

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Darrick J. Wong]

On Fri, Apr 08, 2011 at 02:58:27AM -0600, Andreas Dilger wrote:
> On 2011-04-06, at 4:45 PM, Darrick J. Wong wrote:
> > This patch introduces to ext4 the ability to calculate and verify inode
> > checksums.  This requires the use of a new ro compatibility flag and some
> > accompanying e2fsprogs patches to provide the relevant features in tune2fs and
> > e2fsck.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> > 
> > fs/ext4/ext4.h  |    6 ++++--
> > fs/ext4/inode.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 55 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 4daaf2b..8815928 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -617,7 +617,7 @@ struct ext4_inode {
> > 		} masix2;
> > 	} osd2;				/* OS dependent 2 */
> > 	__le16	i_extra_isize;
> > -	__le16	i_pad1;
> > +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
> 
> In previous discussions, we thought about using part/all of the l_i_reserved2
> field to store the inode checksum.  The inode checksum is important enough to
> warrant a field in the core inode, so that it also works on upgraded
> filesystems.

Hmm, yes, I better like using that field too, it'll simplify the code.  Does
anyone know which of the s_creator_os codes map to the Linux format?  I'm going
to guess 1 and 2 don't... for now I suppose the mount code can check for a
Linux creator code if the inode checksum feature is set, and fail the mount if
it finds 1 or 2.  I'm not sure which version of the osd2 union FreeBSD or
"Lites" use.

> > +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
> > +{
> > +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> > +	struct ext4_inode_info *ei = EXT4_I(inode);
> > +	int offset = offsetof(struct ext4_inode, i_checksum);
> > +	__le32 inum = cpu_to_le32(inode->i_ino);
> > +	__u16 crc = 0;
> > +
> > +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> > +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
> > +	    le16_to_cpu(raw->i_extra_isize) >= 4) {
> 
> If this field remains in the large part of the inode, then this check is
> incorrect.  i_extra_isize is itself only valid if (s_inode_size is >
> EXT4_GOOD_OLD_INODE_SIZE), otherwise it points at the next inode's i_mode.
> 
> Also, instead of hard-coding ">= 4" here, it would be better to use
> EXT4_FITS_IN_INODE(raw, EXT4_I(inode), i_checksum).  Probably no reason to
> put "ei" on the stack at all.

As always, thank you for the feedback!

--D

Re: [PATCH 1/2] ext4: Calculate and verify inode checksums

[Andreas Dilger]

On 2011-04-08, at 1:12 PM, Darrick J. Wong wrote:
> On Fri, Apr 08, 2011 at 02:58:27AM -0600, Andreas Dilger wrote:
>> On 2011-04-06, at 4:45 PM, Darrick J. Wong wrote:
>>> @@ -617,7 +617,7 @@ struct ext4_inode {
>>> 		} masix2;
>>> 	} osd2;				/* OS dependent 2 */
>>> 	__le16	i_extra_isize;
>>> -	__le16	i_pad1;
>>> +	__le16  i_checksum;		/* crc16(sb_uuid+inodenum+inode) */
>> 
>> In previous discussions, we thought about using part/all of the l_i_reserved2
>> field to store the inode checksum.  The inode checksum is important enough to
>> warrant a field in the core inode, so that it also works on upgraded
>> filesystems.
> 
> Hmm, yes, I better like using that field too, it'll simplify the code.  Does
> anyone know which of the s_creator_os codes map to the Linux format?  I'm going
> to guess 1 and 2 don't... for now I suppose the mount code can check for a
> Linux creator code if the inode checksum feature is set, and fail the mount if
> it finds 1 or 2.  I'm not sure which version of the osd2 union FreeBSD or
> "Lites" use.

Actually, the e2fsprogs code has already removed the "masix" part of the union years ago, and I'm not even sure why the "hurd" part of the union remains in the kernel code.  I think only Hurd is using the "h_i_author" field, so I suspect all that is needed is to refuse tune2fs to enable EXT4_FEATURE_RO_COMPAT_INODE_CSUM if s_creator_os == EXT2_OS_HURD (1).

The kernel shouldn't really care about this field - if the checksum feature isn't enabled, then this field will be ignored, and if the checksum feature IS enabled it could only have been done by mke2fs, or tune2fs which should check if it is allowed based on s_creator_os.

>>> +static __le16 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw)
>>> +{
>>> +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>>> +	struct ext4_inode_info *ei = EXT4_I(inode);
>>> +	int offset = offsetof(struct ext4_inode, i_checksum);
>>> +	__le32 inum = cpu_to_le32(inode->i_ino);
>>> +	__u16 crc = 0;
>>> +
>>> +	if (EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
>>> +		EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
>>> +	    le16_to_cpu(raw->i_extra_isize) >= 4) {
>> 
>> If this field remains in the large part of the inode, then this check is
>> incorrect.  i_extra_isize is itself only valid if (s_inode_size is >
>> EXT4_GOOD_OLD_INODE_SIZE), otherwise it points at the next inode's i_mode.
>> 
>> Also, instead of hard-coding ">= 4" here, it would be better to use
>> EXT4_FITS_IN_INODE(raw, EXT4_I(inode), i_checksum).  Probably no reason to
>> put "ei" on the stack at all.
> 
> As always, thank you for the feedback!
> 
> --D


Cheers, Andreas

[PATCH 2/2] e2fsprogs: Add support for toggling, verifying, and fixing inode checksums

[Darrick J. Wong]

This patch adds to tune2fs the ability to toggle the inode checksum rocompat
feature flag, to e2fsck the ability to verify and correct inode checksums, and
to debugfs the ability to dump inode checksums.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 debugfs/debugfs.c         |    6 +++++
 e2fsck/pass1.c            |    9 ++++++-
 e2fsck/problem.c          |    5 ++++
 e2fsck/problem.h          |    3 ++
 lib/blkid/probe.h         |    1 +
 lib/e2p/feature.c         |    2 ++
 lib/ext2fs/csum.c         |   59 +++++++++++++++++++++++++++++++++++++++++++++
 lib/ext2fs/ext2_err.et.in |    3 ++
 lib/ext2fs/ext2_fs.h      |    3 ++
 lib/ext2fs/ext2fs.h       |    9 ++++++-
 lib/ext2fs/inode.c        |   17 +++++++++++++
 lib/ext2fs/swapfs.c       |    2 ++
 misc/tune2fs.c            |   18 ++++++++++++--
 13 files changed, 132 insertions(+), 5 deletions(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index e441bc5..182ac6a 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -509,6 +509,12 @@ static void internal_dump_inode_extra(FILE *out,
 				inode->i_extra_isize);
 		return;
 	}
+
+	if (current_fs->super->s_feature_ro_compat &
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM &&
+	    inode->i_extra_isize > 4)
+		fprintf(out, "Inode checksum: %x\n", inode->i_checksum);
+
 	storage_size = EXT2_INODE_SIZE(current_fs->super) -
 			EXT2_GOOD_OLD_INODE_SIZE -
 			inode->i_extra_isize;
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 67dd986..58f24bb 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -365,7 +365,7 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
 			inode->i_extra_isize);
 #endif
 	/* i_extra_isize must cover i_extra_isize + i_pad1 at least */
-	min = sizeof(inode->i_extra_isize) + sizeof(inode->i_pad1);
+	min = sizeof(inode->i_extra_isize) + sizeof(inode->i_checksum);
 	max = EXT2_INODE_SIZE(sb) - EXT2_GOOD_OLD_INODE_SIZE;
 	/*
 	 * For now we will allow i_extra_isize to be 0, but really
@@ -729,6 +729,13 @@ void e2fsck_pass1(e2fsck_t ctx)
 			}
 		}
 
+		/* Check for invalid inode checksum */
+		if (!ext2fs_inode_csum_verify(fs, ino,
+			(struct ext2_inode_large *)inode) &&
+		    fix_problem(ctx, PR_1_INODE_CSUM_INVALID, &pctx))
+			e2fsck_write_inode_full(ctx, ino, inode,
+				sizeof(struct ext2_inode_large), "pass1");
+
 		/*
 		 * Test for incorrect extent flag settings.
 		 *
diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index 8f0b211..6785fa3 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -890,6 +890,11 @@ static struct e2fsck_problem problem_table[] = {
 	     "(size %Is, lblk %r)\n"),
 	  PROMPT_CLEAR, PR_PREEN_OK },
 
+	/* Fast symlink has EXTENTS_FL set */
+	{ PR_1_INODE_CSUM_INVALID,
+	  N_("inode %i checksum invalid.  "),
+	  PROMPT_FIX, PR_PREEN_OK },
+
 	/* Pass 1b errors */
 
 	/* Pass 1B: Rescan for duplicate/bad blocks */
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index 7c4c156..388d153 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -520,6 +520,9 @@ struct problem_context {
 /* EOFBLOCKS flag set when not necessary */
 #define PR_1_EOFBLOCKS_FL_SET		0x010060
 
+/* inode checksum invalid */
+#define PR_1_INODE_CSUM_INVALID		0x010061
+
 /*
  * Pass 1b errors
  */
diff --git a/lib/blkid/probe.h b/lib/blkid/probe.h
index 37e80ef..e01a689 100644
--- a/lib/blkid/probe.h
+++ b/lib/blkid/probe.h
@@ -110,6 +110,7 @@ struct ext2_super_block {
 #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
 #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
 #define EXT4_FEATURE_RO_COMPAT_QUOTA		0x0100
+#define EXT4_FEATURE_RO_COMPAT_INODE_CSUM	0x0400
 
 /* for s_feature_incompat */
 #define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
diff --git a/lib/e2p/feature.c b/lib/e2p/feature.c
index 16fba53..876f0e1 100644
--- a/lib/e2p/feature.c
+++ b/lib/e2p/feature.c
@@ -59,6 +59,8 @@ static struct feature feature_list[] = {
 			"quota" },
 	{	E2P_FEATURE_RO_INCOMPAT, EXT4_FEATURE_RO_COMPAT_BIGALLOC,
 			"bigalloc"},
+	{	E2P_FEATURE_RO_INCOMPAT, EXT4_FEATURE_RO_COMPAT_INODE_CSUM,
+			"inode_csum"},
 
 	{	E2P_FEATURE_INCOMPAT, EXT2_FEATURE_INCOMPAT_COMPRESSION,
 			"compression" },
diff --git a/lib/ext2fs/csum.c b/lib/ext2fs/csum.c
index 58e2971..3621bd7 100644
--- a/lib/ext2fs/csum.c
+++ b/lib/ext2fs/csum.c
@@ -29,6 +29,65 @@
 #define STATIC static
 #endif
 
+__u16 ext2fs_inode_csum(ext2_filsys fs, ext2_ino_t inum,
+			struct ext2_inode_large *inode)
+{
+	int offset = offsetof(struct ext2_inode_large, i_checksum);
+	int extra_size = inode->i_extra_isize;
+	size_t size = fs->super->s_inode_size;
+	__u16 crc = 0;
+
+	if (fs->super->s_feature_ro_compat &
+	    EXT4_FEATURE_RO_COMPAT_INODE_CSUM &&
+	    extra_size >= 4) {
+#ifdef WORDS_BIGENDIAN
+		struct ext4_inode_large swabinode;
+
+		/* Have to swab back to little-endian to do the checksum */
+		memcpy(&swabinode, inode, size);
+		ext2fs_swap_inode_full(fs, inode, inode, 0, size);
+		desc = &swabinode;
+
+		inum = ext2fs_swab32(inum);
+#endif
+		crc = ext2fs_crc16(~0, fs->super->s_uuid,
+				   sizeof(fs->super->s_uuid));
+		crc = ext2fs_crc16(crc, &inum, sizeof(inum));
+		crc = ext2fs_crc16(crc, inode, offset);
+		offset += sizeof(inode->i_checksum); /* skip checksum */
+		/* for checksum of struct ext4_group_desc do the rest...*/
+		if (extra_size > 4) {
+			crc = ext2fs_crc16(crc, (char *)inode + offset,
+					   extra_size - 4);
+		}
+	}
+
+	return crc;
+}
+
+int ext2fs_inode_csum_verify(ext2_filsys fs, ext2_ino_t inum,
+			     struct ext2_inode_large *inode)
+{
+	if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
+				       EXT4_FEATURE_RO_COMPAT_INODE_CSUM) &&
+	    (inode->i_checksum != ext2fs_inode_csum(fs, inum, inode)))
+		return 0;
+
+	return 1;
+}
+
+void ext2fs_inode_csum_set(ext2_filsys fs, ext2_ino_t inum,
+			   struct ext2_inode_large *inode)
+{
+	if (!EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
+					EXT4_FEATURE_RO_COMPAT_INODE_CSUM))
+		return;
+
+	/* ext2fs_bg_checksum_set() sets the actual checksum field but
+	 * does not calculate the checksum itself. */
+	inode->i_checksum = ext2fs_inode_csum(fs, inum, inode);
+}
+
 STATIC __u16 ext2fs_group_desc_csum(ext2_filsys fs, dgrp_t group)
 {
 	__u16 crc = 0;
diff --git a/lib/ext2fs/ext2_err.et.in b/lib/ext2fs/ext2_err.et.in
index 995ddc3..cc9ac3d 100644
--- a/lib/ext2fs/ext2_err.et.in
+++ b/lib/ext2fs/ext2_err.et.in
@@ -422,4 +422,7 @@ ec	EXT2_NO_MTAB_FILE,
 ec	EXT2_ET_CANT_USE_LEGACY_BITMAPS,
 	"Filesystem too large to use legacy bitmaps"
 
+ec	EXT2_ET_INODE_CSUM_INVALID,
+	"Inode checksum is invalid"
+
 	end
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index a89e33b..dfedd4e 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -416,7 +416,7 @@ struct ext2_inode_large {
 		} hurd2;
 	} osd2;				/* OS dependent 2 */
 	__u16	i_extra_isize;
-	__u16	i_pad1;
+	__u16	i_checksum;
 	__u32	i_ctime_extra;	/* extra Change time (nsec << 2 | epoch) */
 	__u32	i_mtime_extra;	/* extra Modification time (nsec << 2 | epoch) */
 	__u32	i_atime_extra;	/* extra Access time (nsec << 2 | epoch) */
@@ -677,6 +677,7 @@ struct ext2_super_block {
 #define EXT4_FEATURE_RO_COMPAT_HAS_SNAPSHOT	0x0080
 #define EXT4_FEATURE_RO_COMPAT_QUOTA		0x0100
 #define EXT4_FEATURE_RO_COMPAT_BIGALLOC		0x0200
+#define EXT4_FEATURE_RO_COMPAT_INODE_CSUM	0x0400
 
 #define EXT2_FEATURE_INCOMPAT_COMPRESSION	0x0001
 #define EXT2_FEATURE_INCOMPAT_FILETYPE		0x0002
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index d3eb31d..cbfa5a1 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -553,7 +553,8 @@ typedef struct ext2_icount *ext2_icount_t;
 					 EXT2_FEATURE_RO_COMPAT_LARGE_FILE|\
 					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK|\
 					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE|\
-					 EXT4_FEATURE_RO_COMPAT_GDT_CSUM)
+					 EXT4_FEATURE_RO_COMPAT_GDT_CSUM |\
+					 EXT4_FEATURE_RO_COMPAT_INODE_CSUM)
 
 /*
  * These features are only allowed if EXT2_FLAG_SOFTSUPP_FEATURES is passed
@@ -858,6 +859,12 @@ extern int ext2fs_super_and_bgd_loc(ext2_filsys fs,
 extern void ext2fs_update_dynamic_rev(ext2_filsys fs);
 
 /* csum.c */
+extern __u16 ext2fs_inode_csum(ext2_filsys fs, ext2_ino_t inum,
+			      struct ext2_inode_large *inode);
+extern void ext2fs_inode_csum_set(ext2_filsys fs, ext2_ino_t inum,
+				  struct ext2_inode_large *inode);
+extern int ext2fs_inode_csum_verify(ext2_filsys fs, ext2_ino_t inum,
+				    struct ext2_inode_large *inode);
 extern void ext2fs_group_desc_csum_set(ext2_filsys fs, dgrp_t group);
 extern int ext2fs_group_desc_csum_verify(ext2_filsys fs, dgrp_t group);
 extern errcode_t ext2fs_set_gdt_csum(ext2_filsys fs);
diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index a762dbc..5c377b5 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -608,6 +608,10 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
 			       (struct ext2_inode_large *) inode,
 			       0, bufsize);
 #endif
+	if (bufsize >= offsetof(struct ext2_inode_large, i_ctime_extra) &&
+	    !ext2fs_inode_csum_verify(fs, ino,
+			(struct ext2_inode_large *)inode))
+		return EXT2_ET_INODE_CSUM_INVALID;
 
 	/* Update the inode cache */
 	fs->icache->cache_last = (fs->icache->cache_last + 1) %
@@ -675,6 +679,19 @@ errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,
 		w_inode = &temp_inode;
 	memset(w_inode, 0, length);
 
+	if (fs->super->s_feature_ro_compat &
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM &&
+	    ((struct ext2_inode_large *)inode)->i_extra_isize > 4) {
+		if (bufsize < offsetof(struct ext2_inode_large,
+				       i_ctime_extra)) {
+			fprintf(stderr, "Underflow, inode %lu bufsize %u\n",
+				ino, bufsize);
+			abort();
+		}
+		ext2fs_inode_csum_set(fs, ino,
+			(struct ext2_inode_large *)inode);
+	}
+
 #ifdef WORDS_BIGENDIAN
 	ext2fs_swap_inode_full(fs, w_inode,
 			       (struct ext2_inode_large *) inode,
diff --git a/lib/ext2fs/swapfs.c b/lib/ext2fs/swapfs.c
index 3a43c6c..c601d8f 100644
--- a/lib/ext2fs/swapfs.c
+++ b/lib/ext2fs/swapfs.c
@@ -279,6 +279,8 @@ void ext2fs_swap_inode_full(ext2_filsys fs, struct ext2_inode_large *t,
 		return;
 	}
 
+	t->i_checksum = ext2fs_swab16(f->i_checksum);
+
 	i = sizeof(struct ext2_inode) + extra_isize + sizeof(__u32);
 	if (bufsize < (int) i)
 		return; /* no space for EA magic */
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index bcada11..957a19c 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -130,7 +130,8 @@ static __u32 ok_features[3] = {
 		EXT4_FEATURE_RO_COMPAT_DIR_NLINK|
 		EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE|
 		EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
-		EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER
+		EXT2_FEATURE_RO_COMPAT_SPARSE_SUPER |
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM
 };
 
 static __u32 clear_ok_features[3] = {
@@ -146,7 +147,8 @@ static __u32 clear_ok_features[3] = {
 		EXT4_FEATURE_RO_COMPAT_HUGE_FILE|
 		EXT4_FEATURE_RO_COMPAT_DIR_NLINK|
 		EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE|
-		EXT4_FEATURE_RO_COMPAT_GDT_CSUM
+		EXT4_FEATURE_RO_COMPAT_GDT_CSUM |
+		EXT4_FEATURE_RO_COMPAT_INODE_CSUM
 };
 
 /*
@@ -448,6 +450,18 @@ static void update_feature_set(ext2_filsys fs, char *features)
 	}
 
 	if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
+		       EXT4_FEATURE_RO_COMPAT_INODE_CSUM)) {
+		if (sb->s_inode_size < ext4_offsetof(struct ext2_inode_large,
+						     i_ctime_extra)) {
+			fputs(_("Inode checksums are only supported when "
+				"inodes are larger than 128 bytes.\n"),
+			      stderr);
+			exit(1);
+		}
+		request_fsck_afterwards(fs);
+	}
+
+	if (FEATURE_ON(E2P_FEATURE_RO_INCOMPAT,
 		       EXT4_FEATURE_RO_COMPAT_GDT_CSUM)) {
 		for (i = 0; i < fs->group_desc_count; i++) {
 			gd = ext2fs_group_desc(fs, fs->group_desc, i);

Re: [PATCH 2/2] e2fsprogs: Add support for toggling, verifying, and fixing inode checksums

[Andreas Dilger]

On 2011-04-06, at 4:47 PM, Darrick J. Wong wrote:
> This patch adds to tune2fs the ability to toggle the inode checksum rocompat
> feature flag, to e2fsck the ability to verify and correct inode checksums, and
> to debugfs the ability to dump inode checksums.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> @@ -729,6 +729,13 @@ void e2fsck_pass1(e2fsck_t ctx)
> +		/* Check for invalid inode checksum */
> +		if (!ext2fs_inode_csum_verify(fs, ino,
> +			(struct ext2_inode_large *)inode) &&
> +		    fix_problem(ctx, PR_1_INODE_CSUM_INVALID, &pctx))
> +			e2fsck_write_inode_full(ctx, ino, inode,
> +				sizeof(struct ext2_inode_large), "pass1");

If we just correct the checksum when it is found to be incorrect, then there is relatively little benefit in having it at all?  The default action in this case would likely be to declare the inode invalid and clears it, but there also needs to be a fallback option that declares the only checksum invalid and corrects it. 

Do you have an e2fsck testcase for this code, to show that it detects/fixes inodes with data corruption, and to fix the checksums after the ROCOMPAT flag is set the first time?

With the "ibadness" patch in our tree, the bad checksum should be a significant factor in marking the inode as garbage, but possibly not enough to have it thrown out if there are no other errors in the inode.

> @@ -890,6 +890,11 @@ static struct e2fsck_problem problem_table[] = {
> 	     "(size %Is, lblk %r)\n"),
> 	  PROMPT_CLEAR, PR_PREEN_OK },
> 
> +	/* Fast symlink has EXTENTS_FL set */
> +	{ PR_1_INODE_CSUM_INVALID,
> +	  N_("inode %i checksum invalid.  "),

The comment for each problem should exactly mirror the text that is printed.  In this case, you haven't used the abbreviations "@i" and "@n", which would normally make it much harder to search for this error string in the code, but also simplifies the translation of the message.


Cheers, Andreas

Re: [PATCH 2/2] e2fsprogs: Add support for toggling, verifying, and fixing inode checksums

[Darrick J. Wong]

On Fri, Apr 08, 2011 at 03:14:04AM -0600, Andreas Dilger wrote:
> On 2011-04-06, at 4:47 PM, Darrick J. Wong wrote:
> > This patch adds to tune2fs the ability to toggle the inode checksum rocompat
> > feature flag, to e2fsck the ability to verify and correct inode checksums, and
> > to debugfs the ability to dump inode checksums.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> > @@ -729,6 +729,13 @@ void e2fsck_pass1(e2fsck_t ctx)
> > +		/* Check for invalid inode checksum */
> > +		if (!ext2fs_inode_csum_verify(fs, ino,
> > +			(struct ext2_inode_large *)inode) &&
> > +		    fix_problem(ctx, PR_1_INODE_CSUM_INVALID, &pctx))
> > +			e2fsck_write_inode_full(ctx, ino, inode,
> > +				sizeof(struct ext2_inode_large), "pass1");
> 
> If we just correct the checksum when it is found to be incorrect, then there
> is relatively little benefit in having it at all?  The default action in this
> case would likely be to declare the inode invalid and clears it, but there
> also needs to be a fallback option that declares the only checksum invalid
> and corrects it. 
> 
> Do you have an e2fsck testcase for this code, to show that it detects/fixes
> inodes with data corruption, and to fix the checksums after the ROCOMPAT flag
> is set the first time?

Not yet; I suspected that some clarification of exactly that issue was needed.
It looks to me that in general the checksum will be zero for the "flag is
enabled but no checksum has yet been provided" case, and nonzero in the "inode
is corrupt" case.  So if e2fsck sees zero it'd first ask to correct the
checksum, and if it sees nonzero it'll first ask to clear the inode.  If the
user answers no to the first question, e2fsck can then propose the second
option.

> With the "ibadness" patch in our tree, the bad checksum should be a
> significant factor in marking the inode as garbage, but possibly not enough
> to have it thrown out if there are no other errors in the inode.

Or e2fsck could use that heuristic; which tree is the ibadness patch in?
Google shows a patch from 2008, but no recent discussion.

Something along the lines of: if the inode is not very bad, ask first to fix
the checksum and second to clear the inode; if the inode seems bad, ask first
to clear it and second to fix the checksum.

> > @@ -890,6 +890,11 @@ static struct e2fsck_problem problem_table[] = {
> > 	     "(size %Is, lblk %r)\n"),
> > 	  PROMPT_CLEAR, PR_PREEN_OK },
> > 
> > +	/* Fast symlink has EXTENTS_FL set */
> > +	{ PR_1_INODE_CSUM_INVALID,
> > +	  N_("inode %i checksum invalid.  "),
> 
> The comment for each problem should exactly mirror the text that is printed.
> In this case, you haven't used the abbreviations "@i" and "@n", which would
> normally make it much harder to search for this error string in the code, but
> also simplifies the translation of the message.

Oops, comment blooper that was a thinko on my part.  What would the @n be for?

--D

Re: [PATCH 2/2] e2fsprogs: Add support for toggling, verifying, and fixing inode checksums

[Andreas Dilger]

On 2011-04-08, at 1:25 PM, Darrick J. Wong wrote:
> On Fri, Apr 08, 2011 at 03:14:04AM -0600, Andreas Dilger wrote:
>> Do you have an e2fsck testcase for this code, to show that it detects/fixes
>> inodes with data corruption, and to fix the checksums after the ROCOMPAT flag
>> is set the first time?
> 
> Not yet; I suspected that some clarification of exactly that issue was needed.
> It looks to me that in general the checksum will be zero for the "flag is
> enabled but no checksum has yet been provided" case, and nonzero in the "inode
> is corrupt" case.  So if e2fsck sees zero it'd first ask to correct the
> checksum, and if it sees nonzero it'll first ask to clear the inode.  If the
> user answers no to the first question, e2fsck can then propose the second
> option.

Seems reasonable, though it is possible that the inode checksums can also become invalid due to changing the filesystem UUID.  This should probably be handled by tune2fs when the UUID is changed, with an extra prompt if INODE_CSUM is enabled to indicate that the conversion may take a long time.

Looking at the checksum algorithm you used, the inode checksum does not change if the inode is relocated due to resize (i.e. it uses the inode number and not the underlying block number).  This is convenient, and does not impact the correctness in any way - if the wrong block is read/written then the inode number used in the checksum will not match either.

>> With the "ibadness" patch in our tree, the bad checksum should be a
>> significant factor in marking the inode as garbage, but possibly not enough
>> to have it thrown out if there are no other errors in the inode.
> 
> Or e2fsck could use that heuristic; which tree is the ibadness patch in?
> Google shows a patch from 2008, but no recent discussion.

There is a relatively up-to-date version at
http://git.whamcloud.com/?p=tools/e2fsprogs.git;a=blob_plain;f=patches/e2fsprogs-ibadness-counter.patch;hb=8dd11ed9bdf0914d57d78d0c387bd21f747c1d29

> Something along the lines of: if the inode is not very bad, ask first to fix
> the checksum and second to clear the inode; if the inode seems bad, ask first
> to clear it and second to fix the checksum.

Yes, that is what I was thinking.  The real question is why the checksum would be bad in the case of no other "badness"?  If it is due to the UUID, that should be handled when the UUID is changed, and if it is due to a misplaced write (i.e. bad inode number) then it will help us to distinguish between the "real" inode and the misplaced "bad" inode.

>>> @@ -890,6 +890,11 @@ static struct e2fsck_problem problem_table[] = {
>>> 	     "(size %Is, lblk %r)\n"),
>>> 	  PROMPT_CLEAR, PR_PREEN_OK },
>>> 
>>> +	/* Fast symlink has EXTENTS_FL set */
>>> +	{ PR_1_INODE_CSUM_INVALID,
>>> +	  N_("inode %i checksum invalid.  "),
>> 
>> The comment for each problem should exactly mirror the text that is printed.
>> In this case, you haven't used the abbreviations "@i" and "@n", which would
>> normally make it much harder to search for this error string in the code, but
>> also simplifies the translation of the message.
> 
> Oops, comment blooper that was a thinko on my part.  What would the @n be for?

@i is "inode", @n is "invalid", per e2fsck/message.c.

Cheers, Andreas

Re: [PATCH 2/2] e2fsprogs: Add support for toggling, verifying, and fixing inode checksums

[Darrick J. Wong]

On Fri, Apr 08, 2011 at 05:13:13PM -0600, Andreas Dilger wrote:
> On 2011-04-08, at 1:25 PM, Darrick J. Wong wrote:
> > On Fri, Apr 08, 2011 at 03:14:04AM -0600, Andreas Dilger wrote:
> >> Do you have an e2fsck testcase for this code, to show that it detects/fixes
> >> inodes with data corruption, and to fix the checksums after the ROCOMPAT flag
> >> is set the first time?
> > 
> > Not yet; I suspected that some clarification of exactly that issue was needed.
> > It looks to me that in general the checksum will be zero for the "flag is
> > enabled but no checksum has yet been provided" case, and nonzero in the "inode
> > is corrupt" case.  So if e2fsck sees zero it'd first ask to correct the
> > checksum, and if it sees nonzero it'll first ask to clear the inode.  If the
> > user answers no to the first question, e2fsck can then propose the second
> > option.
> 
> Seems reasonable, though it is possible that the inode checksums can also
> become invalid due to changing the filesystem UUID.  This should probably be
> handled by tune2fs when the UUID is changed, with an extra prompt if
> INODE_CSUM is enabled to indicate that the conversion may take a long time.

Yes, sounds reasonable.  I guess we'd have to verify all the inode checksums
before changing the UUID, change the UUID, and then set new checksums.  If the
pre-verification step fails, demand e2fsck and don't write anything.

> Looking at the checksum algorithm you used, the inode checksum does not
> change if the inode is relocated due to resize (i.e. it uses the inode number
> and not the underlying block number).  This is convenient, and does not
> impact the correctness in any way - if the wrong block is read/written then
> the inode number used in the checksum will not match either.
> 
> >> With the "ibadness" patch in our tree, the bad checksum should be a
> >> significant factor in marking the inode as garbage, but possibly not enough
> >> to have it thrown out if there are no other errors in the inode.
> > 
> > Or e2fsck could use that heuristic; which tree is the ibadness patch in?
> > Google shows a patch from 2008, but no recent discussion.
> 
> There is a relatively up-to-date version at
> http://git.whamcloud.com/?p=tools/e2fsprogs.git;a=blob_plain;f=patches/e2fsprogs-ibadness-counter.patch;hb=8dd11ed9bdf0914d57d78d0c387bd21f747c1d29

Ok, I'll pull that into my tree.

> > Something along the lines of: if the inode is not very bad, ask first to fix
> > the checksum and second to clear the inode; if the inode seems bad, ask first
> > to clear it and second to fix the checksum.
> 
> Yes, that is what I was thinking.  The real question is why the checksum
> would be bad in the case of no other "badness"?  If it is due to the UUID,
> that should be handled when the UUID is changed, and if it is due to a
> misplaced write (i.e. bad inode number) then it will help us to distinguish
> between the "real" inode and the misplaced "bad" inode.

Agreed.  I discovered another problem is that there seem to be a number of
places where e2fsck loads only the first 128 bytes out of an inode, checks it,
and then writes out a "corrected" 128 byte inode.  Obviously e2fsck needs to be
changed to read in the full inode size (whatever that is) and write out the
same amount, though this will probably result in a lot of e2fsck code churn.

I added the ability for e2fsck to zero out the checksum if it finds a Linux
ext* fs and the checksum feature disabled.

Mingming was also wondering if we ought to save some rocompat bits and combine
all the current checksumming proposals (extent tree, bitmaps) under one
rocompat bit?  Sounds like a decent idea to me.

By the way, I've been uploading my notes about on-disk layout to the wiki:
https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout

(Not sure if it's 100% clueful, but there we go.)

--D

> 
> >>> @@ -890,6 +890,11 @@ static struct e2fsck_problem problem_table[] = {
> >>> 	     "(size %Is, lblk %r)\n"),
> >>> 	  PROMPT_CLEAR, PR_PREEN_OK },
> >>> 
> >>> +	/* Fast symlink has EXTENTS_FL set */
> >>> +	{ PR_1_INODE_CSUM_INVALID,
> >>> +	  N_("inode %i checksum invalid.  "),
> >> 
> >> The comment for each problem should exactly mirror the text that is printed.
> >> In this case, you haven't used the abbreviations "@i" and "@n", which would
> >> normally make it much harder to search for this error string in the code, but
> >> also simplifies the translation of the message.
> > 
> > Oops, comment blooper that was a thinko on my part.  What would the @n be for?
> 
> @i is "inode", @n is "invalid", per e2fsck/message.c.
> 
> 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

Re: [PATCH 0/2] Add inode checksum support to ext4

[Mingming Cao]

On Wed, 2011-04-06 at 15:44 -0700, Darrick J. Wong wrote:
> Hi all,
> 
> I spent last week analyzing a client's corrupted ext3 image to see if I could
> determine what had gone wrong and caused the filesystem to blow apart.  As best
> as I could tell, a data block got miswritten into a different sector ... which
> happened to be an indirect block.  Some time later the indirect block, which
> now pointed at one of the inode tables (among other things that shouldn't ever
> become file data) was loaded as part of a file write, which caused that inode
> table to be blown to smithereens.  Just for fun I tried reading from one of
> these busted-inode files and ... failed to encounter any errors.  Somehow, they
> didn't find it funny that ext3 would read block numbers from a table with the
> contents "ibm.com" with a straight face.  Fortunately there were backups. :)
> 
> The client at this point asked if ext4 would do a better job of sanity
> checking, which got me to wonder why ext4 checksums block groups but not
> inodes.  It's on Ted's todo list, but apparently nobody wrote any patch, so I
> did.  The following two patches are a first draft of adding inode checksum
> support to both the kernel driver and to the various e2fsprogs.
> 

We had some discussion about this week at SF (at the ext4 bof at the
linux colloboration summit). Beyond checksumming the inode itself, it
would be more useful to checksum the extent indexing blocks, as the ext3
corruption actually happen at the indirect block.  

The idea is to reduce the eh_max (the max # of extents stored in this
block) to save some space to store the checksums in the block, 

/*
 * Each block (leaves and indexes), even inode-stored has header.
 */
struct ext4_extent_header {
        __le16  eh_magic;       /* probably will support different
formats */
        __le16  eh_entries;     /* number of valid entries */
        __le16  eh_max;         /* capacity of store in entries */
        __le16  eh_depth;       /* has tree real underlying blocks? */
        __le32  eh_generation;  /* generation of the tree */
};
This would make us a RO feature to checksum the leaves and indexes
blocks too.

> If you have an existing ext* fs with 256-byte inodes, you ought to be able to
> "tune2fs -O inode_csum /dev/XXX", fsck /dev/XXX, and mount the filesystem with
> checksumming enabled.  It seems to work for me (i386/x86-64), but I'm looking
> for comments for improvement and perhaps some more testing (ppc64 is still
> building).  This inode checksum feature is not enabled by default.
> 
> --D
> --
> 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

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Fri, Apr 08, 2011 at 12:27:48PM -0700, Mingming Cao wrote:
> We had some discussion about this week at SF (at the ext4 bof at the
> linux colloboration summit). Beyond checksumming the inode itself, it
> would be more useful to checksum the extent indexing blocks, as the ext3
> corruption actually happen at the indirect block.  

	You want to checksum all metadata.  ocfs2 takes advantage of
this because we can just EIO any bad metadata and continue to serve the
rest of the filesystem.

Joel

-- 

"Time is an illusion, lunchtime doubly so."
        -Douglas Adams

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Darrick J. Wong]

On Fri, Apr 08, 2011 at 12:27:48PM -0700, Mingming Cao wrote:
> On Wed, 2011-04-06 at 15:44 -0700, Darrick J. Wong wrote:
> > Hi all,
> > 
> > I spent last week analyzing a client's corrupted ext3 image to see if I could
> > determine what had gone wrong and caused the filesystem to blow apart.  As best
> > as I could tell, a data block got miswritten into a different sector ... which
> > happened to be an indirect block.  Some time later the indirect block, which
> > now pointed at one of the inode tables (among other things that shouldn't ever
> > become file data) was loaded as part of a file write, which caused that inode
> > table to be blown to smithereens.  Just for fun I tried reading from one of
> > these busted-inode files and ... failed to encounter any errors.  Somehow, they
> > didn't find it funny that ext3 would read block numbers from a table with the
> > contents "ibm.com" with a straight face.  Fortunately there were backups. :)
> > 
> > The client at this point asked if ext4 would do a better job of sanity
> > checking, which got me to wonder why ext4 checksums block groups but not
> > inodes.  It's on Ted's todo list, but apparently nobody wrote any patch, so I
> > did.  The following two patches are a first draft of adding inode checksum
> > support to both the kernel driver and to the various e2fsprogs.
> > 
> 
> We had some discussion about this week at SF (at the ext4 bof at the
> linux colloboration summit). Beyond checksumming the inode itself, it
> would be more useful to checksum the extent indexing blocks, as the ext3
> corruption actually happen at the indirect block.  
> 
> The idea is to reduce the eh_max (the max # of extents stored in this
> block) to save some space to store the checksums in the block, 
> 
> /*
>  * Each block (leaves and indexes), even inode-stored has header.
>  */
> struct ext4_extent_header {
>         __le16  eh_magic;       /* probably will support different
> formats */
>         __le16  eh_entries;     /* number of valid entries */
>         __le16  eh_max;         /* capacity of store in entries */
>         __le16  eh_depth;       /* has tree real underlying blocks? */
>         __le32  eh_generation;  /* generation of the tree */

Does anyone use eh_generation?  Linux 3.0 shows no users and it didn't look like
the snapshot patches do either.  If nobody intends to start using this field,
(part of) it could become eh_checksum.

--D

Re: [PATCH 0/2] Add inode checksum support to ext4

[Andreas Dilger]

On 2011-07-27, at 2:27 AM, "Darrick J. Wong" <djwong@us.ibm.com> wrote:
> On Fri, Apr 08, 2011 at 12:27:48PM -0700, Mingming Cao wrote:
>> Beyond checksumming the inode itself, it
>> would be more useful to checksum the extent indexing blocks, as the ext3
>> corruption actually happen at the indirect block.  
>> 
>> The idea is to reduce the eh_max (the max # of extents stored in this
>> block) to save some space to store the checksums in the block, 
>> 
>> /*
>> * Each block (leaves and indexes), even inode-stored has header.
>> */
>> struct ext4_extent_header {
>>        __le16  eh_magic;       /* probably will support different
>> formats */
>>        __le16  eh_entries;     /* number of valid entries */
>>        __le16  eh_max;         /* capacity of store in entries */
>>        __le16  eh_depth;       /* has tree real underlying blocks? */
>>        __le32  eh_generation;  /* generation of the tree */
> 
> Does anyone use eh_generation?  Linux 3.0 shows no users and it didn't look like
> the snapshot patches do either.  If nobody intends to start using this field,
> (part of) it could become eh_checksum

We use eh_generation in Lustre to detect if the extent tree is being changed while it is unlocked.

In the past, the discussion about adding checksums to the index and extent blocks was about using an ext4_extent_tail that contained not only the checksum of the block, but also a back-pointer to the inode/generation of the inode using this block.

That would allow e2fsck to verify that it is using the correct index/extent blocks and not pointing to a stale block that belonged to some other inode.

Since the header and index/extent entries are always 3 *__u32 in size, the extent tail can always be 4 * __u32 in size yet only consume a single slot in the block. There of course is no reason to put an extent tail inside the inode itself.

Cheers, Andreas

Re: [PATCH 0/2] Add inode checksum support to ext4

[Darrick J. Wong]

On Wed, Jul 27, 2011 at 03:16:12AM -0600, Andreas Dilger wrote:
> On 2011-07-27, at 2:27 AM, "Darrick J. Wong" <djwong@us.ibm.com> wrote:
> > On Fri, Apr 08, 2011 at 12:27:48PM -0700, Mingming Cao wrote:
> >> Beyond checksumming the inode itself, it
> >> would be more useful to checksum the extent indexing blocks, as the ext3
> >> corruption actually happen at the indirect block.  
> >> 
> >> The idea is to reduce the eh_max (the max # of extents stored in this
> >> block) to save some space to store the checksums in the block, 
> >> 
> >> /*
> >> * Each block (leaves and indexes), even inode-stored has header.
> >> */
> >> struct ext4_extent_header {
> >>        __le16  eh_magic;       /* probably will support different
> >> formats */
> >>        __le16  eh_entries;     /* number of valid entries */
> >>        __le16  eh_max;         /* capacity of store in entries */
> >>        __le16  eh_depth;       /* has tree real underlying blocks? */
> >>        __le32  eh_generation;  /* generation of the tree */
> > 
> > Does anyone use eh_generation?  Linux 3.0 shows no users and it didn't look like
> > the snapshot patches do either.  If nobody intends to start using this field,
> > (part of) it could become eh_checksum
> 
> We use eh_generation in Lustre to detect if the extent tree is being changed
> while it is unlocked.
> 
> In the past, the discussion about adding checksums to the index and extent
> blocks was about using an ext4_extent_tail that contained not only the
> checksum of the block, but also a back-pointer to the inode/generation of the
> inode using this block.
> 
> That would allow e2fsck to verify that it is using the correct index/extent
> blocks and not pointing to a stale block that belonged to some other inode.
> 
> Since the header and index/extent entries are always 3 *__u32 in size, the
> extent tail can always be 4 * __u32 in size yet only consume a single slot in
> the block. There of course is no reason to put an extent tail inside the
> inode itself.

Does anybody have any objection to using crc32c (which we can hardware
accelerate on new Intel boxen) over crc16?  I think it'll be pretty easy to use
some of the reserved space in the group descriptor to store checksums of the
block and inode bitmaps.  Adding tails to the extent tree blocks seems a bit
trickier than that, but not a big deal, though I guess I'll have to reshuffle
the extent tree to free up space at the end of the block.

I was also wondering what people think of adding checksums to directory files?
I think that it's possible to put a checksum in each directory block -- for
blocks containing a linear array of actual directory entries, we could zero out
the space past the end of the array and put a checksum at the very end of the
block.  For the dx_node/dx_root blocks, we could probably use the space
occupied by the last dx_entry to store the checksum.  Obviously, we'd have to
move whatever's at the end of the block elsewhere, but then, we have to do that
for the extent tree too.  Basically, the last 4 bytes become the checksum after
whatever's occupying the space is relocated. :)

It looks like there's sufficient unused space in ext4_xattr_header to add a
checksum.

Also -- should I create separate rocompat feature flags for each metadata
object that I add checksums to?  Or just have one flag that covers them all?

Ok, enough crazy ideas for now...

--D

Re: [PATCH 0/2] Add inode checksum support to ext4

[Darrick J. Wong]

On Thu, Jul 28, 2011 at 08:07:17PM +0300, Amir Goldstein wrote:
> On Jul 28, 2011 7:56 PM, "Darrick J. Wong" <djwong@us.ibm.com> wrote:
> >
> > On Wed, Jul 27, 2011 at 03:16:12AM -0600, Andreas Dilger wrote:
> > > On 2011-07-27, at 2:27 AM, "Darrick J. Wong" <djwong@us.ibm.com> wrote:
> > > > On Fri, Apr 08, 2011 at 12:27:48PM -0700, Mingming Cao wrote:
> > > >> Beyond checksumming the inode itself, it
> > > >> would be more useful to checksum the extent indexing blocks, as the
> ext3
> > > >> corruption actually happen at the indirect block.
> > > >>
> > > >> The idea is to reduce the eh_max (the max # of extents stored in this
> > > >> block) to save some space to store the checksums in the block,
> > > >>
> > > >> /*
> > > >> * Each block (leaves and indexes), even inode-stored has header.
> > > >> */
> > > >> struct ext4_extent_header {
> > > >>        __le16  eh_magic;       /* probably will support different
> > > >> formats */
> > > >>        __le16  eh_entries;     /* number of valid entries */
> > > >>        __le16  eh_max;         /* capacity of store in entries */
> > > >>        __le16  eh_depth;       /* has tree real underlying blocks? */
> > > >>        __le32  eh_generation;  /* generation of the tree */
> > > >
> > > > Does anyone use eh_generation?  Linux 3.0 shows no users and it didn't
> look like
> > > > the snapshot patches do either.  If nobody intends to start using this
> field,
> > > > (part of) it could become eh_checksum
> > >
> > > We use eh_generation in Lustre to detect if the extent tree is being
> changed
> > > while it is unlocked.
> > >
> > > In the past, the discussion about adding checksums to the index and
> extent
> > > blocks was about using an ext4_extent_tail that contained not only the
> > > checksum of the block, but also a back-pointer to the inode/generation
> of the
> > > inode using this block.
> > >
> > > That would allow e2fsck to verify that it is using the correct
> index/extent
> > > blocks and not pointing to a stale block that belonged to some other
> inode.
> > >
> > > Since the header and index/extent entries are always 3 *__u32 in size,
> the
> > > extent tail can always be 4 * __u32 in size yet only consume a single
> slot in
> > > the block. There of course is no reason to put an extent tail inside the
> > > inode itself.
> >
> > Does anybody have any objection to using crc32c (which we can hardware
> > accelerate on new Intel boxen) over crc16?  I think it'll be pretty easy
> to use
> > some of the reserved space in the group descriptor to store checksums of
> the
> > block and inode bitmaps.
> 
> On LSF Ted told me i can use 32bit from the group descriptor for exclude
> bitmap block and that inode and block bitmap checksum would use 16bit each .

I know;  as far as I know there's still 96 bits of free space in the group
descriptor, which could be used to crc32 all three bitmaps.  But then that
leaves the descriptors with no room for further expansion, unless we decide to
expand them sort of like what was done for inodes.  Do we have a strategy for
handling continued expansion of metadata objects in ext4?

Besides "don't" or "use btrfs"? :)

--D

> 
>  Adding tails to the extent tree blocks seems a bit
> > trickier than that, but not a big deal, though I guess I'll have to
> reshuffle
> > the extent tree to free up space at the end of the block.
> >
> > I was also wondering what people think of adding checksums to directory
> files?
> > I think that it's possible to put a checksum in each directory block --
> for
> > blocks containing a linear array of actual directory entries, we could
> zero out
> > the space past the end of the array and put a checksum at the very end of
> the
> > block.  For the dx_node/dx_root blocks, we could probably use the space
> > occupied by the last dx_entry to store the checksum.  Obviously, we'd have
> to
> > move whatever's at the end of the block elsewhere, but then, we have to do
> that
> > for the extent tree too.  Basically, the last 4 bytes become the checksum
> after
> > whatever's occupying the space is relocated. :)
> >
> > It looks like there's sufficient unused space in ext4_xattr_header to add
> a
> > checksum.
> >
> > Also -- should I create separate rocompat feature flags for each metadata
> > object that I add checksums to?  Or just have one flag that covers them
> all?
> >
> > Ok, enough crazy ideas for now...
> >
> > --D
> > --
> > 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

Re: [PATCH 0/2] Add inode checksum support to ext4

[Andreas Dilger]

On 2011-07-28, at 12:57 PM, Darrick J. Wong wrote:
>>> On Wed, Jul 27, 2011 at 03:16:12AM -0600, Andreas Dilger wrote:
>>>> In the past, the discussion about adding checksums to the index and
>>>> extent blocks was about using an ext4_extent_tail that contained not
>>>> only the checksum of the block, but also a back-pointer to the
>>>> inode/generation of the inode using this block.
>>>> 
>>>> That would allow e2fsck to verify that it is using the correct
>>>> index/extent blocks and not pointing to a stale block that belonged
>>>> to some other inode.
>>>> 
>>>> Since the header and index/extent entries are always 3 *__u32 in size,
>>>> the extent tail can always be 4 * __u32 in size yet only consume a
>>>> single slot in the block. There of course is no reason to put an extent
>>>> tail inside the inode itself.
>>> 
>>> Does anybody have any objection to using crc32c (which we can hardware
>>> accelerate on new Intel boxen) over crc16?  I think it'll be pretty easy
>>> to use some of the reserved space in the group descriptor to store
>>> checksums of the block and inode bitmaps.
>> 
>> On LSF Ted told me i can use 32bit from the group descriptor for exclude
>> bitmap block and that inode and block bitmap checksum would use 16bit each .
> 
> I know;  as far as I know there's still 96 bits of free space in the group
> descriptor, which could be used to crc32 all three bitmaps.  But then that
> leaves the descriptors with no room for further expansion, unless we decide to
> expand them sort of like what was done for inodes.  Do we have a strategy for
> handling continued expansion of metadata objects in ext4?

One reason to stick to crc16 for the block checksums is that this would still
fit inside the ext3_group_desc space, so upgraded filesystems could enable
checksums.

>>> Adding tails to the extent tree blocks seems a bit trickier than that,
>>> but not a big deal, though I guess I'll have to reshuffle
>>> the extent tree to free up space at the end of the block.

I don't think reshuffling is necessary.  It should be OK to add it to blocks
that fit, and skip blocks that don't have enough space.

>>> I was also wondering what people think of adding checksums to directory
>> files?
>>> I think that it's possible to put a checksum in each directory block --
>> for
>>> blocks containing a linear array of actual directory entries, we could
>> zero out
>>> the space past the end of the array and put a checksum at the very end of
>> the
>>> block.  For the dx_node/dx_root blocks, we could probably use the space
>>> occupied by the last dx_entry to store the checksum.  Obviously, we'd have
>> to
>>> move whatever's at the end of the block elsewhere, but then, we have to do
>> that
>>> for the extent tree too.  Basically, the last 4 bytes become the checksum
>> after
>>> whatever's occupying the space is relocated. :)
>>> 
>>> It looks like there's sufficient unused space in ext4_xattr_header to add
>> a
>>> checksum.
>>> 
>>> Also -- should I create separate rocompat feature flags for each metadata
>>> object that I add checksums to?  Or just have one flag that covers them
>> all?
>>> 
>>> Ok, enough crazy ideas for now...
>>> 
>>> --D
>>> --
>>> 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

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Thu, Jul 28, 2011 at 09:56:15AM -0700, Darrick J. Wong wrote:
> > the block. There of course is no reason to put an extent tail inside the
> > inode itself.
> 
> Does anybody have any objection to using crc32c (which we can hardware
> accelerate on new Intel boxen) over crc16?  I think it'll be pretty easy to use

	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
could have used crc32c if I'd really thought about the hardware
acceleration benefits.  I think it's a good idea for ext4.

> some of the reserved space in the group descriptor to store checksums of the
> block and inode bitmaps.  Adding tails to the extent tree blocks seems a bit
> trickier than that, but not a big deal, though I guess I'll have to reshuffle
> the extent tree to free up space at the end of the block.
> 
> I was also wondering what people think of adding checksums to directory files?
> I think that it's possible to put a checksum in each directory block -- for
> blocks containing a linear array of actual directory entries, we could zero out
> the space past the end of the array and put a checksum at the very end of the
> block.  For the dx_node/dx_root blocks, we could probably use the space
> occupied by the last dx_entry to store the checksum.  Obviously, we'd have to
> move whatever's at the end of the block elsewhere, but then, we have to do that
> for the extent tree too.  Basically, the last 4 bytes become the checksum after
> whatever's occupying the space is relocated. :)

	ocfs2 adds trailer entries to every dirblock for the checksum.
We also do our dirindex free list there.  Since ocfs2 dirblocks are ext3
dirblocks, I bet you can rip off a lot of that code, including the
feature compatibility stuff.  See ocfs2_fs.h.

> It looks like there's sufficient unused space in ext4_xattr_header to add a
> checksum.
> 
> Also -- should I create separate rocompat feature flags for each metadata
> object that I add checksums to?  Or just have one flag that covers them all?

	I really think you should checksum every metadata block.  A few
things will take some effort to shoehorn it in, but it is worth it.

Joel

-- 

Life's Little Instruction Book #173

	"Be kinder than necessary."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Andreas Dilger]

On 2011-07-28, at 4:07 PM, Joel Becker wrote:
> On Thu, Jul 28, 2011 at 09:56:15AM -0700, Darrick J. Wong wrote:
>>> the block. There of course is no reason to put an extent tail inside the
>>> inode itself.
>> 
>> Does anybody have any objection to using crc32c (which we can hardware
>> accelerate on new Intel boxen) over crc16?  I think it'll be pretty easy to use
> 
> 	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
> could have used crc32c if I'd really thought about the hardware
> acceleration benefits.  I think it's a good idea for ext4.

The problem with crc32[c] is that if you don't have hardware acceleration
it is terribly slow.

>> some of the reserved space in the group descriptor to store checksums of the
>> block and inode bitmaps.  Adding tails to the extent tree blocks seems a bit
>> trickier than that, but not a big deal, though I guess I'll have to reshuffle
>> the extent tree to free up space at the end of the block.
>> 
>> I was also wondering what people think of adding checksums to directory files?
>> I think that it's possible to put a checksum in each directory block -- for
>> blocks containing a linear array of actual directory entries, we could zero out
>> the space past the end of the array and put a checksum at the very end of the
>> block.  For the dx_node/dx_root blocks, we could probably use the space
>> occupied by the last dx_entry to store the checksum.  Obviously, we'd have to
>> move whatever's at the end of the block elsewhere, but then, we have to do that
>> for the extent tree too.  Basically, the last 4 bytes become the checksum after
>> whatever's occupying the space is relocated. :)
> 
> 	ocfs2 adds trailer entries to every dirblock for the checksum.
> We also do our dirindex free list there.  Since ocfs2 dirblocks are ext3
> dirblocks, I bet you can rip off a lot of that code, including the
> feature compatibility stuff.  See ocfs2_fs.h.

Yes, it makes sense to just put a "fake" dirent at the end of the leaf block,
or similar.  I don't think it is necessary to modify existing directories or
extent blocks to add these structures in, if there is no room, but for new
blocks, or blocks with space it is enough.

>> It looks like there's sufficient unused space in ext4_xattr_header to add a
>> checksum.
>> 
>> Also -- should I create separate rocompat feature flags for each metadata
>> object that I add checksums to?  Or just have one flag that covers them all?
> 
> 	I really think you should checksum every metadata block.  A few
> things will take some effort to shoehorn it in, but it is worth it.
> 
> Joel
> 
> -- 
> 
> Life's Little Instruction Book #173
> 
> 	"Be kinder than necessary."
> 
> 			http://www.jlbec.org/
> 			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Fri, Jul 29, 2011 at 03:48:45AM -0600, Andreas Dilger wrote:
> On 2011-07-28, at 4:07 PM, Joel Becker wrote:
> > 	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
> > could have used crc32c if I'd really thought about the hardware
> > acceleration benefits.  I think it's a good idea for ext4.
> 
> The problem with crc32[c] is that if you don't have hardware acceleration
> it is terribly slow.

	We find ethernet crc32 just fine in ocfs2.  I use the kernel's
implementation, which survives everyone's network traffic, and of course
we added the triggers to jbd2 so we only have to do the calculations on
read and write.

> Yes, it makes sense to just put a "fake" dirent at the end of the leaf block,
> or similar.  I don't think it is necessary to modify existing directories or
> extent blocks to add these structures in, if there is no room, but for new
> blocks, or blocks with space it is enough.

	We have tunefs.ocfs2 code to adjust existing directories to add
the trailer.  It's not too bad, really.
 
Joel

-- 

"People with narrow minds usually have broad tongues."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Coly Li]

On 2011年07月29日 21:19, Joel Becker Wrote:
> On Fri, Jul 29, 2011 at 03:48:45AM -0600, Andreas Dilger wrote:
>> On 2011-07-28, at 4:07 PM, Joel Becker wrote:
>>> 	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
>>> could have used crc32c if I'd really thought about the hardware
>>> acceleration benefits.  I think it's a good idea for ext4.
>>
>> The problem with crc32[c] is that if you don't have hardware acceleration
>> it is terribly slow.
> 
> 	We find ethernet crc32 just fine in ocfs2.  I use the kernel's
> implementation, which survives everyone's network traffic, and of course
> we added the triggers to jbd2 so we only have to do the calculations on
> read and write.
> 

Ext4 supports non-journal mode, and there are a few users (Google, Taobao, etc.).
A trigger of jbd2 may not work well for non-journal Ext4 ...

And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
more careful in check summing, e.g. inode/block bitmap blocks...


>> Yes, it makes sense to just put a "fake" dirent at the end of the leaf block,
>> or similar.  I don't think it is necessary to modify existing directories or
>> extent blocks to add these structures in, if there is no room, but for new
>> blocks, or blocks with space it is enough.
> 
> 	We have tunefs.ocfs2 code to adjust existing directories to add
> the trailer.  It's not too bad, really.

Agree, enable/disable the trailer isn't that difficult. We need more eyes to take care the user space tools.

Coly

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Sat, Jul 30, 2011 at 03:25:32PM +0800, Coly Li wrote:
> On 2011年07月29日 21:19, Joel Becker Wrote:
> > On Fri, Jul 29, 2011 at 03:48:45AM -0600, Andreas Dilger wrote:
> >> On 2011-07-28, at 4:07 PM, Joel Becker wrote:
> >>> 	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
> >>> could have used crc32c if I'd really thought about the hardware
> >>> acceleration benefits.  I think it's a good idea for ext4.
> >>
> >> The problem with crc32[c] is that if you don't have hardware acceleration
> >> it is terribly slow.
> > 
> > 	We find ethernet crc32 just fine in ocfs2.  I use the kernel's
> > implementation, which survives everyone's network traffic, and of course
> > we added the triggers to jbd2 so we only have to do the calculations on
> > read and write.
> > 
> 
> Ext4 supports non-journal mode, and there are a few users (Google, Taobao, etc.).
> A trigger of jbd2 may not work well for non-journal Ext4 ...
> 
> And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
> more careful in check summing, e.g. inode/block bitmap blocks...

	Sure, but you could use a trigger in journaled mode and then do
the checksums directly in the __ext4_handle_journal_dirty_*() functions
in non-journaled mode.  Sure, it would be a little more CPU time, but
the user picked "checksums + no journal" at mkfs time.

Joel

-- 

 Joel's Second Law:

	If a code change requires additional user setup, it is wrong.

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Coly Li]

On 2011年07月31日 15:08, Joel Becker Wrote:
> On Sat, Jul 30, 2011 at 03:25:32PM +0800, Coly Li wrote:
>> On 2011年07月29日 21:19, Joel Becker Wrote:
>>> On Fri, Jul 29, 2011 at 03:48:45AM -0600, Andreas Dilger wrote:
>>>> On 2011-07-28, at 4:07 PM, Joel Becker wrote:
>>>>> 	We use ethernet crc32 in ocfs2.  btrfs uses crc32c.  Frankly, I
>>>>> could have used crc32c if I'd really thought about the hardware
>>>>> acceleration benefits.  I think it's a good idea for ext4.
>>>>
>>>> The problem with crc32[c] is that if you don't have hardware acceleration
>>>> it is terribly slow.
>>>
>>> 	We find ethernet crc32 just fine in ocfs2.  I use the kernel's
>>> implementation, which survives everyone's network traffic, and of course
>>> we added the triggers to jbd2 so we only have to do the calculations on
>>> read and write.
>>>
>>
>> Ext4 supports non-journal mode, and there are a few users (Google, Taobao, etc.).
>> A trigger of jbd2 may not work well for non-journal Ext4 ...
>>
>> And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
>> more careful in check summing, e.g. inode/block bitmap blocks...
> 
> 	Sure, but you could use a trigger in journaled mode and then do
> the checksums directly in the __ext4_handle_journal_dirty_*() functions
> in non-journaled mode.  Sure, it would be a little more CPU time, but
> the user picked "checksums + no journal" at mkfs time.
> 

Yes, my idea was similar to you.
One thing not clear to me is, in non-journal mode, how to make the page of bitmap block being stable. Because bits
setting in Ext4 bitmap is non-locking, it might be possible that new bit setting after check sum is calculated.

Coly

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Mon, Aug 01, 2011 at 07:52:41AM +0800, Coly Li wrote:
> On 2011年07月31日 15:08, Joel Becker Wrote:
> > On Sat, Jul 30, 2011 at 03:25:32PM +0800, Coly Li wrote:
> >> And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
> >> more careful in check summing, e.g. inode/block bitmap blocks...
> > 
> > 	Sure, but you could use a trigger in journaled mode and then do
> > the checksums directly in the __ext4_handle_journal_dirty_*() functions
> > in non-journaled mode.  Sure, it would be a little more CPU time, but
> > the user picked "checksums + no journal" at mkfs time.
> > 
> 
> Yes, my idea was similar to you.
> One thing not clear to me is, in non-journal mode, how to make the page of bitmap block being stable. Because bits
> setting in Ext4 bitmap is non-locking, it might be possible that new bit setting after check sum is calculated.

	Every place that changes the bits will eventually call
ext4_journal_dirty(), which recalculates the checksum.  So there's no
danger of a set-bit-after-last-checksum.  But you will have to lock
around the checksum calculation in non-journaling mode.  JBD2 handles it
for journaling mode.

Joel

-- 

"The whole principle is wrong; it's like demanding that grown men 
 live on skim milk because the baby can't eat steak."
        - author Robert A. Heinlein on censorship

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Joel Becker]

On Sun, Jul 31, 2011 at 09:57:11PM -0700, Joel Becker wrote:
> On Mon, Aug 01, 2011 at 07:52:41AM +0800, Coly Li wrote:
> > On 2011年07月31日 15:08, Joel Becker Wrote:
> > > On Sat, Jul 30, 2011 at 03:25:32PM +0800, Coly Li wrote:
> > >> And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
> > >> more careful in check summing, e.g. inode/block bitmap blocks...
> > > 
> > > 	Sure, but you could use a trigger in journaled mode and then do
> > > the checksums directly in the __ext4_handle_journal_dirty_*() functions
> > > in non-journaled mode.  Sure, it would be a little more CPU time, but
> > > the user picked "checksums + no journal" at mkfs time.
> > > 
> > 
> > Yes, my idea was similar to you.
> > One thing not clear to me is, in non-journal mode, how to make the page of bitmap block being stable. Because bits
> > setting in Ext4 bitmap is non-locking, it might be possible that new bit setting after check sum is calculated.
> 
> 	Every place that changes the bits will eventually call
> ext4_journal_dirty(), which recalculates the checksum.  So there's no
> danger of a set-bit-after-last-checksum.  But you will have to lock
> around the checksum calculation in non-journaling mode.  JBD2 handles it
> for journaling mode.

	Wait, bitsetting in ext4 can't be non-locking.  Or are they
crazily stomping on memory?  I sure see an assert_spin_locked() in
mb_mark_used().

Joel

-- 

"Well-timed silence hath more eloquence than speech."  
         - Martin Fraquhar Tupper

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [PATCH 0/2] Add inode checksum support to ext4

[Coly Li]

On 2011年08月01日 13:04, Joel Becker Wrote:
> On Sun, Jul 31, 2011 at 09:57:11PM -0700, Joel Becker wrote:
>> On Mon, Aug 01, 2011 at 07:52:41AM +0800, Coly Li wrote:
>>> On 2011年07月31日 15:08, Joel Becker Wrote:
>>>> On Sat, Jul 30, 2011 at 03:25:32PM +0800, Coly Li wrote:
>>>>> And in non-journal mode, there is not copy of any meta data block in jbd2, we need to be
>>>>> more careful in check summing, e.g. inode/block bitmap blocks...
>>>>
>>>> 	Sure, but you could use a trigger in journaled mode and then do
>>>> the checksums directly in the __ext4_handle_journal_dirty_*() functions
>>>> in non-journaled mode.  Sure, it would be a little more CPU time, but
>>>> the user picked "checksums + no journal" at mkfs time.
>>>>
>>>
>>> Yes, my idea was similar to you.
>>> One thing not clear to me is, in non-journal mode, how to make the page of bitmap block being stable. Because bits
>>> setting in Ext4 bitmap is non-locking, it might be possible that new bit setting after check sum is calculated.
>>
>> 	Every place that changes the bits will eventually call
>> ext4_journal_dirty(), which recalculates the checksum.  So there's no
>> danger of a set-bit-after-last-checksum.  But you will have to lock
>> around the checksum calculation in non-journaling mode.  JBD2 handles it
>> for journaling mode.
> 
> 	Wait, bitsetting in ext4 can't be non-locking.  Or are they
> crazily stomping on memory?  I sure see an assert_spin_locked() in
> mb_mark_used().
> 

Yes, you are right.
What I worried was inode bitmap, because last time (before uninit_bg was developed), inode bitmap was set by
ext4_set_bit_atomic(). Now I see inode bitmap is set by ext4_claim_inode() which locks the group internally.
And I confirm block bitmap is protected by ext4_{lock,unlock}_group(). So there is no risk for
set-bit-after-last-checksum.

Thanks for the clarification.

-- 
Coly Li

Re: [PATCH 0/2] Add inode checksum support to ext4

[Andi Kleen]

"Darrick J. Wong" <djwong@us.ibm.com> writes:

FWIW I had a similar idea quite some time ago and implemented checksums for
superblocks and inodes. I never posted it because I didn't get around
to write the e2fsprogs support and do a lot of performance testing. 
There was one result that showed a slow down in quick tests, but 
I suspect it was a fluke and probably needs to be redone. In other
tests it was neutral.

Anyways here's the old patch if anyone is interested (against a ~.35ish kernel)
I can forward port it if there's interest.

IMHO it's a good idea but it should be done for the super blocks too
(and ideally for all objects, although unfortunately that breaks 
the disk format)

The locking for stable buffers is still something that needs to be
double checked.

-Andi

commit 8ece10cfa4b148075dbb93ca65855a7e2aad7b07
Author: Andi Kleen <ak@linux.intel.com>
Date:   Mon May 31 18:27:29 2010 +0200

    EXT4: Add checksums to the super block and the inodes
    
    Currently when a on-disk structure is corrupted ext4 usually
    detects it only by some internal inconsistency, but this can happen
    too late or give confusing error messages.
    
    One way to detect corruptions sooner is to use checksums. This
    means the file system can stop using a corrupted object ASAP, not
    follow any potentially corrupted pointers to other blocks,
    and also give a clear message on what happened.
    
    Potentially it can also be used to limit read only mounts and
    forced fscks. When the extent of a corruption can be detected
    accurately using checksums and only force errors on that
    particular object (this is right now not enabled though
    because there are still too many objects with missing checksums)
    
    There's a general trend in file systems recently to add metadata
    checksums, this just makes ext4 catch up a bit (in addition
    to the already existing transaction and block group checksums)
    
    Another advantage is that the checksum can also check for misplaced
    writes (by including the intended block location) and objects
    left over from before mkfs (by including the file system UUID).
    
    Adding checksums to all metadata in ext4 would likely need some
    incompatible format changes, but it's relatively straight-forward to add
    them to inodes and the super block at least. This patch-kit does this.
    
    Again it only handles some meta-data objects. This is not a full
    user-data checksumming feature or not even a "all metadata objects"
    feature.
    
    I didn't do a lot of research into different CRC functions, but simply
    used the same one as btrfs and iSCSI (crc32c).  ocfs2 uses ECCs
    that have some self-correcting ability, but that seemed overkill to me.
    The standard CRC has the advantage that the CRC is accelerated by hardware
    instructions on newer Intel CPUs and possibly on other platforms too.
    The CRC32C function is also hard coded, although in theory it could be made
    configurable per file system. But since that would lead to a multitude of
    incompatible formats I decided to simply hard code for now.
    
    Right now there is no e2fsprogs support, so fsck doesn't know about CRCs.
    
    For now the checksums can be enabled with a simple shell script
    (which is just a wrapper around debugfs):
    
    wget ftp://firstfloor.org/pub/ak/shell/e2checksum
    chmod +x e2checksum
    
    To enable:
    
    ./e2checksum /dev/device enable
    
    To disable:
    
    ./e2checksum /dev/device disable
    
    This works with the current ext4 e2fsprogs without any changes.
    
    The kernel will automatically add checksums to the file system when the
    feature is turned on, as the inodes are rewritten.
    
    WARNING: Note there is no fsck support currently, so before you run fsck better
    disable the checksums. After disabling/changing/reenabling
    you may also need to mount with ignore_crc until the checksums
    are fixed up again.
    
    The checksums are implemented as a read-only compat feature: this means
    the a file system with checksums enabled can be read by a kernel that
    does not know about checksums, but not written. Of course you can turn
    off the flag again to make the file system write compatible again (but
    that will put the checksums into a inconsistent state) or use the
    ignore_crc mount option.
    
    WARNING: the in-kernel upgrader relies on the checksum fields being
    zero when checksums are enabled. When you turn on the feature, use the
    file system, turn it off again, use it again and turn it on again the
    checksums will be in a inconsistent state. Right now this can be
    fixed by mounting with -o ignore_crc and then doing a find | xargs touch
    on the file system.
    
    I expect real e2fsprogs support can do that better.
    
    The code also adds some more sanity checks on inodes to distingush zeroed
    inodes from inodes with no checksums. This is currently done by enforcing
    that a/m/ctime are not zero. If there are broken file systems around
    where that is not true, the sanity check can be turned off (see below)
    
    There are two new mount options:
    
    ignore_crc	       Ignore the CRCs on reading but still update them
    noinode_sanity	       Don't do new inode sanity checks
    
    The implementation is not particularly optimized: it always recomputes
    the full CRC on each inode or super block write. Some more optimizations
    would be possible in this area.
    
    BENCHMARKS
    
    I ran kernelbench and it didn't show any significant difference between
    the run with checksums and the run without.
    
    I also tried timing fsstress from XFS/LTP, and it gave a 35% slowdown
    on a disk and 5% slow down on the ramdisk for the system time
    during the test.  However I'm a bit suspicious of these results.
    This was on a core 2.
    
    I also tested on a Nehalem system which has CRC acceleration instructions
    in the CPU.
    
    Anyone has a better inode metadata intensive benchmark to run?
    
    This gobbled up the last mount flag bits, but there are still some unused
    holes in the middle.
    
    The patch is definitely still experimental and needs more testing and
    review and e2fsprogs support. In particular I would appreciate review
    of my bh locking scheme.
    
    Also ext4_abort() now takes the super lock, which implies that the callers
    shouldn't.  I think I checked all callers that it's safe, but double checking
    that would be good.
    
    Opens:
    - e2fsprogs support. In this case the zero crc hack could be removed from
    the super block code at least.
    - Right now checksum numbers by default lead to a read-only file system remount
    and are also logged as "IO error". This could be made more gentle, because
    a checksum catches problems early enough that continuation might be possible.
    - Incremental checksumming
    - Checksums for extents and directories
    - In ignore_crc mode the checksums are only rewritten when the inode is somehow
    dirtied otherwise. Do this implicitely? Might be better to move this to e2fsprogs

diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
index e1def17..e98141a 100644
--- a/Documentation/filesystems/ext4.txt
+++ b/Documentation/filesystems/ext4.txt
@@ -359,6 +359,14 @@ nodiscard(*)		commands to the underlying block device when
 			and sparse/thinly-provisioned LUNs, but it is off
 			by default until sufficient testing has been done.
 
+nosanity_check		Don't check for zero m/c/a times when reading a inode.
+			Should not normally be needed.
+
+ignore_crc		Ignore checksum failures while reading the super block
+			or inodes, but still update the checksums on writing
+			(if you don't want to update the checksums, clear the
+			checksum feature bit in the super block)
+
 Data Mode
 =========
 There are 3 different data modes:
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 19a4de5..3a99769 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -611,6 +611,7 @@ struct ext4_inode {
 	__le32  i_crtime;       /* File Creation time */
 	__le32  i_crtime_extra; /* extra FileCreationtime (nsec << 2 | epoch) */
 	__le32  i_version_hi;	/* high 32 bits for 64-bit version */
+	__le32  i_crc;		/* CRC32 for this inode */
 };
 
 struct move_extent {
@@ -836,6 +837,12 @@ struct ext4_inode_info {
 	 */
 	tid_t i_sync_tid;
 	tid_t i_datasync_tid;
+
+	/* 
+	 * Protect raw inode modifications, mostly to ensure
+	 * stability for checksumming.
+	 */
+	spinlock_t raw_inode_lock;
 };
 
 /*
@@ -881,10 +888,12 @@ struct ext4_inode_info {
 #define EXT4_MOUNT_JOURNAL_CHECKSUM	0x800000 /* Journal checksums */
 #define EXT4_MOUNT_JOURNAL_ASYNC_COMMIT	0x1000000 /* Journal Async Commit */
 #define EXT4_MOUNT_I_VERSION            0x2000000 /* i_version support */
+#define EXT4_MOUNT_IGNORE_CRC           0x4000000 /* Ignore CRCs on read */
 #define EXT4_MOUNT_DELALLOC		0x8000000 /* Delalloc support */
 #define EXT4_MOUNT_DATA_ERR_ABORT	0x10000000 /* Abort on file data write */
 #define EXT4_MOUNT_BLOCK_VALIDITY	0x20000000 /* Block validity checking */
 #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
+#define EXT4_MOUNT_INODE_SANITY         0x80000000 /* Inode sanity check */
 
 #define clear_opt(o, opt)		o &= ~EXT4_MOUNT_##opt
 #define set_opt(o, opt)			o |= EXT4_MOUNT_##opt
@@ -1003,7 +1012,8 @@ struct ext4_super_block {
 	__u8	s_reserved_char_pad2;
 	__le16  s_reserved_pad;
 	__le64	s_kbytes_written;	/* nr of lifetime kilobytes written */
-	__u32   s_reserved[160];        /* Padding to the end of the block */
+	__le32  s_crc;			/* CRC32 for this super block */
+	__u32   s_reserved[159];        /* Padding to the end of the block */
 };
 
 #ifdef __KERNEL__
@@ -1051,6 +1061,7 @@ struct ext4_sb_info {
 	u32 s_hash_seed[4];
 	int s_def_hash_version;
 	int s_hash_unsigned;	/* 3 if hash should be signed, 0 if not */
+	u8 s_uuid[16];
 	struct percpu_counter s_freeblocks_counter;
 	struct percpu_counter s_freeinodes_counter;
 	struct percpu_counter s_dirs_counter;
@@ -1265,6 +1276,7 @@ EXT4_INODE_BIT_FNS(state, state_flags)
 #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
 #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
 #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
+#define EXT4_FEATURE_RO_COMPAT_SBI_CRC		0x0080 /* sb and inode CRCs */
 
 #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
 #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
@@ -1291,7 +1303,8 @@ EXT4_INODE_BIT_FNS(state, state_flags)
 					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
 					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
 					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
-					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
+					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
+					 EXT4_FEATURE_RO_COMPAT_SBI_CRC)
 
 /*
  * Default values for user and/or group using reserved blocks
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 42272d6..8ba2f24 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -37,6 +37,7 @@
 #include <linux/namei.h>
 #include <linux/uio.h>
 #include <linux/bio.h>
+#include <linux/crc32c.h>
 #include <linux/workqueue.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -72,6 +73,141 @@ static int ext4_inode_is_fast_symlink(struct inode *inode)
 	return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
 }
 
+#define ZERO_MAGIC 1
+
+static __le32 inode_crc(struct super_block *sb, struct ext4_inode *raw_inode, long ino)
+{
+	struct ext4_csum_header {
+		__le64 ino;
+		__le64 pad;
+		u8     uuid[16];
+	} __attribute__((aligned)) hdr;
+	u32 crc;
+
+	/*
+	 * First CRC in the inode number and the file system UUID
+	 * to detect inodes from before the last mkfs and misplaced inode
+	 * writes.
+	 */
+	hdr.ino = cpu_to_le64(ino);
+	memcpy(&hdr.uuid, EXT4_SB(sb)->s_uuid, 16);
+	hdr.pad = 0;
+
+	raw_inode->i_crc = 0;
+	crc = crc32c(~0U, &hdr, sizeof(struct ext4_csum_header));
+	/* Could CRC precompute the common zero tail? (if it's really common) */
+	crc = crc32c(crc, raw_inode, EXT4_INODE_SIZE(sb));
+	if (crc == 0)
+		crc = ZERO_MAGIC;
+	return cpu_to_le32(crc);
+}
+
+/*
+ * Update CRC in a inode, including all additional fields after the
+ * standard inode structure.
+ *
+ * This relies on the raw_inode_lock protecting against all writes
+ * to the raw inode state in the bh. Right now the JBD locking
+ * is not good enough for that.
+ *
+ * This always precomputes the full checksum. In principle it would
+ * be possible to be more clever and do incremental changes at least
+ * for some state changes.
+ */
+static void inode_update_crc(struct super_block *sb, struct ext4_inode *raw_inode,
+			     long ino)
+{
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
+		return;
+	raw_inode->i_crc = inode_crc(sb, raw_inode, ino);
+}
+
+/*
+ * This is needed because nfsd might try to access dead inodes
+ * the test is that same one that e2fsck uses
+ * NeilBrown 1999oct15
+ */
+static int inode_deleted(struct super_block *sb, struct ext4_inode *raw_inode)
+{
+	if (raw_inode->i_links_count == 0) {
+		if (raw_inode->i_mode == 0 ||
+		    !(EXT4_SB(sb)->s_mount_state & EXT4_ORPHAN_FS))
+			return -ESTALE;
+		/*
+		 * The only unlinked inodes we let through here have
+		 * valid i_mode and are being read by the orphan
+		 * recovery code: that's fine, we're about to complete
+		 * the process of deleting those.
+		 */
+		return 1;
+	}
+	return 0;
+}
+
+/*
+ * Does this checksum-less inode look like a valid inode?
+ * Do a few sanity checks.
+ */
+static int inode_sanity_check(struct super_block *sb, struct ext4_inode *raw_inode, char **msg)
+{
+	int err = inode_deleted(sb, raw_inode);
+	if (err)
+		return err;
+	if (!test_opt(sb, INODE_SANITY))
+		return 0;
+	if (raw_inode->i_mode == 0 ||
+	    raw_inode->i_atime == 0 ||
+	    raw_inode->i_ctime == 0 ||
+	    raw_inode->i_mtime == 0) {
+		*msg = "inode has invalid zero times";
+		return -1;
+	}
+	/* More sanity checks? */
+	return 0;
+}
+
+/*
+ * Check CRC for a newly read inode.
+ */
+static int inode_check_crc(struct super_block *sb, struct ext4_inode *raw_inode,
+			   long ino, char **msg)
+{
+	__le32 crc, check;
+
+	/*
+	 * Special case: zero CRC.  This is handled like no checksum yet,
+	 * otherwise tune2fs would need to rewrite all inodes when CRCs
+	 * are turned on. The CRC will be updated when the inode
+	 * is written out.
+	 *
+	 * This however means that if zeroes are blasted over the inodes
+	 * we would think the checksum is not there. So instead do
+	 * some special sanity checks in the other fields when this happens,
+	 * to catch this case.
+ 	 * This is not 100% fool proof, but should be reasonable.
+	 * When the checksum function returns a real zero we turn that
+	 * into a one to avoid ambiguity.
+	 *
+	 * The sanity check is done unconditionally even if the checksum passed
+	 * because it's cheap enough.
+	 */
+	crc = raw_inode->i_crc;
+	if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC) && crc
+	    && !test_opt(sb, IGNORE_CRC)) {
+ 		check = inode_crc(sb, raw_inode, ino);
+		if (check != crc) {
+			*msg = "inode has invalid checksum";
+			/*
+			 * Restore bad CRC so that if the inode is reread it'll fail
+			 * the check again.
+			 */
+			raw_inode->i_crc = crc;
+			return -1;
+		}
+	}
+	return inode_sanity_check(sb, raw_inode, msg);
+}
+
 /*
  * Work out how many blocks we need to proceed with the next chunk of a
  * truncate transaction.
@@ -4996,6 +5132,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	journal_t *journal = EXT4_SB(sb)->s_journal;
 	long ret;
 	int block;
+	char *msg = NULL;
 
 	inode = iget_locked(sb, ino);
 	if (!inode)
@@ -5010,6 +5147,26 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	if (ret < 0)
 		goto bad_inode;
 	raw_inode = ext4_raw_inode(&iloc);
+
+ 	/*
+ 	 * Relies on the inode lock to protect the raw_inode bh contents.
+ 	 */
+ 	ret = inode_check_crc(sb, raw_inode, ino, &msg);
+ 	if (ret < 0) {
+ 		/*
+ 		 * Here would be the place to send a "read other mirror"
+ 		 * retry hint to the block layer.
+ 		 */
+ 		brelse(iloc.bh);
+ 		if (ret != -ESTALE)
+ 			ext4_error(sb,
+ 				   "%s: inode=%lu, block=%llu", msg,
+ 				   ino, 
+				   (unsigned long long)iloc.bh->b_blocknr);
+ 		goto bad_inode;
+ 	}
+ 	ret = 0;
+
 	inode->i_mode = le16_to_cpu(raw_inode->i_mode);
 	inode->i_uid = (uid_t)le16_to_cpu(raw_inode->i_uid_low);
 	inode->i_gid = (gid_t)le16_to_cpu(raw_inode->i_gid_low);
@@ -5022,23 +5179,6 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
 	ei->i_state_flags = 0;
 	ei->i_dir_start_lookup = 0;
 	ei->i_dtime = le32_to_cpu(raw_inode->i_dtime);
-	/* We now have enough fields to check if the inode was active or not.
-	 * This is needed because nfsd might try to access dead inodes
-	 * the test is that same one that e2fsck uses
-	 * NeilBrown 1999oct15
-	 */
-	if (inode->i_nlink == 0) {
-		if (inode->i_mode == 0 ||
-		    !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
-			/* this inode is deleted */
-			ret = -ESTALE;
-			goto bad_inode;
-		}
-		/* The only unlinked inodes we let through here have
-		 * valid i_mode and are being read by the orphan
-		 * recovery code: that's fine, we're about to complete
-		 * the process of deleting those. */
-	}
 	ei->i_flags = le32_to_cpu(raw_inode->i_flags);
 	inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
 	ei->i_file_acl = le32_to_cpu(raw_inode->i_file_acl_lo);
@@ -5232,11 +5372,19 @@ static int ext4_do_update_inode(handle_t *handle,
 				struct inode *inode,
 				struct ext4_iloc *iloc)
 {
+	struct super_block *sb = inode->i_sb;
 	struct ext4_inode *raw_inode = ext4_raw_inode(iloc);
 	struct ext4_inode_info *ei = EXT4_I(inode);
 	struct buffer_head *bh = iloc->bh;
 	int err = 0, rc, block;
 
+	/*
+	 * Protect the on disk inode against parallel modification
+	 * until we compute the checksum and pass the resulting block
+	 * to JBD, which protects it then.
+	 */
+	spin_lock(&ei->raw_inode_lock);
+
 	/* For fields not not tracking in the in-memory inode,
 	 * initialise them to zero for new inodes. */
 	if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
@@ -5291,18 +5439,23 @@ static int ext4_do_update_inode(handle_t *handle,
 				EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ||
 				EXT4_SB(sb)->s_es->s_rev_level ==
 				cpu_to_le32(EXT4_GOOD_OLD_REV)) {
+ 			spin_unlock(&ei->raw_inode_lock);
 			/* If this is the first large file
 			 * created, add a flag to the superblock.
 			 */
 			err = ext4_journal_get_write_access(handle,
 					EXT4_SB(sb)->s_sbh);
-			if (err)
+			if (err) {
+ 				spin_lock(&ei->raw_inode_lock);
 				goto out_brelse;
+			}
 			ext4_update_dynamic_rev(sb);
 			EXT4_SET_RO_COMPAT_FEATURE(sb,
 					EXT4_FEATURE_RO_COMPAT_LARGE_FILE);
 			sb->s_dirt = 1;
 			ext4_handle_sync(handle);
+ 			spin_lock(&ei->raw_inode_lock);
+ 			inode_update_crc(sb, raw_inode, inode->i_ino);
 			err = ext4_handle_dirty_metadata(handle, NULL,
 					EXT4_SB(sb)->s_sbh);
 		}
@@ -5330,6 +5483,7 @@ static int ext4_do_update_inode(handle_t *handle,
 			cpu_to_le32(inode->i_version >> 32);
 		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
 	}
+	inode_update_crc(sb, raw_inode, inode->i_ino);
 
 	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
 	rc = ext4_handle_dirty_metadata(handle, NULL, bh);
@@ -5339,6 +5493,7 @@ static int ext4_do_update_inode(handle_t *handle,
 
 	ext4_update_inode_fsync_trans(handle, inode, 0);
 out_brelse:
+	spin_unlock(&ei->raw_inode_lock);
 	brelse(bh);
 	ext4_std_error(inode->i_sb, err);
 	return err;
@@ -5759,6 +5914,8 @@ static int ext4_expand_extra_isize(struct inode *inode,
 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
 		return 0;
 
+	spin_lock(&EXT4_I(inode)->raw_inode_lock);
+
 	raw_inode = ext4_raw_inode(&iloc);
 
 	header = IHDR(inode, raw_inode);
@@ -5770,10 +5927,12 @@ static int ext4_expand_extra_isize(struct inode *inode,
 		memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE, 0,
 			new_extra_isize);
 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
+		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
 		return 0;
 	}
 
 	/* try to expand with EAs present */
+	spin_unlock(&EXT4_I(inode)->raw_inode_lock);
 	return ext4_expand_extra_isize_ea(inode, new_extra_isize,
 					  raw_inode, handle);
 }
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 4e8983a..4012753 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -39,6 +39,7 @@
 #include <linux/ctype.h>
 #include <linux/log2.h>
 #include <linux/crc16.h>
+#include <linux/crc32c.h>
 #include <asm/uaccess.h>
 
 #include "ext4.h"
@@ -70,6 +71,8 @@ static void ext4_write_super(struct super_block *sb);
 static int ext4_freeze(struct super_block *sb);
 static int ext4_get_sb(struct file_system_type *fs_type, int flags,
 		       const char *dev_name, void *data, struct vfsmount *mnt);
+static void ext4_super_update_crc(struct super_block *sb,
+				  struct ext4_super_block *es);
 
 #if !defined(CONFIG_EXT3_FS) && !defined(CONFIG_EXT3_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT23)
 static struct file_system_type ext3_fs_type = {
@@ -342,7 +345,14 @@ static void ext4_handle_error(struct super_block *sb)
 		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
 		sb->s_flags |= MS_RDONLY;
 	}
+	/*
+	 * Unfortunately must take the lock here, to make sure there
+	 * is consistent super state for the checksum. This is very easy to get
+	 * wrong in the caller.
+	 */
+	lock_super(sb);
 	ext4_commit_super(sb, 1);
+	unlock_super(sb);
 	if (test_opt(sb, ERRORS_PANIC))
 		panic("EXT4-fs (device %s): panic forced after error\n",
 			sb->s_id);
@@ -531,7 +541,9 @@ __acquires(bitlock)
 	if (test_opt(sb, ERRORS_CONT)) {
 		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
+		lock_super(sb);
 		ext4_commit_super(sb, 0);
+		unlock_super(sb);
 		return;
 	}
 	ext4_unlock_group(sb, grp);
@@ -659,9 +671,12 @@ static void ext4_put_super(struct super_block *sb)
 	if (sbi->s_journal) {
 		err = jbd2_journal_destroy(sbi->s_journal);
 		sbi->s_journal = NULL;
-		if (err < 0)
+		if (err < 0) {
+			unlock_super(sb);
 			ext4_abort(sb, __func__,
 				   "Couldn't clean up the journal");
+			lock_super(sb);
+		}
 	}
 
 	ext4_release_system_zone(sb);
@@ -758,6 +773,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 	ei->i_da_metadata_calc_len = 0;
 	ei->i_delalloc_reserved_flag = 0;
 	spin_lock_init(&(ei->i_block_reservation_lock));
+	spin_lock_init(&(ei->raw_inode_lock));
 #ifdef CONFIG_QUOTA
 	ei->i_reserved_quota = 0;
 #endif
@@ -1161,6 +1177,7 @@ enum {
 	Opt_inode_readahead_blks, Opt_journal_ioprio,
 	Opt_dioread_nolock, Opt_dioread_lock,
 	Opt_discard, Opt_nodiscard,
+ 	Opt_noinode_sanity, Opt_ignore_crc,
 };
 
 static const match_table_t tokens = {
@@ -1231,6 +1248,8 @@ static const match_table_t tokens = {
 	{Opt_dioread_lock, "dioread_lock"},
 	{Opt_discard, "discard"},
 	{Opt_nodiscard, "nodiscard"},
+ 	{Opt_noinode_sanity, "noinode_sanity"},
+ 	{Opt_ignore_crc, "ignore_crc"},
 	{Opt_err, NULL},
 };
 
@@ -1408,6 +1427,12 @@ static int parse_options(char *options, struct super_block *sb,
 		case Opt_orlov:
 			clear_opt(sbi->s_mount_opt, OLDALLOC);
 			break;
+		case Opt_noinode_sanity:
+			clear_opt(sbi->s_mount_opt, INODE_SANITY);
+			break;
+		case Opt_ignore_crc:
+			set_opt(sbi->s_mount_opt, IGNORE_CRC);
+			break;
 #ifdef CONFIG_EXT4_FS_XATTR
 		case Opt_user_xattr:
 			set_opt(sbi->s_mount_opt, XATTR_USER);
@@ -1946,6 +1971,7 @@ static int ext4_check_descriptors(struct super_block *sb)
 	}
 
 	ext4_free_blocks_count_set(sbi->s_es, ext4_count_free_blocks(sb));
+ 	ext4_super_update_crc(sb, sbi->s_es);
 	sbi->s_es->s_free_inodes_count =cpu_to_le32(ext4_count_free_inodes(sb));
 	return 1;
 }
@@ -2431,6 +2457,50 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly)
 	return 1;
 }
 
+/* could be removed for SBs once e2fsutils are fixed to always compute
+   the CRC when the feature is turned on. */
+#define ZERO_MAGIC 1
+
+/*
+ * Manage CRC32 for the superblock.
+ */
+static int ext4_super_check_crc(struct super_block *sb,
+				struct ext4_super_block *es)
+{
+	u32 crc, check;
+
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
+		return 0;
+	crc = le32_to_cpu(es->s_crc);
+	if (crc == 0 || test_opt(sb, IGNORE_CRC)) {
+		ext4_msg(sb, KERN_INFO, "super block checksum ignored");
+		return 0;
+	}
+	es->s_crc = 0;
+	check = crc32c(~0U, es, sizeof(struct ext4_super_block));
+	if (check == 0)
+		check = ZERO_MAGIC;
+	if (check != crc)
+		return -1;
+	/* Remove me */
+	ext4_msg(sb, KERN_INFO, "super block checksum passed");
+	return 0;
+}
+
+static void ext4_super_update_crc(struct super_block *sb,
+				  struct ext4_super_block *es)
+{
+	u32 crc;
+
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
+		return;
+	es->s_crc = 0;
+        crc = crc32c(~0U, es, sizeof(struct ext4_super_block));
+	if (crc == 0)
+		crc = ZERO_MAGIC;
+	es->s_crc = cpu_to_le32(crc);
+}
+
 static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 				__releases(kernel_lock)
 				__acquires(kernel_lock)
@@ -2554,6 +2624,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	sbi->s_max_batch_time = EXT4_DEF_MAX_BATCH_TIME;
 
 	set_opt(sbi->s_mount_opt, BARRIER);
+	set_opt(sbi->s_mount_opt, INODE_SANITY);
 
 	/*
 	 * enable delayed allocation by default
@@ -2585,6 +2656,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	if (!ext4_feature_set_ok(sb, (sb->s_flags & MS_RDONLY)))
 		goto failed_mount;
 
+	if (ext4_super_check_crc(sb, es) < 0) {
+		ext4_msg(sb, KERN_ERR, "Invalid checksum in super block");
+		goto failed_mount;
+	}
+
 	blocksize = BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
 
 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
@@ -2675,6 +2751,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 
 	for (i = 0; i < 4; i++)
 		sbi->s_hash_seed[i] = le32_to_cpu(es->s_hash_seed[i]);
+	memcpy(sbi->s_uuid, es->s_uuid, 16);
 	sbi->s_def_hash_version = es->s_def_hash_version;
 	i = le32_to_cpu(es->s_flags);
 	if (i & EXT2_FLAGS_UNSIGNED_HASH)
@@ -3393,6 +3470,9 @@ static int ext4_commit_super(struct super_block *sb, int sync)
 	es->s_free_inodes_count = cpu_to_le32(percpu_counter_sum_positive(
 					&EXT4_SB(sb)->s_freeinodes_counter));
 	sb->s_dirt = 0;
+	/* can be removed if this is properly journaled, see
+         * http://bugzilla.kernel.org/show_bug.cgi?id=14354 */
+	ext4_super_update_crc(sb, es);
 	BUFFER_TRACE(sbh, "marking dirty");
 	mark_buffer_dirty(sbh);
 	if (sync) {
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 0433800..fbba95a 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1171,6 +1171,7 @@ retry:
 
 	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
 	if (free >= new_extra_isize) {
+		spin_lock(&EXT4_I(inode)->raw_inode_lock);
 		entry = IFIRST(header);
 		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
 				- new_extra_isize, (void *)raw_inode +
@@ -1179,6 +1180,7 @@ retry:
 				inode->i_sb->s_blocksize);
 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
 		error = 0;
+		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
 		goto cleanup;
 	}
 
@@ -1301,6 +1303,7 @@ retry:
 		if (error)
 			goto cleanup;
 
+		spin_lock(&EXT4_I(inode)->raw_inode_lock);
 		entry = IFIRST(header);
 		if (entry_size + EXT4_XATTR_SIZE(size) >= new_extra_isize)
 			shift_bytes = new_extra_isize;
@@ -1312,6 +1315,7 @@ retry:
 			EXT4_GOOD_OLD_INODE_SIZE + extra_isize + shift_bytes,
 			(void *)header, total_ino - entry_size,
 			inode->i_sb->s_blocksize);
+		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
 
 		extra_isize += shift_bytes;
 		new_extra_isize -= shift_bytes;


-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 0/2] Add inode checksum support to ext4

[Darrick J. Wong]

On Wed, Apr 20, 2011 at 10:40:26AM -0700, Andi Kleen wrote:
> "Darrick J. Wong" <djwong@us.ibm.com> writes:
> 
> FWIW I had a similar idea quite some time ago and implemented checksums for
> superblocks and inodes. I never posted it because I didn't get around
> to write the e2fsprogs support and do a lot of performance testing. 
> There was one result that showed a slow down in quick tests, but 
> I suspect it was a fluke and probably needs to be redone. In other
> tests it was neutral.

I've also seen comparable results between having inode checksums and not having
them.  Unfortunately, like you said, modifying e2fsprogs is really what's
slowing this down right now-- there are a lot of places that assume inode_size
= 128 and therefore only read/write that much.

> Anyways here's the old patch if anyone is interested (against a ~.35ish kernel)
> I can forward port it if there's interest.
> 
> IMHO it's a good idea but it should be done for the super blocks too
> (and ideally for all objects, although unfortunately that breaks 
> the disk format)

I think I've seen some proposals for checksumming the bitmaps and the extent
tree nodes.  It might be worth it to save some rocompat bits and combine them
all into one big(ger) rocompat flag.  I guess it wouldn't be too hard to throw
in superblock checksumming too. :)

> The locking for stable buffers is still something that needs to be
> double checked.



> -Andi
> 
> commit 8ece10cfa4b148075dbb93ca65855a7e2aad7b07
> Author: Andi Kleen <ak@linux.intel.com>
> Date:   Mon May 31 18:27:29 2010 +0200
> 
>     EXT4: Add checksums to the super block and the inodes
>     
>     Currently when a on-disk structure is corrupted ext4 usually
>     detects it only by some internal inconsistency, but this can happen
>     too late or give confusing error messages.
>     
>     One way to detect corruptions sooner is to use checksums. This
>     means the file system can stop using a corrupted object ASAP, not
>     follow any potentially corrupted pointers to other blocks,
>     and also give a clear message on what happened.
>     
>     Potentially it can also be used to limit read only mounts and
>     forced fscks. When the extent of a corruption can be detected
>     accurately using checksums and only force errors on that
>     particular object (this is right now not enabled though
>     because there are still too many objects with missing checksums)
>     
>     There's a general trend in file systems recently to add metadata
>     checksums, this just makes ext4 catch up a bit (in addition
>     to the already existing transaction and block group checksums)
>     
>     Another advantage is that the checksum can also check for misplaced
>     writes (by including the intended block location) and objects
>     left over from before mkfs (by including the file system UUID).
>     
>     Adding checksums to all metadata in ext4 would likely need some
>     incompatible format changes, but it's relatively straight-forward to add
>     them to inodes and the super block at least. This patch-kit does this.
>     
>     Again it only handles some meta-data objects. This is not a full
>     user-data checksumming feature or not even a "all metadata objects"
>     feature.
>     
>     I didn't do a lot of research into different CRC functions, but simply
>     used the same one as btrfs and iSCSI (crc32c).  ocfs2 uses ECCs
>     that have some self-correcting ability, but that seemed overkill to me.
>     The standard CRC has the advantage that the CRC is accelerated by hardware
>     instructions on newer Intel CPUs and possibly on other platforms too.
>     The CRC32C function is also hard coded, although in theory it could be made
>     configurable per file system. But since that would lead to a multitude of
>     incompatible formats I decided to simply hard code for now.
>     
>     Right now there is no e2fsprogs support, so fsck doesn't know about CRCs.
>     
>     For now the checksums can be enabled with a simple shell script
>     (which is just a wrapper around debugfs):
>     
>     wget ftp://firstfloor.org/pub/ak/shell/e2checksum
>     chmod +x e2checksum
>     
>     To enable:
>     
>     ./e2checksum /dev/device enable
>     
>     To disable:
>     
>     ./e2checksum /dev/device disable
>     
>     This works with the current ext4 e2fsprogs without any changes.
>     
>     The kernel will automatically add checksums to the file system when the
>     feature is turned on, as the inodes are rewritten.
>     
>     WARNING: Note there is no fsck support currently, so before you run fsck better
>     disable the checksums. After disabling/changing/reenabling
>     you may also need to mount with ignore_crc until the checksums
>     are fixed up again.
>     
>     The checksums are implemented as a read-only compat feature: this means
>     the a file system with checksums enabled can be read by a kernel that
>     does not know about checksums, but not written. Of course you can turn
>     off the flag again to make the file system write compatible again (but
>     that will put the checksums into a inconsistent state) or use the
>     ignore_crc mount option.
>     
>     WARNING: the in-kernel upgrader relies on the checksum fields being
>     zero when checksums are enabled. When you turn on the feature, use the
>     file system, turn it off again, use it again and turn it on again the
>     checksums will be in a inconsistent state. Right now this can be
>     fixed by mounting with -o ignore_crc and then doing a find | xargs touch
>     on the file system.
>     
>     I expect real e2fsprogs support can do that better.
>     
>     The code also adds some more sanity checks on inodes to distingush zeroed
>     inodes from inodes with no checksums. This is currently done by enforcing
>     that a/m/ctime are not zero. If there are broken file systems around
>     where that is not true, the sanity check can be turned off (see below)
>     
>     There are two new mount options:
>     
>     ignore_crc	       Ignore the CRCs on reading but still update them
>     noinode_sanity	       Don't do new inode sanity checks

Hm, the usage model of my patch is tune2fs; fsck; mount.  I suspect it's a good
idea to run fsck before/while turning on this feature to correct any other
errors.  Though, that means that the kernel will simply -EIO anything it thinks
is corrupt.

>     The implementation is not particularly optimized: it always recomputes
>     the full CRC on each inode or super block write. Some more optimizations
>     would be possible in this area.
>     
>     BENCHMARKS
>     
>     I ran kernelbench and it didn't show any significant difference between
>     the run with checksums and the run without.
>     
>     I also tried timing fsstress from XFS/LTP, and it gave a 35% slowdown
>     on a disk and 5% slow down on the ramdisk for the system time
>     during the test.  However I'm a bit suspicious of these results.
>     This was on a core 2.

I'll give fsstress a try when I get back to this (Friday skunkworks project).

Either way, thanks for the patch!

--D
>     
>     I also tested on a Nehalem system which has CRC acceleration instructions
>     in the CPU.
>     
>     Anyone has a better inode metadata intensive benchmark to run?
>     
>     This gobbled up the last mount flag bits, but there are still some unused
>     holes in the middle.
>     
>     The patch is definitely still experimental and needs more testing and
>     review and e2fsprogs support. In particular I would appreciate review
>     of my bh locking scheme.
>     
>     Also ext4_abort() now takes the super lock, which implies that the callers
>     shouldn't.  I think I checked all callers that it's safe, but double checking
>     that would be good.
>     
>     Opens:
>     - e2fsprogs support. In this case the zero crc hack could be removed from
>     the super block code at least.
>     - Right now checksum numbers by default lead to a read-only file system remount
>     and are also logged as "IO error". This could be made more gentle, because
>     a checksum catches problems early enough that continuation might be possible.
>     - Incremental checksumming
>     - Checksums for extents and directories
>     - In ignore_crc mode the checksums are only rewritten when the inode is somehow
>     dirtied otherwise. Do this implicitely? Might be better to move this to e2fsprogs
> 
> diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
> index e1def17..e98141a 100644
> --- a/Documentation/filesystems/ext4.txt
> +++ b/Documentation/filesystems/ext4.txt
> @@ -359,6 +359,14 @@ nodiscard(*)		commands to the underlying block device when
>  			and sparse/thinly-provisioned LUNs, but it is off
>  			by default until sufficient testing has been done.
> 
> +nosanity_check		Don't check for zero m/c/a times when reading a inode.
> +			Should not normally be needed.
> +
> +ignore_crc		Ignore checksum failures while reading the super block
> +			or inodes, but still update the checksums on writing
> +			(if you don't want to update the checksums, clear the
> +			checksum feature bit in the super block)
> +
>  Data Mode
>  =========
>  There are 3 different data modes:
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 19a4de5..3a99769 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -611,6 +611,7 @@ struct ext4_inode {
>  	__le32  i_crtime;       /* File Creation time */
>  	__le32  i_crtime_extra; /* extra FileCreationtime (nsec << 2 | epoch) */
>  	__le32  i_version_hi;	/* high 32 bits for 64-bit version */
> +	__le32  i_crc;		/* CRC32 for this inode */
>  };
> 
>  struct move_extent {
> @@ -836,6 +837,12 @@ struct ext4_inode_info {
>  	 */
>  	tid_t i_sync_tid;
>  	tid_t i_datasync_tid;
> +
> +	/* 
> +	 * Protect raw inode modifications, mostly to ensure
> +	 * stability for checksumming.
> +	 */
> +	spinlock_t raw_inode_lock;
>  };
> 
>  /*
> @@ -881,10 +888,12 @@ struct ext4_inode_info {
>  #define EXT4_MOUNT_JOURNAL_CHECKSUM	0x800000 /* Journal checksums */
>  #define EXT4_MOUNT_JOURNAL_ASYNC_COMMIT	0x1000000 /* Journal Async Commit */
>  #define EXT4_MOUNT_I_VERSION            0x2000000 /* i_version support */
> +#define EXT4_MOUNT_IGNORE_CRC           0x4000000 /* Ignore CRCs on read */
>  #define EXT4_MOUNT_DELALLOC		0x8000000 /* Delalloc support */
>  #define EXT4_MOUNT_DATA_ERR_ABORT	0x10000000 /* Abort on file data write */
>  #define EXT4_MOUNT_BLOCK_VALIDITY	0x20000000 /* Block validity checking */
>  #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
> +#define EXT4_MOUNT_INODE_SANITY         0x80000000 /* Inode sanity check */
> 
>  #define clear_opt(o, opt)		o &= ~EXT4_MOUNT_##opt
>  #define set_opt(o, opt)			o |= EXT4_MOUNT_##opt
> @@ -1003,7 +1012,8 @@ struct ext4_super_block {
>  	__u8	s_reserved_char_pad2;
>  	__le16  s_reserved_pad;
>  	__le64	s_kbytes_written;	/* nr of lifetime kilobytes written */
> -	__u32   s_reserved[160];        /* Padding to the end of the block */
> +	__le32  s_crc;			/* CRC32 for this super block */
> +	__u32   s_reserved[159];        /* Padding to the end of the block */
>  };
> 
>  #ifdef __KERNEL__
> @@ -1051,6 +1061,7 @@ struct ext4_sb_info {
>  	u32 s_hash_seed[4];
>  	int s_def_hash_version;
>  	int s_hash_unsigned;	/* 3 if hash should be signed, 0 if not */
> +	u8 s_uuid[16];
>  	struct percpu_counter s_freeblocks_counter;
>  	struct percpu_counter s_freeinodes_counter;
>  	struct percpu_counter s_dirs_counter;
> @@ -1265,6 +1276,7 @@ EXT4_INODE_BIT_FNS(state, state_flags)
>  #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
>  #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
>  #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
> +#define EXT4_FEATURE_RO_COMPAT_SBI_CRC		0x0080 /* sb and inode CRCs */
> 
>  #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
>  #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
> @@ -1291,7 +1303,8 @@ EXT4_INODE_BIT_FNS(state, state_flags)
>  					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
>  					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
>  					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
> -					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
> +					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
> +					 EXT4_FEATURE_RO_COMPAT_SBI_CRC)
> 
>  /*
>   * Default values for user and/or group using reserved blocks
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 42272d6..8ba2f24 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -37,6 +37,7 @@
>  #include <linux/namei.h>
>  #include <linux/uio.h>
>  #include <linux/bio.h>
> +#include <linux/crc32c.h>
>  #include <linux/workqueue.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> @@ -72,6 +73,141 @@ static int ext4_inode_is_fast_symlink(struct inode *inode)
>  	return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
>  }
> 
> +#define ZERO_MAGIC 1
> +
> +static __le32 inode_crc(struct super_block *sb, struct ext4_inode *raw_inode, long ino)
> +{
> +	struct ext4_csum_header {
> +		__le64 ino;
> +		__le64 pad;
> +		u8     uuid[16];
> +	} __attribute__((aligned)) hdr;
> +	u32 crc;
> +
> +	/*
> +	 * First CRC in the inode number and the file system UUID
> +	 * to detect inodes from before the last mkfs and misplaced inode
> +	 * writes.
> +	 */
> +	hdr.ino = cpu_to_le64(ino);
> +	memcpy(&hdr.uuid, EXT4_SB(sb)->s_uuid, 16);
> +	hdr.pad = 0;
> +
> +	raw_inode->i_crc = 0;
> +	crc = crc32c(~0U, &hdr, sizeof(struct ext4_csum_header));
> +	/* Could CRC precompute the common zero tail? (if it's really common) */
> +	crc = crc32c(crc, raw_inode, EXT4_INODE_SIZE(sb));
> +	if (crc == 0)
> +		crc = ZERO_MAGIC;
> +	return cpu_to_le32(crc);
> +}
> +
> +/*
> + * Update CRC in a inode, including all additional fields after the
> + * standard inode structure.
> + *
> + * This relies on the raw_inode_lock protecting against all writes
> + * to the raw inode state in the bh. Right now the JBD locking
> + * is not good enough for that.
> + *
> + * This always precomputes the full checksum. In principle it would
> + * be possible to be more clever and do incremental changes at least
> + * for some state changes.
> + */
> +static void inode_update_crc(struct super_block *sb, struct ext4_inode *raw_inode,
> +			     long ino)
> +{
> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
> +		return;
> +	raw_inode->i_crc = inode_crc(sb, raw_inode, ino);
> +}
> +
> +/*
> + * This is needed because nfsd might try to access dead inodes
> + * the test is that same one that e2fsck uses
> + * NeilBrown 1999oct15
> + */
> +static int inode_deleted(struct super_block *sb, struct ext4_inode *raw_inode)
> +{
> +	if (raw_inode->i_links_count == 0) {
> +		if (raw_inode->i_mode == 0 ||
> +		    !(EXT4_SB(sb)->s_mount_state & EXT4_ORPHAN_FS))
> +			return -ESTALE;
> +		/*
> +		 * The only unlinked inodes we let through here have
> +		 * valid i_mode and are being read by the orphan
> +		 * recovery code: that's fine, we're about to complete
> +		 * the process of deleting those.
> +		 */
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Does this checksum-less inode look like a valid inode?
> + * Do a few sanity checks.
> + */
> +static int inode_sanity_check(struct super_block *sb, struct ext4_inode *raw_inode, char **msg)
> +{
> +	int err = inode_deleted(sb, raw_inode);
> +	if (err)
> +		return err;
> +	if (!test_opt(sb, INODE_SANITY))
> +		return 0;
> +	if (raw_inode->i_mode == 0 ||
> +	    raw_inode->i_atime == 0 ||
> +	    raw_inode->i_ctime == 0 ||
> +	    raw_inode->i_mtime == 0) {
> +		*msg = "inode has invalid zero times";
> +		return -1;
> +	}
> +	/* More sanity checks? */
> +	return 0;
> +}
> +
> +/*
> + * Check CRC for a newly read inode.
> + */
> +static int inode_check_crc(struct super_block *sb, struct ext4_inode *raw_inode,
> +			   long ino, char **msg)
> +{
> +	__le32 crc, check;
> +
> +	/*
> +	 * Special case: zero CRC.  This is handled like no checksum yet,
> +	 * otherwise tune2fs would need to rewrite all inodes when CRCs
> +	 * are turned on. The CRC will be updated when the inode
> +	 * is written out.
> +	 *
> +	 * This however means that if zeroes are blasted over the inodes
> +	 * we would think the checksum is not there. So instead do
> +	 * some special sanity checks in the other fields when this happens,
> +	 * to catch this case.
> + 	 * This is not 100% fool proof, but should be reasonable.
> +	 * When the checksum function returns a real zero we turn that
> +	 * into a one to avoid ambiguity.
> +	 *
> +	 * The sanity check is done unconditionally even if the checksum passed
> +	 * because it's cheap enough.
> +	 */
> +	crc = raw_inode->i_crc;
> +	if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC) && crc
> +	    && !test_opt(sb, IGNORE_CRC)) {
> + 		check = inode_crc(sb, raw_inode, ino);
> +		if (check != crc) {
> +			*msg = "inode has invalid checksum";
> +			/*
> +			 * Restore bad CRC so that if the inode is reread it'll fail
> +			 * the check again.
> +			 */
> +			raw_inode->i_crc = crc;
> +			return -1;
> +		}
> +	}
> +	return inode_sanity_check(sb, raw_inode, msg);
> +}
> +
>  /*
>   * Work out how many blocks we need to proceed with the next chunk of a
>   * truncate transaction.
> @@ -4996,6 +5132,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>  	journal_t *journal = EXT4_SB(sb)->s_journal;
>  	long ret;
>  	int block;
> +	char *msg = NULL;
> 
>  	inode = iget_locked(sb, ino);
>  	if (!inode)
> @@ -5010,6 +5147,26 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>  	if (ret < 0)
>  		goto bad_inode;
>  	raw_inode = ext4_raw_inode(&iloc);
> +
> + 	/*
> + 	 * Relies on the inode lock to protect the raw_inode bh contents.
> + 	 */
> + 	ret = inode_check_crc(sb, raw_inode, ino, &msg);
> + 	if (ret < 0) {
> + 		/*
> + 		 * Here would be the place to send a "read other mirror"
> + 		 * retry hint to the block layer.
> + 		 */
> + 		brelse(iloc.bh);
> + 		if (ret != -ESTALE)
> + 			ext4_error(sb,
> + 				   "%s: inode=%lu, block=%llu", msg,
> + 				   ino, 
> +				   (unsigned long long)iloc.bh->b_blocknr);
> + 		goto bad_inode;
> + 	}
> + 	ret = 0;
> +
>  	inode->i_mode = le16_to_cpu(raw_inode->i_mode);
>  	inode->i_uid = (uid_t)le16_to_cpu(raw_inode->i_uid_low);
>  	inode->i_gid = (gid_t)le16_to_cpu(raw_inode->i_gid_low);
> @@ -5022,23 +5179,6 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>  	ei->i_state_flags = 0;
>  	ei->i_dir_start_lookup = 0;
>  	ei->i_dtime = le32_to_cpu(raw_inode->i_dtime);
> -	/* We now have enough fields to check if the inode was active or not.
> -	 * This is needed because nfsd might try to access dead inodes
> -	 * the test is that same one that e2fsck uses
> -	 * NeilBrown 1999oct15
> -	 */
> -	if (inode->i_nlink == 0) {
> -		if (inode->i_mode == 0 ||
> -		    !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
> -			/* this inode is deleted */
> -			ret = -ESTALE;
> -			goto bad_inode;
> -		}
> -		/* The only unlinked inodes we let through here have
> -		 * valid i_mode and are being read by the orphan
> -		 * recovery code: that's fine, we're about to complete
> -		 * the process of deleting those. */
> -	}
>  	ei->i_flags = le32_to_cpu(raw_inode->i_flags);
>  	inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
>  	ei->i_file_acl = le32_to_cpu(raw_inode->i_file_acl_lo);
> @@ -5232,11 +5372,19 @@ static int ext4_do_update_inode(handle_t *handle,
>  				struct inode *inode,
>  				struct ext4_iloc *iloc)
>  {
> +	struct super_block *sb = inode->i_sb;
>  	struct ext4_inode *raw_inode = ext4_raw_inode(iloc);
>  	struct ext4_inode_info *ei = EXT4_I(inode);
>  	struct buffer_head *bh = iloc->bh;
>  	int err = 0, rc, block;
> 
> +	/*
> +	 * Protect the on disk inode against parallel modification
> +	 * until we compute the checksum and pass the resulting block
> +	 * to JBD, which protects it then.
> +	 */
> +	spin_lock(&ei->raw_inode_lock);
> +
>  	/* For fields not not tracking in the in-memory inode,
>  	 * initialise them to zero for new inodes. */
>  	if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
> @@ -5291,18 +5439,23 @@ static int ext4_do_update_inode(handle_t *handle,
>  				EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ||
>  				EXT4_SB(sb)->s_es->s_rev_level ==
>  				cpu_to_le32(EXT4_GOOD_OLD_REV)) {
> + 			spin_unlock(&ei->raw_inode_lock);
>  			/* If this is the first large file
>  			 * created, add a flag to the superblock.
>  			 */
>  			err = ext4_journal_get_write_access(handle,
>  					EXT4_SB(sb)->s_sbh);
> -			if (err)
> +			if (err) {
> + 				spin_lock(&ei->raw_inode_lock);
>  				goto out_brelse;
> +			}
>  			ext4_update_dynamic_rev(sb);
>  			EXT4_SET_RO_COMPAT_FEATURE(sb,
>  					EXT4_FEATURE_RO_COMPAT_LARGE_FILE);
>  			sb->s_dirt = 1;
>  			ext4_handle_sync(handle);
> + 			spin_lock(&ei->raw_inode_lock);
> + 			inode_update_crc(sb, raw_inode, inode->i_ino);
>  			err = ext4_handle_dirty_metadata(handle, NULL,
>  					EXT4_SB(sb)->s_sbh);
>  		}
> @@ -5330,6 +5483,7 @@ static int ext4_do_update_inode(handle_t *handle,
>  			cpu_to_le32(inode->i_version >> 32);
>  		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
>  	}
> +	inode_update_crc(sb, raw_inode, inode->i_ino);
> 
>  	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
>  	rc = ext4_handle_dirty_metadata(handle, NULL, bh);
> @@ -5339,6 +5493,7 @@ static int ext4_do_update_inode(handle_t *handle,
> 
>  	ext4_update_inode_fsync_trans(handle, inode, 0);
>  out_brelse:
> +	spin_unlock(&ei->raw_inode_lock);
>  	brelse(bh);
>  	ext4_std_error(inode->i_sb, err);
>  	return err;
> @@ -5759,6 +5914,8 @@ static int ext4_expand_extra_isize(struct inode *inode,
>  	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
>  		return 0;
> 
> +	spin_lock(&EXT4_I(inode)->raw_inode_lock);
> +
>  	raw_inode = ext4_raw_inode(&iloc);
> 
>  	header = IHDR(inode, raw_inode);
> @@ -5770,10 +5927,12 @@ static int ext4_expand_extra_isize(struct inode *inode,
>  		memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE, 0,
>  			new_extra_isize);
>  		EXT4_I(inode)->i_extra_isize = new_extra_isize;
> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>  		return 0;
>  	}
> 
>  	/* try to expand with EAs present */
> +	spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>  	return ext4_expand_extra_isize_ea(inode, new_extra_isize,
>  					  raw_inode, handle);
>  }
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 4e8983a..4012753 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -39,6 +39,7 @@
>  #include <linux/ctype.h>
>  #include <linux/log2.h>
>  #include <linux/crc16.h>
> +#include <linux/crc32c.h>
>  #include <asm/uaccess.h>
> 
>  #include "ext4.h"
> @@ -70,6 +71,8 @@ static void ext4_write_super(struct super_block *sb);
>  static int ext4_freeze(struct super_block *sb);
>  static int ext4_get_sb(struct file_system_type *fs_type, int flags,
>  		       const char *dev_name, void *data, struct vfsmount *mnt);
> +static void ext4_super_update_crc(struct super_block *sb,
> +				  struct ext4_super_block *es);
> 
>  #if !defined(CONFIG_EXT3_FS) && !defined(CONFIG_EXT3_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT23)
>  static struct file_system_type ext3_fs_type = {
> @@ -342,7 +345,14 @@ static void ext4_handle_error(struct super_block *sb)
>  		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
>  		sb->s_flags |= MS_RDONLY;
>  	}
> +	/*
> +	 * Unfortunately must take the lock here, to make sure there
> +	 * is consistent super state for the checksum. This is very easy to get
> +	 * wrong in the caller.
> +	 */
> +	lock_super(sb);
>  	ext4_commit_super(sb, 1);
> +	unlock_super(sb);
>  	if (test_opt(sb, ERRORS_PANIC))
>  		panic("EXT4-fs (device %s): panic forced after error\n",
>  			sb->s_id);
> @@ -531,7 +541,9 @@ __acquires(bitlock)
>  	if (test_opt(sb, ERRORS_CONT)) {
>  		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
>  		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
> +		lock_super(sb);
>  		ext4_commit_super(sb, 0);
> +		unlock_super(sb);
>  		return;
>  	}
>  	ext4_unlock_group(sb, grp);
> @@ -659,9 +671,12 @@ static void ext4_put_super(struct super_block *sb)
>  	if (sbi->s_journal) {
>  		err = jbd2_journal_destroy(sbi->s_journal);
>  		sbi->s_journal = NULL;
> -		if (err < 0)
> +		if (err < 0) {
> +			unlock_super(sb);
>  			ext4_abort(sb, __func__,
>  				   "Couldn't clean up the journal");
> +			lock_super(sb);
> +		}
>  	}
> 
>  	ext4_release_system_zone(sb);
> @@ -758,6 +773,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
>  	ei->i_da_metadata_calc_len = 0;
>  	ei->i_delalloc_reserved_flag = 0;
>  	spin_lock_init(&(ei->i_block_reservation_lock));
> +	spin_lock_init(&(ei->raw_inode_lock));
>  #ifdef CONFIG_QUOTA
>  	ei->i_reserved_quota = 0;
>  #endif
> @@ -1161,6 +1177,7 @@ enum {
>  	Opt_inode_readahead_blks, Opt_journal_ioprio,
>  	Opt_dioread_nolock, Opt_dioread_lock,
>  	Opt_discard, Opt_nodiscard,
> + 	Opt_noinode_sanity, Opt_ignore_crc,
>  };
> 
>  static const match_table_t tokens = {
> @@ -1231,6 +1248,8 @@ static const match_table_t tokens = {
>  	{Opt_dioread_lock, "dioread_lock"},
>  	{Opt_discard, "discard"},
>  	{Opt_nodiscard, "nodiscard"},
> + 	{Opt_noinode_sanity, "noinode_sanity"},
> + 	{Opt_ignore_crc, "ignore_crc"},
>  	{Opt_err, NULL},
>  };
> 
> @@ -1408,6 +1427,12 @@ static int parse_options(char *options, struct super_block *sb,
>  		case Opt_orlov:
>  			clear_opt(sbi->s_mount_opt, OLDALLOC);
>  			break;
> +		case Opt_noinode_sanity:
> +			clear_opt(sbi->s_mount_opt, INODE_SANITY);
> +			break;
> +		case Opt_ignore_crc:
> +			set_opt(sbi->s_mount_opt, IGNORE_CRC);
> +			break;
>  #ifdef CONFIG_EXT4_FS_XATTR
>  		case Opt_user_xattr:
>  			set_opt(sbi->s_mount_opt, XATTR_USER);
> @@ -1946,6 +1971,7 @@ static int ext4_check_descriptors(struct super_block *sb)
>  	}
> 
>  	ext4_free_blocks_count_set(sbi->s_es, ext4_count_free_blocks(sb));
> + 	ext4_super_update_crc(sb, sbi->s_es);
>  	sbi->s_es->s_free_inodes_count =cpu_to_le32(ext4_count_free_inodes(sb));
>  	return 1;
>  }
> @@ -2431,6 +2457,50 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly)
>  	return 1;
>  }
> 
> +/* could be removed for SBs once e2fsutils are fixed to always compute
> +   the CRC when the feature is turned on. */
> +#define ZERO_MAGIC 1
> +
> +/*
> + * Manage CRC32 for the superblock.
> + */
> +static int ext4_super_check_crc(struct super_block *sb,
> +				struct ext4_super_block *es)
> +{
> +	u32 crc, check;
> +
> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
> +		return 0;
> +	crc = le32_to_cpu(es->s_crc);
> +	if (crc == 0 || test_opt(sb, IGNORE_CRC)) {
> +		ext4_msg(sb, KERN_INFO, "super block checksum ignored");
> +		return 0;
> +	}
> +	es->s_crc = 0;
> +	check = crc32c(~0U, es, sizeof(struct ext4_super_block));
> +	if (check == 0)
> +		check = ZERO_MAGIC;
> +	if (check != crc)
> +		return -1;
> +	/* Remove me */
> +	ext4_msg(sb, KERN_INFO, "super block checksum passed");
> +	return 0;
> +}
> +
> +static void ext4_super_update_crc(struct super_block *sb,
> +				  struct ext4_super_block *es)
> +{
> +	u32 crc;
> +
> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
> +		return;
> +	es->s_crc = 0;
> +        crc = crc32c(~0U, es, sizeof(struct ext4_super_block));
> +	if (crc == 0)
> +		crc = ZERO_MAGIC;
> +	es->s_crc = cpu_to_le32(crc);
> +}
> +
>  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  				__releases(kernel_lock)
>  				__acquires(kernel_lock)
> @@ -2554,6 +2624,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	sbi->s_max_batch_time = EXT4_DEF_MAX_BATCH_TIME;
> 
>  	set_opt(sbi->s_mount_opt, BARRIER);
> +	set_opt(sbi->s_mount_opt, INODE_SANITY);
> 
>  	/*
>  	 * enable delayed allocation by default
> @@ -2585,6 +2656,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!ext4_feature_set_ok(sb, (sb->s_flags & MS_RDONLY)))
>  		goto failed_mount;
> 
> +	if (ext4_super_check_crc(sb, es) < 0) {
> +		ext4_msg(sb, KERN_ERR, "Invalid checksum in super block");
> +		goto failed_mount;
> +	}
> +
>  	blocksize = BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
> 
>  	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
> @@ -2675,6 +2751,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> 
>  	for (i = 0; i < 4; i++)
>  		sbi->s_hash_seed[i] = le32_to_cpu(es->s_hash_seed[i]);
> +	memcpy(sbi->s_uuid, es->s_uuid, 16);
>  	sbi->s_def_hash_version = es->s_def_hash_version;
>  	i = le32_to_cpu(es->s_flags);
>  	if (i & EXT2_FLAGS_UNSIGNED_HASH)
> @@ -3393,6 +3470,9 @@ static int ext4_commit_super(struct super_block *sb, int sync)
>  	es->s_free_inodes_count = cpu_to_le32(percpu_counter_sum_positive(
>  					&EXT4_SB(sb)->s_freeinodes_counter));
>  	sb->s_dirt = 0;
> +	/* can be removed if this is properly journaled, see
> +         * http://bugzilla.kernel.org/show_bug.cgi?id=14354 */
> +	ext4_super_update_crc(sb, es);
>  	BUFFER_TRACE(sbh, "marking dirty");
>  	mark_buffer_dirty(sbh);
>  	if (sync) {
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 0433800..fbba95a 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1171,6 +1171,7 @@ retry:
> 
>  	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
>  	if (free >= new_extra_isize) {
> +		spin_lock(&EXT4_I(inode)->raw_inode_lock);
>  		entry = IFIRST(header);
>  		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
>  				- new_extra_isize, (void *)raw_inode +
> @@ -1179,6 +1180,7 @@ retry:
>  				inode->i_sb->s_blocksize);
>  		EXT4_I(inode)->i_extra_isize = new_extra_isize;
>  		error = 0;
> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>  		goto cleanup;
>  	}
> 
> @@ -1301,6 +1303,7 @@ retry:
>  		if (error)
>  			goto cleanup;
> 
> +		spin_lock(&EXT4_I(inode)->raw_inode_lock);
>  		entry = IFIRST(header);
>  		if (entry_size + EXT4_XATTR_SIZE(size) >= new_extra_isize)
>  			shift_bytes = new_extra_isize;
> @@ -1312,6 +1315,7 @@ retry:
>  			EXT4_GOOD_OLD_INODE_SIZE + extra_isize + shift_bytes,
>  			(void *)header, total_ino - entry_size,
>  			inode->i_sb->s_blocksize);
> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
> 
>  		extra_isize += shift_bytes;
>  		new_extra_isize -= shift_bytes;
> 
> 
> -- 
> ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 0/2] Add inode checksum support to ext4

[Andreas Dilger]

On 2011-04-20, at 4:54 PM, Darrick J. Wong wrote:
> On Wed, Apr 20, 2011 at 10:40:26AM -0700, Andi Kleen wrote:
>> "Darrick J. Wong" <djwong@us.ibm.com> writes:
>> 
>> FWIW I had a similar idea quite some time ago and implemented checksums for
>> superblocks and inodes. I never posted it because I didn't get around
>> to write the e2fsprogs support and do a lot of performance testing. 
>> There was one result that showed a slow down in quick tests, but 
>> I suspect it was a fluke and probably needs to be redone. In other
>> tests it was neutral.
> 
> I've also seen comparable results between having inode checksums and not having
> them.  Unfortunately, like you said, modifying e2fsprogs is really what's
> slowing this down right now-- there are a lot of places that assume inode_size
> = 128 and therefore only read/write that much.

I think it makes sense to include the inode checksum into the core 128-bit inode, so that it is available to all upgraded ext3 filesystems as well.  Having a 16-bit checksum is probably sufficient for the 128- or 256-byte inodes, I don't know that we really need to have a full 32-bit checksum?  Also, the current group descriptor checksums are already CRC-16 so it probably makes sense to stick with that.

>> Anyways here's the old patch if anyone is interested (against a ~.35ish kernel)
>> I can forward port it if there's interest.
>> 
>> IMHO it's a good idea but it should be done for the super blocks too
>> (and ideally for all objects, although unfortunately that breaks 
>> the disk format)
> 
> I think I've seen some proposals for checksumming the bitmaps and the extent
> tree nodes.  It might be worth it to save some rocompat bits and combine them
> all into one big(ger) rocompat flag.  I guess it wouldn't be too hard to throw
> in superblock checksumming too. :)
> 
>> The locking for stable buffers is still something that needs to be
>> double checked.
> 
> 
> 
>> -Andi
>> 
>> commit 8ece10cfa4b148075dbb93ca65855a7e2aad7b07
>> Author: Andi Kleen <ak@linux.intel.com>
>> Date:   Mon May 31 18:27:29 2010 +0200
>> 
>>    EXT4: Add checksums to the super block and the inodes
>> 
>>    Currently when a on-disk structure is corrupted ext4 usually
>>    detects it only by some internal inconsistency, but this can happen
>>    too late or give confusing error messages.
>> 
>>    One way to detect corruptions sooner is to use checksums. This
>>    means the file system can stop using a corrupted object ASAP, not
>>    follow any potentially corrupted pointers to other blocks,
>>    and also give a clear message on what happened.
>> 
>>    Potentially it can also be used to limit read only mounts and
>>    forced fscks. When the extent of a corruption can be detected
>>    accurately using checksums and only force errors on that
>>    particular object (this is right now not enabled though
>>    because there are still too many objects with missing checksums)
>> 
>>    There's a general trend in file systems recently to add metadata
>>    checksums, this just makes ext4 catch up a bit (in addition
>>    to the already existing transaction and block group checksums)
>> 
>>    Another advantage is that the checksum can also check for misplaced
>>    writes (by including the intended block location) and objects
>>    left over from before mkfs (by including the file system UUID).
>> 
>>    Adding checksums to all metadata in ext4 would likely need some
>>    incompatible format changes, but it's relatively straight-forward to add
>>    them to inodes and the super block at least. This patch-kit does this.
>> 
>>    Again it only handles some meta-data objects. This is not a full
>>    user-data checksumming feature or not even a "all metadata objects"
>>    feature.
>> 
>>    I didn't do a lot of research into different CRC functions, but simply
>>    used the same one as btrfs and iSCSI (crc32c).  ocfs2 uses ECCs
>>    that have some self-correcting ability, but that seemed overkill to me.
>>    The standard CRC has the advantage that the CRC is accelerated by hardware
>>    instructions on newer Intel CPUs and possibly on other platforms too.
>>    The CRC32C function is also hard coded, although in theory it could be made
>>    configurable per file system. But since that would lead to a multitude of
>>    incompatible formats I decided to simply hard code for now.
>> 
>>    Right now there is no e2fsprogs support, so fsck doesn't know about CRCs.
>> 
>>    For now the checksums can be enabled with a simple shell script
>>    (which is just a wrapper around debugfs):
>> 
>>    wget ftp://firstfloor.org/pub/ak/shell/e2checksum
>>    chmod +x e2checksum
>> 
>>    To enable:
>> 
>>    ./e2checksum /dev/device enable
>> 
>>    To disable:
>> 
>>    ./e2checksum /dev/device disable
>> 
>>    This works with the current ext4 e2fsprogs without any changes.
>> 
>>    The kernel will automatically add checksums to the file system when the
>>    feature is turned on, as the inodes are rewritten.
>> 
>>    WARNING: Note there is no fsck support currently, so before you run fsck better
>>    disable the checksums. After disabling/changing/reenabling
>>    you may also need to mount with ignore_crc until the checksums
>>    are fixed up again.
>> 
>>    The checksums are implemented as a read-only compat feature: this means
>>    the a file system with checksums enabled can be read by a kernel that
>>    does not know about checksums, but not written. Of course you can turn
>>    off the flag again to make the file system write compatible again (but
>>    that will put the checksums into a inconsistent state) or use the
>>    ignore_crc mount option.
>> 
>>    WARNING: the in-kernel upgrader relies on the checksum fields being
>>    zero when checksums are enabled. When you turn on the feature, use the
>>    file system, turn it off again, use it again and turn it on again the
>>    checksums will be in a inconsistent state. Right now this can be
>>    fixed by mounting with -o ignore_crc and then doing a find | xargs touch
>>    on the file system.
>> 
>>    I expect real e2fsprogs support can do that better.
>> 
>>    The code also adds some more sanity checks on inodes to distingush zeroed
>>    inodes from inodes with no checksums. This is currently done by enforcing
>>    that a/m/ctime are not zero. If there are broken file systems around
>>    where that is not true, the sanity check can be turned off (see below)
>> 
>>    There are two new mount options:
>> 
>>    ignore_crc	       Ignore the CRCs on reading but still update them
>>    noinode_sanity	       Don't do new inode sanity checks
> 
> Hm, the usage model of my patch is tune2fs; fsck; mount.  I suspect it's a good
> idea to run fsck before/while turning on this feature to correct any other
> errors.  Though, that means that the kernel will simply -EIO anything it thinks
> is corrupt.
> 
>>    The implementation is not particularly optimized: it always recomputes
>>    the full CRC on each inode or super block write. Some more optimizations
>>    would be possible in this area.
>> 
>>    BENCHMARKS
>> 
>>    I ran kernelbench and it didn't show any significant difference between
>>    the run with checksums and the run without.
>> 
>>    I also tried timing fsstress from XFS/LTP, and it gave a 35% slowdown
>>    on a disk and 5% slow down on the ramdisk for the system time
>>    during the test.  However I'm a bit suspicious of these results.
>>    This was on a core 2.
> 
> I'll give fsstress a try when I get back to this (Friday skunkworks project).
> 
> Either way, thanks for the patch!
> 
> --D
>> 
>>    I also tested on a Nehalem system which has CRC acceleration instructions
>>    in the CPU.
>> 
>>    Anyone has a better inode metadata intensive benchmark to run?
>> 
>>    This gobbled up the last mount flag bits, but there are still some unused
>>    holes in the middle.
>> 
>>    The patch is definitely still experimental and needs more testing and
>>    review and e2fsprogs support. In particular I would appreciate review
>>    of my bh locking scheme.
>> 
>>    Also ext4_abort() now takes the super lock, which implies that the callers
>>    shouldn't.  I think I checked all callers that it's safe, but double checking
>>    that would be good.
>> 
>>    Opens:
>>    - e2fsprogs support. In this case the zero crc hack could be removed from
>>    the super block code at least.
>>    - Right now checksum numbers by default lead to a read-only file system remount
>>    and are also logged as "IO error". This could be made more gentle, because
>>    a checksum catches problems early enough that continuation might be possible.
>>    - Incremental checksumming
>>    - Checksums for extents and directories
>>    - In ignore_crc mode the checksums are only rewritten when the inode is somehow
>>    dirtied otherwise. Do this implicitely? Might be better to move this to e2fsprogs
>> 
>> diff --git a/Documentation/filesystems/ext4.txt b/Documentation/filesystems/ext4.txt
>> index e1def17..e98141a 100644
>> --- a/Documentation/filesystems/ext4.txt
>> +++ b/Documentation/filesystems/ext4.txt
>> @@ -359,6 +359,14 @@ nodiscard(*)		commands to the underlying block device when
>> 			and sparse/thinly-provisioned LUNs, but it is off
>> 			by default until sufficient testing has been done.
>> 
>> +nosanity_check		Don't check for zero m/c/a times when reading a inode.
>> +			Should not normally be needed.
>> +
>> +ignore_crc		Ignore checksum failures while reading the super block
>> +			or inodes, but still update the checksums on writing
>> +			(if you don't want to update the checksums, clear the
>> +			checksum feature bit in the super block)
>> +
>> Data Mode
>> =========
>> There are 3 different data modes:
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 19a4de5..3a99769 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -611,6 +611,7 @@ struct ext4_inode {
>> 	__le32  i_crtime;       /* File Creation time */
>> 	__le32  i_crtime_extra; /* extra FileCreationtime (nsec << 2 | epoch) */
>> 	__le32  i_version_hi;	/* high 32 bits for 64-bit version */
>> +	__le32  i_crc;		/* CRC32 for this inode */
>> };
>> 
>> struct move_extent {
>> @@ -836,6 +837,12 @@ struct ext4_inode_info {
>> 	 */
>> 	tid_t i_sync_tid;
>> 	tid_t i_datasync_tid;
>> +
>> +	/* 
>> +	 * Protect raw inode modifications, mostly to ensure
>> +	 * stability for checksumming.
>> +	 */
>> +	spinlock_t raw_inode_lock;
>> };
>> 
>> /*
>> @@ -881,10 +888,12 @@ struct ext4_inode_info {
>> #define EXT4_MOUNT_JOURNAL_CHECKSUM	0x800000 /* Journal checksums */
>> #define EXT4_MOUNT_JOURNAL_ASYNC_COMMIT	0x1000000 /* Journal Async Commit */
>> #define EXT4_MOUNT_I_VERSION            0x2000000 /* i_version support */
>> +#define EXT4_MOUNT_IGNORE_CRC           0x4000000 /* Ignore CRCs on read */
>> #define EXT4_MOUNT_DELALLOC		0x8000000 /* Delalloc support */
>> #define EXT4_MOUNT_DATA_ERR_ABORT	0x10000000 /* Abort on file data write */
>> #define EXT4_MOUNT_BLOCK_VALIDITY	0x20000000 /* Block validity checking */
>> #define EXT4_MOUNT_DISCARD		0x40000000 /* Issue DISCARD requests */
>> +#define EXT4_MOUNT_INODE_SANITY         0x80000000 /* Inode sanity check */
>> 
>> #define clear_opt(o, opt)		o &= ~EXT4_MOUNT_##opt
>> #define set_opt(o, opt)			o |= EXT4_MOUNT_##opt
>> @@ -1003,7 +1012,8 @@ struct ext4_super_block {
>> 	__u8	s_reserved_char_pad2;
>> 	__le16  s_reserved_pad;
>> 	__le64	s_kbytes_written;	/* nr of lifetime kilobytes written */
>> -	__u32   s_reserved[160];        /* Padding to the end of the block */
>> +	__le32  s_crc;			/* CRC32 for this super block */
>> +	__u32   s_reserved[159];        /* Padding to the end of the block */
>> };
>> 
>> #ifdef __KERNEL__
>> @@ -1051,6 +1061,7 @@ struct ext4_sb_info {
>> 	u32 s_hash_seed[4];
>> 	int s_def_hash_version;
>> 	int s_hash_unsigned;	/* 3 if hash should be signed, 0 if not */
>> +	u8 s_uuid[16];
>> 	struct percpu_counter s_freeblocks_counter;
>> 	struct percpu_counter s_freeinodes_counter;
>> 	struct percpu_counter s_dirs_counter;
>> @@ -1265,6 +1276,7 @@ EXT4_INODE_BIT_FNS(state, state_flags)
>> #define EXT4_FEATURE_RO_COMPAT_GDT_CSUM		0x0010
>> #define EXT4_FEATURE_RO_COMPAT_DIR_NLINK	0x0020
>> #define EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE	0x0040
>> +#define EXT4_FEATURE_RO_COMPAT_SBI_CRC		0x0080 /* sb and inode CRCs */
>> 
>> #define EXT4_FEATURE_INCOMPAT_COMPRESSION	0x0001
>> #define EXT4_FEATURE_INCOMPAT_FILETYPE		0x0002
>> @@ -1291,7 +1303,8 @@ EXT4_INODE_BIT_FNS(state, state_flags)
>> 					 EXT4_FEATURE_RO_COMPAT_DIR_NLINK | \
>> 					 EXT4_FEATURE_RO_COMPAT_EXTRA_ISIZE | \
>> 					 EXT4_FEATURE_RO_COMPAT_BTREE_DIR |\
>> -					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE)
>> +					 EXT4_FEATURE_RO_COMPAT_HUGE_FILE |\
>> +					 EXT4_FEATURE_RO_COMPAT_SBI_CRC)
>> 
>> /*
>>  * Default values for user and/or group using reserved blocks
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 42272d6..8ba2f24 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -37,6 +37,7 @@
>> #include <linux/namei.h>
>> #include <linux/uio.h>
>> #include <linux/bio.h>
>> +#include <linux/crc32c.h>
>> #include <linux/workqueue.h>
>> #include <linux/kernel.h>
>> #include <linux/slab.h>
>> @@ -72,6 +73,141 @@ static int ext4_inode_is_fast_symlink(struct inode *inode)
>> 	return (S_ISLNK(inode->i_mode) && inode->i_blocks - ea_blocks == 0);
>> }
>> 
>> +#define ZERO_MAGIC 1
>> +
>> +static __le32 inode_crc(struct super_block *sb, struct ext4_inode *raw_inode, long ino)
>> +{
>> +	struct ext4_csum_header {
>> +		__le64 ino;
>> +		__le64 pad;
>> +		u8     uuid[16];
>> +	} __attribute__((aligned)) hdr;
>> +	u32 crc;
>> +
>> +	/*
>> +	 * First CRC in the inode number and the file system UUID
>> +	 * to detect inodes from before the last mkfs and misplaced inode
>> +	 * writes.
>> +	 */
>> +	hdr.ino = cpu_to_le64(ino);
>> +	memcpy(&hdr.uuid, EXT4_SB(sb)->s_uuid, 16);
>> +	hdr.pad = 0;
>> +
>> +	raw_inode->i_crc = 0;
>> +	crc = crc32c(~0U, &hdr, sizeof(struct ext4_csum_header));
>> +	/* Could CRC precompute the common zero tail? (if it's really common) */
>> +	crc = crc32c(crc, raw_inode, EXT4_INODE_SIZE(sb));
>> +	if (crc == 0)
>> +		crc = ZERO_MAGIC;
>> +	return cpu_to_le32(crc);
>> +}
>> +
>> +/*
>> + * Update CRC in a inode, including all additional fields after the
>> + * standard inode structure.
>> + *
>> + * This relies on the raw_inode_lock protecting against all writes
>> + * to the raw inode state in the bh. Right now the JBD locking
>> + * is not good enough for that.
>> + *
>> + * This always precomputes the full checksum. In principle it would
>> + * be possible to be more clever and do incremental changes at least
>> + * for some state changes.
>> + */
>> +static void inode_update_crc(struct super_block *sb, struct ext4_inode *raw_inode,
>> +			     long ino)
>> +{
>> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
>> +		return;
>> +	raw_inode->i_crc = inode_crc(sb, raw_inode, ino);
>> +}
>> +
>> +/*
>> + * This is needed because nfsd might try to access dead inodes
>> + * the test is that same one that e2fsck uses
>> + * NeilBrown 1999oct15
>> + */
>> +static int inode_deleted(struct super_block *sb, struct ext4_inode *raw_inode)
>> +{
>> +	if (raw_inode->i_links_count == 0) {
>> +		if (raw_inode->i_mode == 0 ||
>> +		    !(EXT4_SB(sb)->s_mount_state & EXT4_ORPHAN_FS))
>> +			return -ESTALE;
>> +		/*
>> +		 * The only unlinked inodes we let through here have
>> +		 * valid i_mode and are being read by the orphan
>> +		 * recovery code: that's fine, we're about to complete
>> +		 * the process of deleting those.
>> +		 */
>> +		return 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Does this checksum-less inode look like a valid inode?
>> + * Do a few sanity checks.
>> + */
>> +static int inode_sanity_check(struct super_block *sb, struct ext4_inode *raw_inode, char **msg)
>> +{
>> +	int err = inode_deleted(sb, raw_inode);
>> +	if (err)
>> +		return err;
>> +	if (!test_opt(sb, INODE_SANITY))
>> +		return 0;
>> +	if (raw_inode->i_mode == 0 ||
>> +	    raw_inode->i_atime == 0 ||
>> +	    raw_inode->i_ctime == 0 ||
>> +	    raw_inode->i_mtime == 0) {
>> +		*msg = "inode has invalid zero times";
>> +		return -1;
>> +	}
>> +	/* More sanity checks? */
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Check CRC for a newly read inode.
>> + */
>> +static int inode_check_crc(struct super_block *sb, struct ext4_inode *raw_inode,
>> +			   long ino, char **msg)
>> +{
>> +	__le32 crc, check;
>> +
>> +	/*
>> +	 * Special case: zero CRC.  This is handled like no checksum yet,
>> +	 * otherwise tune2fs would need to rewrite all inodes when CRCs
>> +	 * are turned on. The CRC will be updated when the inode
>> +	 * is written out.
>> +	 *
>> +	 * This however means that if zeroes are blasted over the inodes
>> +	 * we would think the checksum is not there. So instead do
>> +	 * some special sanity checks in the other fields when this happens,
>> +	 * to catch this case.
>> + 	 * This is not 100% fool proof, but should be reasonable.
>> +	 * When the checksum function returns a real zero we turn that
>> +	 * into a one to avoid ambiguity.
>> +	 *
>> +	 * The sanity check is done unconditionally even if the checksum passed
>> +	 * because it's cheap enough.
>> +	 */
>> +	crc = raw_inode->i_crc;
>> +	if (EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC) && crc
>> +	    && !test_opt(sb, IGNORE_CRC)) {
>> + 		check = inode_crc(sb, raw_inode, ino);
>> +		if (check != crc) {
>> +			*msg = "inode has invalid checksum";
>> +			/*
>> +			 * Restore bad CRC so that if the inode is reread it'll fail
>> +			 * the check again.
>> +			 */
>> +			raw_inode->i_crc = crc;
>> +			return -1;
>> +		}
>> +	}
>> +	return inode_sanity_check(sb, raw_inode, msg);
>> +}
>> +
>> /*
>>  * Work out how many blocks we need to proceed with the next chunk of a
>>  * truncate transaction.
>> @@ -4996,6 +5132,7 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>> 	journal_t *journal = EXT4_SB(sb)->s_journal;
>> 	long ret;
>> 	int block;
>> +	char *msg = NULL;
>> 
>> 	inode = iget_locked(sb, ino);
>> 	if (!inode)
>> @@ -5010,6 +5147,26 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>> 	if (ret < 0)
>> 		goto bad_inode;
>> 	raw_inode = ext4_raw_inode(&iloc);
>> +
>> + 	/*
>> + 	 * Relies on the inode lock to protect the raw_inode bh contents.
>> + 	 */
>> + 	ret = inode_check_crc(sb, raw_inode, ino, &msg);
>> + 	if (ret < 0) {
>> + 		/*
>> + 		 * Here would be the place to send a "read other mirror"
>> + 		 * retry hint to the block layer.
>> + 		 */
>> + 		brelse(iloc.bh);
>> + 		if (ret != -ESTALE)
>> + 			ext4_error(sb,
>> + 				   "%s: inode=%lu, block=%llu", msg,
>> + 				   ino, 
>> +				   (unsigned long long)iloc.bh->b_blocknr);
>> + 		goto bad_inode;
>> + 	}
>> + 	ret = 0;
>> +
>> 	inode->i_mode = le16_to_cpu(raw_inode->i_mode);
>> 	inode->i_uid = (uid_t)le16_to_cpu(raw_inode->i_uid_low);
>> 	inode->i_gid = (gid_t)le16_to_cpu(raw_inode->i_gid_low);
>> @@ -5022,23 +5179,6 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)
>> 	ei->i_state_flags = 0;
>> 	ei->i_dir_start_lookup = 0;
>> 	ei->i_dtime = le32_to_cpu(raw_inode->i_dtime);
>> -	/* We now have enough fields to check if the inode was active or not.
>> -	 * This is needed because nfsd might try to access dead inodes
>> -	 * the test is that same one that e2fsck uses
>> -	 * NeilBrown 1999oct15
>> -	 */
>> -	if (inode->i_nlink == 0) {
>> -		if (inode->i_mode == 0 ||
>> -		    !(EXT4_SB(inode->i_sb)->s_mount_state & EXT4_ORPHAN_FS)) {
>> -			/* this inode is deleted */
>> -			ret = -ESTALE;
>> -			goto bad_inode;
>> -		}
>> -		/* The only unlinked inodes we let through here have
>> -		 * valid i_mode and are being read by the orphan
>> -		 * recovery code: that's fine, we're about to complete
>> -		 * the process of deleting those. */
>> -	}
>> 	ei->i_flags = le32_to_cpu(raw_inode->i_flags);
>> 	inode->i_blocks = ext4_inode_blocks(raw_inode, ei);
>> 	ei->i_file_acl = le32_to_cpu(raw_inode->i_file_acl_lo);
>> @@ -5232,11 +5372,19 @@ static int ext4_do_update_inode(handle_t *handle,
>> 				struct inode *inode,
>> 				struct ext4_iloc *iloc)
>> {
>> +	struct super_block *sb = inode->i_sb;
>> 	struct ext4_inode *raw_inode = ext4_raw_inode(iloc);
>> 	struct ext4_inode_info *ei = EXT4_I(inode);
>> 	struct buffer_head *bh = iloc->bh;
>> 	int err = 0, rc, block;
>> 
>> +	/*
>> +	 * Protect the on disk inode against parallel modification
>> +	 * until we compute the checksum and pass the resulting block
>> +	 * to JBD, which protects it then.
>> +	 */
>> +	spin_lock(&ei->raw_inode_lock);
>> +
>> 	/* For fields not not tracking in the in-memory inode,
>> 	 * initialise them to zero for new inodes. */
>> 	if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
>> @@ -5291,18 +5439,23 @@ static int ext4_do_update_inode(handle_t *handle,
>> 				EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ||
>> 				EXT4_SB(sb)->s_es->s_rev_level ==
>> 				cpu_to_le32(EXT4_GOOD_OLD_REV)) {
>> + 			spin_unlock(&ei->raw_inode_lock);
>> 			/* If this is the first large file
>> 			 * created, add a flag to the superblock.
>> 			 */
>> 			err = ext4_journal_get_write_access(handle,
>> 					EXT4_SB(sb)->s_sbh);
>> -			if (err)
>> +			if (err) {
>> + 				spin_lock(&ei->raw_inode_lock);
>> 				goto out_brelse;
>> +			}
>> 			ext4_update_dynamic_rev(sb);
>> 			EXT4_SET_RO_COMPAT_FEATURE(sb,
>> 					EXT4_FEATURE_RO_COMPAT_LARGE_FILE);
>> 			sb->s_dirt = 1;
>> 			ext4_handle_sync(handle);
>> + 			spin_lock(&ei->raw_inode_lock);
>> + 			inode_update_crc(sb, raw_inode, inode->i_ino);
>> 			err = ext4_handle_dirty_metadata(handle, NULL,
>> 					EXT4_SB(sb)->s_sbh);
>> 		}
>> @@ -5330,6 +5483,7 @@ static int ext4_do_update_inode(handle_t *handle,
>> 			cpu_to_le32(inode->i_version >> 32);
>> 		raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
>> 	}
>> +	inode_update_crc(sb, raw_inode, inode->i_ino);
>> 
>> 	BUFFER_TRACE(bh, "call ext4_handle_dirty_metadata");
>> 	rc = ext4_handle_dirty_metadata(handle, NULL, bh);
>> @@ -5339,6 +5493,7 @@ static int ext4_do_update_inode(handle_t *handle,
>> 
>> 	ext4_update_inode_fsync_trans(handle, inode, 0);
>> out_brelse:
>> +	spin_unlock(&ei->raw_inode_lock);
>> 	brelse(bh);
>> 	ext4_std_error(inode->i_sb, err);
>> 	return err;
>> @@ -5759,6 +5914,8 @@ static int ext4_expand_extra_isize(struct inode *inode,
>> 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
>> 		return 0;
>> 
>> +	spin_lock(&EXT4_I(inode)->raw_inode_lock);
>> +
>> 	raw_inode = ext4_raw_inode(&iloc);
>> 
>> 	header = IHDR(inode, raw_inode);
>> @@ -5770,10 +5927,12 @@ static int ext4_expand_extra_isize(struct inode *inode,
>> 		memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE, 0,
>> 			new_extra_isize);
>> 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
>> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>> 		return 0;
>> 	}
>> 
>> 	/* try to expand with EAs present */
>> +	spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>> 	return ext4_expand_extra_isize_ea(inode, new_extra_isize,
>> 					  raw_inode, handle);
>> }
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index 4e8983a..4012753 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -39,6 +39,7 @@
>> #include <linux/ctype.h>
>> #include <linux/log2.h>
>> #include <linux/crc16.h>
>> +#include <linux/crc32c.h>
>> #include <asm/uaccess.h>
>> 
>> #include "ext4.h"
>> @@ -70,6 +71,8 @@ static void ext4_write_super(struct super_block *sb);
>> static int ext4_freeze(struct super_block *sb);
>> static int ext4_get_sb(struct file_system_type *fs_type, int flags,
>> 		       const char *dev_name, void *data, struct vfsmount *mnt);
>> +static void ext4_super_update_crc(struct super_block *sb,
>> +				  struct ext4_super_block *es);
>> 
>> #if !defined(CONFIG_EXT3_FS) && !defined(CONFIG_EXT3_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT23)
>> static struct file_system_type ext3_fs_type = {
>> @@ -342,7 +345,14 @@ static void ext4_handle_error(struct super_block *sb)
>> 		ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
>> 		sb->s_flags |= MS_RDONLY;
>> 	}
>> +	/*
>> +	 * Unfortunately must take the lock here, to make sure there
>> +	 * is consistent super state for the checksum. This is very easy to get
>> +	 * wrong in the caller.
>> +	 */
>> +	lock_super(sb);
>> 	ext4_commit_super(sb, 1);
>> +	unlock_super(sb);
>> 	if (test_opt(sb, ERRORS_PANIC))
>> 		panic("EXT4-fs (device %s): panic forced after error\n",
>> 			sb->s_id);
>> @@ -531,7 +541,9 @@ __acquires(bitlock)
>> 	if (test_opt(sb, ERRORS_CONT)) {
>> 		EXT4_SB(sb)->s_mount_state |= EXT4_ERROR_FS;
>> 		es->s_state |= cpu_to_le16(EXT4_ERROR_FS);
>> +		lock_super(sb);
>> 		ext4_commit_super(sb, 0);
>> +		unlock_super(sb);
>> 		return;
>> 	}
>> 	ext4_unlock_group(sb, grp);
>> @@ -659,9 +671,12 @@ static void ext4_put_super(struct super_block *sb)
>> 	if (sbi->s_journal) {
>> 		err = jbd2_journal_destroy(sbi->s_journal);
>> 		sbi->s_journal = NULL;
>> -		if (err < 0)
>> +		if (err < 0) {
>> +			unlock_super(sb);
>> 			ext4_abort(sb, __func__,
>> 				   "Couldn't clean up the journal");
>> +			lock_super(sb);
>> +		}
>> 	}
>> 
>> 	ext4_release_system_zone(sb);
>> @@ -758,6 +773,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
>> 	ei->i_da_metadata_calc_len = 0;
>> 	ei->i_delalloc_reserved_flag = 0;
>> 	spin_lock_init(&(ei->i_block_reservation_lock));
>> +	spin_lock_init(&(ei->raw_inode_lock));
>> #ifdef CONFIG_QUOTA
>> 	ei->i_reserved_quota = 0;
>> #endif
>> @@ -1161,6 +1177,7 @@ enum {
>> 	Opt_inode_readahead_blks, Opt_journal_ioprio,
>> 	Opt_dioread_nolock, Opt_dioread_lock,
>> 	Opt_discard, Opt_nodiscard,
>> + 	Opt_noinode_sanity, Opt_ignore_crc,
>> };
>> 
>> static const match_table_t tokens = {
>> @@ -1231,6 +1248,8 @@ static const match_table_t tokens = {
>> 	{Opt_dioread_lock, "dioread_lock"},
>> 	{Opt_discard, "discard"},
>> 	{Opt_nodiscard, "nodiscard"},
>> + 	{Opt_noinode_sanity, "noinode_sanity"},
>> + 	{Opt_ignore_crc, "ignore_crc"},
>> 	{Opt_err, NULL},
>> };
>> 
>> @@ -1408,6 +1427,12 @@ static int parse_options(char *options, struct super_block *sb,
>> 		case Opt_orlov:
>> 			clear_opt(sbi->s_mount_opt, OLDALLOC);
>> 			break;
>> +		case Opt_noinode_sanity:
>> +			clear_opt(sbi->s_mount_opt, INODE_SANITY);
>> +			break;
>> +		case Opt_ignore_crc:
>> +			set_opt(sbi->s_mount_opt, IGNORE_CRC);
>> +			break;
>> #ifdef CONFIG_EXT4_FS_XATTR
>> 		case Opt_user_xattr:
>> 			set_opt(sbi->s_mount_opt, XATTR_USER);
>> @@ -1946,6 +1971,7 @@ static int ext4_check_descriptors(struct super_block *sb)
>> 	}
>> 
>> 	ext4_free_blocks_count_set(sbi->s_es, ext4_count_free_blocks(sb));
>> + 	ext4_super_update_crc(sb, sbi->s_es);
>> 	sbi->s_es->s_free_inodes_count =cpu_to_le32(ext4_count_free_inodes(sb));
>> 	return 1;
>> }
>> @@ -2431,6 +2457,50 @@ static int ext4_feature_set_ok(struct super_block *sb, int readonly)
>> 	return 1;
>> }
>> 
>> +/* could be removed for SBs once e2fsutils are fixed to always compute
>> +   the CRC when the feature is turned on. */
>> +#define ZERO_MAGIC 1
>> +
>> +/*
>> + * Manage CRC32 for the superblock.
>> + */
>> +static int ext4_super_check_crc(struct super_block *sb,
>> +				struct ext4_super_block *es)
>> +{
>> +	u32 crc, check;
>> +
>> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
>> +		return 0;
>> +	crc = le32_to_cpu(es->s_crc);
>> +	if (crc == 0 || test_opt(sb, IGNORE_CRC)) {
>> +		ext4_msg(sb, KERN_INFO, "super block checksum ignored");
>> +		return 0;
>> +	}
>> +	es->s_crc = 0;
>> +	check = crc32c(~0U, es, sizeof(struct ext4_super_block));
>> +	if (check == 0)
>> +		check = ZERO_MAGIC;
>> +	if (check != crc)
>> +		return -1;
>> +	/* Remove me */
>> +	ext4_msg(sb, KERN_INFO, "super block checksum passed");
>> +	return 0;
>> +}
>> +
>> +static void ext4_super_update_crc(struct super_block *sb,
>> +				  struct ext4_super_block *es)
>> +{
>> +	u32 crc;
>> +
>> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, EXT4_FEATURE_RO_COMPAT_SBI_CRC))
>> +		return;
>> +	es->s_crc = 0;
>> +        crc = crc32c(~0U, es, sizeof(struct ext4_super_block));
>> +	if (crc == 0)
>> +		crc = ZERO_MAGIC;
>> +	es->s_crc = cpu_to_le32(crc);
>> +}
>> +
>> static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> 				__releases(kernel_lock)
>> 				__acquires(kernel_lock)
>> @@ -2554,6 +2624,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> 	sbi->s_max_batch_time = EXT4_DEF_MAX_BATCH_TIME;
>> 
>> 	set_opt(sbi->s_mount_opt, BARRIER);
>> +	set_opt(sbi->s_mount_opt, INODE_SANITY);
>> 
>> 	/*
>> 	 * enable delayed allocation by default
>> @@ -2585,6 +2656,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> 	if (!ext4_feature_set_ok(sb, (sb->s_flags & MS_RDONLY)))
>> 		goto failed_mount;
>> 
>> +	if (ext4_super_check_crc(sb, es) < 0) {
>> +		ext4_msg(sb, KERN_ERR, "Invalid checksum in super block");
>> +		goto failed_mount;
>> +	}
>> +
>> 	blocksize = BLOCK_SIZE << le32_to_cpu(es->s_log_block_size);
>> 
>> 	if (blocksize < EXT4_MIN_BLOCK_SIZE ||
>> @@ -2675,6 +2751,7 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> 
>> 	for (i = 0; i < 4; i++)
>> 		sbi->s_hash_seed[i] = le32_to_cpu(es->s_hash_seed[i]);
>> +	memcpy(sbi->s_uuid, es->s_uuid, 16);
>> 	sbi->s_def_hash_version = es->s_def_hash_version;
>> 	i = le32_to_cpu(es->s_flags);
>> 	if (i & EXT2_FLAGS_UNSIGNED_HASH)
>> @@ -3393,6 +3470,9 @@ static int ext4_commit_super(struct super_block *sb, int sync)
>> 	es->s_free_inodes_count = cpu_to_le32(percpu_counter_sum_positive(
>> 					&EXT4_SB(sb)->s_freeinodes_counter));
>> 	sb->s_dirt = 0;
>> +	/* can be removed if this is properly journaled, see
>> +         * http://bugzilla.kernel.org/show_bug.cgi?id=14354 */
>> +	ext4_super_update_crc(sb, es);
>> 	BUFFER_TRACE(sbh, "marking dirty");
>> 	mark_buffer_dirty(sbh);
>> 	if (sync) {
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index 0433800..fbba95a 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -1171,6 +1171,7 @@ retry:
>> 
>> 	free = ext4_xattr_free_space(last, &min_offs, base, &total_ino);
>> 	if (free >= new_extra_isize) {
>> +		spin_lock(&EXT4_I(inode)->raw_inode_lock);
>> 		entry = IFIRST(header);
>> 		ext4_xattr_shift_entries(entry,	EXT4_I(inode)->i_extra_isize
>> 				- new_extra_isize, (void *)raw_inode +
>> @@ -1179,6 +1180,7 @@ retry:
>> 				inode->i_sb->s_blocksize);
>> 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
>> 		error = 0;
>> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>> 		goto cleanup;
>> 	}
>> 
>> @@ -1301,6 +1303,7 @@ retry:
>> 		if (error)
>> 			goto cleanup;
>> 
>> +		spin_lock(&EXT4_I(inode)->raw_inode_lock);
>> 		entry = IFIRST(header);
>> 		if (entry_size + EXT4_XATTR_SIZE(size) >= new_extra_isize)
>> 			shift_bytes = new_extra_isize;
>> @@ -1312,6 +1315,7 @@ retry:
>> 			EXT4_GOOD_OLD_INODE_SIZE + extra_isize + shift_bytes,
>> 			(void *)header, total_ino - entry_size,
>> 			inode->i_sb->s_blocksize);
>> +		spin_unlock(&EXT4_I(inode)->raw_inode_lock);
>> 
>> 		extra_isize += shift_bytes;
>> 		new_extra_isize -= shift_bytes;
>> 
>> 
>> -- 
>> ak@linux.intel.com -- Speaking for myself only


Cheers, Andreas