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