On Mon, Nov 12, 2012 at 03:44:11AM -0500, George Spelvin wrote: > The following patch is marked "Cc: [email protected]", > but I do not see it in any -stable release as of 3.6.6. > > Ted, Greg, can you two please figure out where it got dropped?
Well, it was cc'ed to [email protected]. It may be because you needed to apply the hunk you've identified, but I don't recall Greg sending me a note saying this patch was dropped from consideration for the stable series. On the other hand, between travel and my getting distracted by the Phoronix-inspired hysteria, I may have missed something.... Greg, did I miss a patch rejected notice from you? - Ted > > It's pretty significant if you're using metadata_csum, so > it should probably go in. It's already in Linus' v3.7-rc3. > (06db49e68ae70cf16819b85a14057acb2820776a). > > You can also add my Tested-by: if youlike. > > It still applies cleanly to 3.6.6, but needs one more call site > fixed up to compile: > > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c > index 71241bc..101b41c 100644 > --- a/fs/ext4/resize.c > +++ b/fs/ext4/resize.c > @@ -979,7 +979,7 @@ static void update_backups(struct super_block *sb, > goto exit_err; > } > > - ext4_superblock_csum_set(sb, (struct ext4_super_block *)data); > + ext4_superblock_csum_set(sb); > > while ((group = ext4_list_backups(sb, &three, &five, &seven)) < last) { > struct buffer_head *bh; > > I'm not sure if you should apply the above hunk, or just cherry-pick > commit bef53b01faeb791e27605cba1a71ba21364cb23e from mainline which > removed the call entirely. > > > From [email protected] Mon Oct 08 02:41:29 2012 > Date: Sun, 7 Oct 2012 22:41:26 -0400 > From: Theodore Ts'o <[email protected]> > To: George Spelvin <[email protected]> > Cc: [email protected], [email protected] > Subject: Re: metadata_csum + unclean shutdown = failure to boot > References: <[email protected]> > <[email protected]> > MIME-Version: 1.0 > Content-Type: text/plain; charset=us-ascii > Content-Disposition: inline > In-Reply-To: <[email protected]> > User-Agent: Mutt/1.5.21 (2010-09-15) > X-SA-Exim-Connect-IP: <locally generated> > X-SA-Exim-Mail-From: [email protected] > X-SA-Exim-Scanned: No (on imap.thunk.org); SAEximRunCond expanded to false > > I found the problem. It turns out ext4_handle_dirty_super() was > completely FUBAR'ed and was calculating the checksum on the wrong data > (for all but 1k block file systems, sigh). > > We just didn't notice because the checksum would be correctly set when > the file system was unmounted cleanly. (Sigh). > > The following patch should fix things. Thanks for testing out the > metadata checksum on the root file system, and reporting this > problem!!! > > - Ted > > >From bdd7ed290bf12c2e9132fbe97208a1af79c7a29d Mon Sep 17 00:00:00 2001 > From: Theodore Ts'o <[email protected]> > Date: Sun, 7 Oct 2012 22:18:56 -0400 > Subject: [PATCH] ext4: fix metadata checksum calculation for the superblock > > The function ext4_handle_dirty_super() was calculating the superblock > on the wrong block data. As a result, when the superblock is modified > while it is mounted (most commonly, when inodes are added or removed > from the orphan list), the superblock checksum would be wrong. We > didn't notice because the superblock *was* being correctly calculated > in ext4_commit_super(), and this would get called when the file system > was unmounted. So the problem only became obvious if the system > crashed while the file system was mounted. > > Fix this by removing the poorly designed function signature for > ext4_superblock_Csum_set(); if it only took a single argument, the > pointer to a struct superblock, the ambiguity which caused this > mistake would have been impossible. > > Reported-by: George Spelvin <[email protected]> > Signed-off-by: "Theodore Ts'o" <[email protected]> > Cc: [email protected] > --- > fs/ext4/ext4.h | 3 +-- > fs/ext4/ext4_jbd2.c | 8 ++------ > fs/ext4/super.c | 7 ++++--- > 3 files changed, 7 insertions(+), 11 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 3ab2539..78971cf 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -2063,8 +2063,7 @@ extern int ext4_resize_fs(struct super_block *sb, > ext4_fsblk_t n_blocks_count); > extern int ext4_calculate_overhead(struct super_block *sb); > extern int ext4_superblock_csum_verify(struct super_block *sb, > struct ext4_super_block *es); > -extern void ext4_superblock_csum_set(struct super_block *sb, > - struct ext4_super_block *es); > +extern void ext4_superblock_csum_set(struct super_block *sb); > extern void *ext4_kvmalloc(size_t size, gfp_t flags); > extern void *ext4_kvzalloc(size_t size, gfp_t flags); > extern void ext4_kvfree(void *ptr); > diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c > index bfa65b4..b4323ba 100644 > --- a/fs/ext4/ext4_jbd2.c > +++ b/fs/ext4/ext4_jbd2.c > @@ -143,17 +143,13 @@ int __ext4_handle_dirty_super(const char *where, > unsigned int line, > struct buffer_head *bh = EXT4_SB(sb)->s_sbh; > int err = 0; > > + ext4_superblock_csum_set(sb); > if (ext4_handle_valid(handle)) { > - ext4_superblock_csum_set(sb, > - (struct ext4_super_block *)bh->b_data); > err = jbd2_journal_dirty_metadata(handle, bh); > if (err) > ext4_journal_abort_handle(where, line, __func__, > bh, handle, err); > - } else { > - ext4_superblock_csum_set(sb, > - (struct ext4_super_block *)bh->b_data); > + } else > mark_buffer_dirty(bh); > - } > return err; > } > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 982f6fc..5ededf1 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -143,9 +143,10 @@ int ext4_superblock_csum_verify(struct super_block *sb, > return es->s_checksum == ext4_superblock_csum(sb, es); > } > > -void ext4_superblock_csum_set(struct super_block *sb, > - struct ext4_super_block *es) > +void ext4_superblock_csum_set(struct super_block *sb) > { > + struct ext4_super_block *es = EXT4_SB(sb)->s_es; > + > if (!EXT4_HAS_RO_COMPAT_FEATURE(sb, > EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) > return; > @@ -4387,7 +4388,7 @@ static int ext4_commit_super(struct super_block *sb, > int sync) > cpu_to_le32(percpu_counter_sum_positive( > &EXT4_SB(sb)->s_freeinodes_counter)); > BUFFER_TRACE(sbh, "marking dirty"); > - ext4_superblock_csum_set(sb, es); > + ext4_superblock_csum_set(sb); > mark_buffer_dirty(sbh); > if (sync) { > error = sync_dirty_buffer(sbh); > -- > 1.7.12.rc0.22.gcdd159b > > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
