diff mbox

[03/22] ext4: Record the checksum algorithm in use in the superblock

Message ID 20111128232636.19194.92115.stgit@elm3c44.beaverton.ibm.com
State Superseded, archived
Headers show

Commit Message

Darrick J. Wong Nov. 28, 2011, 11:26 p.m. UTC 
Record the type of checksum algorithm we're using for metadata in the
superblock, in case we ever want/need to change the algorithm.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---
 fs/ext4/ext4.h  |    5 ++++-
 fs/ext4/super.c |   18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletions(-)



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

Comments

Theodore Ts'o Dec. 5, 2011, 3:46 p.m. UTC | #1 
On Mon, Nov 28, 2011 at 03:26:36PM -0800, Darrick J. Wong wrote:
> Record the type of checksum algorithm we're using for metadata in the
> superblock, in case we ever want/need to change the algorithm.
> 
> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

In general, it's useful to group changes to the on-disk file system
separately from the other patches, so this should be grouped with the
addition of s_csum_seed (aka c_uuid_csum) in patch #2, and all other
patches that add metadata changes.  That way we can see easily what
all of the metadata changes are, at the beginning of the patch, and
make sure that what we have in the e2fsprogs patch set matches what we
have in the ext4 patch set.

Also, the change to actually *enable* a file system feature should be
at the very end of the file system.  This makes life safer if at some
point in the future we need to do a kernel bisect.  If we are
advertising the present of some feature such in /sys/fs/ext4/features,
that should also go at the very end of the patch set (again for
obvious reasons), and it can be grouped with the patch which enables
the feature being defined as being enabled in EXT4_FEATURE_*_SUPP.
(This is really a comment about patch #2 in this series, but I forgot
to mention it in my previous e-mail.)

A similar set of guidelines apply to patches for e2fsprogs as well.

  	      	 	    	     	     - Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Dec. 5, 2011, 7:32 p.m. UTC | #2 
On Mon, Dec 05, 2011 at 10:46:55AM -0500, Ted Ts'o wrote:
> On Mon, Nov 28, 2011 at 03:26:36PM -0800, Darrick J. Wong wrote:
> > Record the type of checksum algorithm we're using for metadata in the
> > superblock, in case we ever want/need to change the algorithm.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> 
> In general, it's useful to group changes to the on-disk file system
> separately from the other patches, so this should be grouped with the
> addition of s_csum_seed (aka c_uuid_csum) in patch #2, and all other
> patches that add metadata changes.  That way we can see easily what
> all of the metadata changes are, at the beginning of the patch, and
> make sure that what we have in the e2fsprogs patch set matches what we
> have in the ext4 patch set.

Ok, I'll reorganize the patches to put all the disk format changes into a
separate patch at the beginning of the set.

As for e2fsprogs, I think you already committed most of the disk format
changes.  I'll check the e2fsprogs set.

> Also, the change to actually *enable* a file system feature should be
> at the very end of the file system.  This makes life safer if at some
> point in the future we need to do a kernel bisect.  If we are
> advertising the present of some feature such in /sys/fs/ext4/features,
> that should also go at the very end of the patch set (again for
> obvious reasons), and it can be grouped with the patch which enables
> the feature being defined as being enabled in EXT4_FEATURE_*_SUPP.
> (This is really a comment about patch #2 in this series, but I forgot
> to mention it in my previous e-mail.)
> 
> A similar set of guidelines apply to patches for e2fsprogs as well.

Okay, I'll move the *_SUPP changes into a separate patch at the end of the
sets.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong Dec. 7, 2011, 7:42 a.m. UTC | #3 
On Tue, Dec 06, 2011 at 10:01:35PM -0700, Andreas Dilger wrote:
> On 2011-11-28, at 4:26 PM, Darrick J. Wong wrote:
> > Record the type of checksum algorithm we're using for metadata in the
> > superblock, in case we ever want/need to change the algorithm.
> > 
> > @@ -982,6 +982,9 @@ extern void ext4_set_bits(void *bm, int cur, int len);
> > +/* Metadata checksum algorithm codes */
> > +#define EXT4_CRC32C_CHKSUM		1
> 
> It might make sense to add a constant for the existing CRC16 type:
> 
> #define EXT4_CRC16_CHKSUM    0
> #define EXT4_CRC32C_CHKSUM   1
> #define EXT4_CHKSUM_MAX      1

I disagree -- this field is only defined if metadata_csum is set, and most of
the checksumming code doesn't know what crc16 is.  If a user somehow set the
checksum type to "crc16", the code would keep using crc32c.  

If we ever extend ext4_chksum() to support other algorithms I suppose we could
make it smart enough to switch algorithms based on the field setting, but I
don't really want to do for crc16. :)

> > +static int ext4_verify_csum_type(struct super_block *sb,
> > +				 struct ext4_super_block *es)
> > +{
> > +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > +					EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > +		return 1;
> > +
> > +	return es->s_checksum_type == EXT4_CRC32C_CHKSUM;
> > +}
> 
> Then this checks "return es->s_checksum_type <= EXT4_CHKSUM_MAX;".

Not really -- this would allow us to mount a file system with metadata_csum=1
and csum_type=crc16, and crc16 is not supported by most of the checksumming
code.  I'd rather have the code reject the mount and send fs in for fsck, since
crc32c is the only one it knows about.

> > @@ -3182,6 +3192,14 @@ static int ext4_fill_super(struct super_block *sb, 
> > +	/* Check for a known checksum algorithm */
> > +	if (!ext4_verify_csum_type(sb, es)) {
> > +		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> > +			 "unknown checksum algorithm.");
> 
> It would be useful if this error message printed the checksum type.

Agreed.

--D
> 
> 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
Andreas Dilger Dec. 7, 2011, 8:40 a.m. UTC | #4 
On 2011-12-06, at 10:01 PM, Andreas Dilger wrote:
> On 2011-11-28, at 4:26 PM, Darrick J. Wong wrote:
>> Record the type of checksum algorithm we're using for metadata in the
>> superblock, in case we ever want/need to change the algorithm.
>> 
>> @@ -982,6 +982,9 @@ extern void ext4_set_bits(void *bm, int cur, int len);
>> +/* Metadata checksum algorithm codes */
>> +#define EXT4_CRC32C_CHKSUM		1
> 
> It might make sense to add a constant for the existing CRC16 type:
> 
> #define EXT4_CRC16_CHKSUM    0
> #define EXT4_CRC32C_CHKSUM   1
> #define EXT4_CHKSUM_MAX      1

Never mind, I had EXT4_FEATURE_INCOMPAT_BG_USE_META_CSUM on the brain.
CRC16 is completely unrelated to EXT4_FEATURE_RO_COMPAT_METADATA_CSUM.

>> +static int ext4_verify_csum_type(struct super_block *sb,
>> +				 struct ext4_super_block *es)
>> +{
>> +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
>> +					EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
>> +		return 1;
>> +
>> +	return es->s_checksum_type == EXT4_CRC32C_CHKSUM;
>> +}
> 
> Then this checks "return es->s_checksum_type <= EXT4_CHKSUM_MAX;".
> 
>> @@ -3182,6 +3192,14 @@ static int ext4_fill_super(struct super_block *sb, 
>> +	/* Check for a known checksum algorithm */
>> +	if (!ext4_verify_csum_type(sb, es)) {
>> +		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
>> +			 "unknown checksum algorithm.");
> 
> It would be useful if this error message printed the checksum type.

This is still a valid comment.

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
diff mbox

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 414c2f8..e496e72 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -982,6 +982,9 @@  extern void ext4_set_bits(void *bm, int cur, int len);
 #define EXT4_ERRORS_PANIC		3	/* Panic */
 #define EXT4_ERRORS_DEFAULT		EXT4_ERRORS_CONTINUE
 
+/* Metadata checksum algorithm codes */
+#define EXT4_CRC32C_CHKSUM		1
+
 /*
  * Structure of the super block
  */
@@ -1068,7 +1071,7 @@  struct ext4_super_block {
 	__le64  s_mmp_block;            /* Block for multi-mount protection */
 	__le32  s_raid_stripe_width;    /* blocks on all data disks (N*stride)*/
 	__u8	s_log_groups_per_flex;  /* FLEX_BG group size */
-	__u8	s_reserved_char_pad;
+	__u8	s_checksum_type;	/* metadata checksum algorithm used */
 	__le16  s_reserved_pad;
 	__le64	s_kbytes_written;	/* nr of lifetime kilobytes written */
 	__le32	s_snapshot_inum;	/* Inode number of active snapshot */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3858767..a56c14e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -111,6 +111,16 @@  static struct file_system_type ext3_fs_type = {
 #define IS_EXT3_SB(sb) (0)
 #endif
 
+static int ext4_verify_csum_type(struct super_block *sb,
+				 struct ext4_super_block *es)
+{
+	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
+					EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
+		return 1;
+
+	return es->s_checksum_type == EXT4_CRC32C_CHKSUM;
+}
+
 void *ext4_kvmalloc(size_t size, gfp_t flags)
 {
 	void *ret;
@@ -3182,6 +3192,14 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		goto cantfind_ext4;
 	sbi->s_kbytes_written = le64_to_cpu(es->s_kbytes_written);
 
+	/* Check for a known checksum algorithm */
+	if (!ext4_verify_csum_type(sb, es)) {
+		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
+			 "unknown checksum algorithm.");
+		silent = 1;
+		goto cantfind_ext4;
+	}
+
 	/* Set defaults before we parse the mount options */
 	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
 	set_opt(sb, INIT_INODE_TABLE);