Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal

Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal

[Darrick J. Wong]

[cc linux-ext4]

On Fri, Sep 12, 2014 at 10:09:55AM -0400, TR Reardon wrote:
> Note that this only works (zeroes out) when removing inode journal.  Removing
> an existing journal_dev leaves s_jnl_blocks untouched.  To be absolutely
> clean, perhaps it should be wiped in all removal cases?

s_jnl_blocks shouldn't be set if an external journal is in use.

(Unless it is somehow?)

--D

> 
> --- Original Message ---
> 
> From: "Theodore Ts'o" <tytso@mit.edu>
> Sent: September 11, 2014 12:44 PM
> To: "Darrick J. Wong" <darrick.wong@oracle.com>
> Cc: linux-ext4@vger.kernel.org, "TR Reardon" <thomas_reardon@hotmail.com>
> Subject: Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal
> 
> On Mon, Sep 08, 2014 at 04:12:35PM -0700, Darrick J. Wong wrote:
> > When we're removing the internal journal (broken journal, turning it
> > off, or adding an external journal), zero s_jnl_blocks so that they
> > can't be picked up by accident later.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Cc: TR Reardon <thomas_reardon@hotmail.com>
> 
> Applied, thanks.
> 
>                                         - 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

RE: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal

[TR Reardon]

If the change from journal_inode to journal_dev is/was made prior to the recent change, the s_jnl_blocks will have whatever it last had.  So if it started with journal_inode and then switched to journal_dev (again, prior to your fix) or no journal, the s_jnl_blocks will have the old journal_inode info, and adding/removing journal_dev does no clear it out.  For a new fs with created only with journal_dev, there is no issue.  I'm just arguing that *adding* journal_dev should also zero this out.

+Reardon


----------------------------------------
> Date: Fri, 12 Sep 2014 09:43:42 -0700
> From: darrick.wong@oracle.com
> To: thomas_reardon@hotmail.com
> CC: linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal
>
> [cc linux-ext4]
>
> On Fri, Sep 12, 2014 at 10:09:55AM -0400, TR Reardon wrote:
>> Note that this only works (zeroes out) when removing inode journal. Removing
>> an existing journal_dev leaves s_jnl_blocks untouched. To be absolutely
>> clean, perhaps it should be wiped in all removal cases?
>
> s_jnl_blocks shouldn't be set if an external journal is in use.
>
> (Unless it is somehow?)
>
> --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 09/25] misc: zero s_jnl_blocks when removing internal journal

[Darrick J. Wong]

On Fri, Sep 12, 2014 at 03:06:23PM -0400, TR Reardon wrote:
> If the change from journal_inode to journal_dev is/was made prior to the
> recent change, the s_jnl_blocks will have whatever it last had.  So if it
> started with journal_inode and then switched to journal_dev (again, prior to
> your fix) or no journal, the s_jnl_blocks will have the old journal_inode
> info, and adding/removing journal_dev does no clear it out.  For a new fs
> with created only with journal_dev, there is no issue.  I'm just arguing that
> *adding* journal_dev should also zero this out.

You would also want to zero out s_jnl_blocks if creating a journal on a mounted
FS, since there's no way to find the block map/ETB blocks; the best we can do
is hope the user runs e2fsck, which will fix it.

--D
> 
> +Reardon
> 
> 
> ----------------------------------------
> > Date: Fri, 12 Sep 2014 09:43:42 -0700
> > From: darrick.wong@oracle.com
> > To: thomas_reardon@hotmail.com
> > CC: linux-ext4@vger.kernel.org
> > Subject: Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal
> >
> > [cc linux-ext4]
> >
> > On Fri, Sep 12, 2014 at 10:09:55AM -0400, TR Reardon wrote:
> >> Note that this only works (zeroes out) when removing inode journal. Removing
> >> an existing journal_dev leaves s_jnl_blocks untouched. To be absolutely
> >> clean, perhaps it should be wiped in all removal cases?
> >
> > s_jnl_blocks shouldn't be set if an external journal is in use.
> >
> > (Unless it is somehow?)
> >
> > --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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/25] e2fsprogs Summer 2014 patchbomb, part 5.2

[Darrick J. Wong]

Hi all,

This is the third release of part 5^H4 of the Summer 2014 e2fsprogs
patchset.  Whereas all the patches I've sent previously were to fix
problems in the library and e2fsck uncovered by the e2fuzz metadata
fuzzer, we're now to the part of the patch set that includes new
features.  I'm only posting things that have been discussed recently;
the other "new" features (most of which have been out for review for
quite some time) haven't changed since May.  They'll be in part 6,
which I'm holding until we get through this pile of patches.

Patch 1 introduces journal_csum v3 to fix numerous journal block tag
size handling bugs when metadata_csum+journal_checksum are turned on.
The test of 64bitness should not rely on guessing the tag size when it
could simply query the feature flags, since it was guessing
incorrectly.  Furthermore, the journal_csum v2 structure had memory
access alignment issues.  Just replace this all with a 16-byte tag
with everything in it; the overhead for checksums is no more than
0.1%.

Patches 2-4 fix some problems with incorrect error codes being
returned and a place where error codes could be dropped.

Patches 5-6 enable us to format the ext4 superblock of an external
journal device with metadata_csum enabled, so that the ext4 superblock
can be protected with a checksum.  Note that this is entirely separate
from journal_checksum, which only applies to the journal blocks on the
external device.  See section 11.2 of the ext4 disk layout page for
details about external journal devices.

Patches 7-8 fix a few small problems when dealing with external
journal devices, namely that dumpe2fs failed to print external journal
features, and that tune2fs shouldn't print 'cannot find superblock'
when passed an external journal device.

Patch 9 zeroes the superblock's j_blocks field when discarding an
internal journal.

Patches 10-12 enhance debugfs with the ability to replay journals with
dirty data and more importantly to create journal transactions; this
feature gives us the ability to (easily) create journal replay
scenarios for testing.  #11 fixes a minor bug in the e2fsck journal
management code that could in theory lead to journal corruption.

Patches 13-22 add 'make check' tests that exercise (1) the new
transaction writing ability and e2fsck' ability to replay; (2)
recovering with journal_csum v1-3; and (3) the ability to replay
journals where different kinds of journal blocks fail checksum
verification.  The test journals created in these tests have been run
through kernel jbd2 to verify that they recover properly.  There
are now tests to verify the writability and replayability when
an external journal is used.

NOTE: The test "j_corrupt_journal_block" in patch 21 ensures that
e2fsck will replay everything but the corrupt block, and then proceeds
with the fsck to fix up whatever might be broken.  You can decompress
the image.gz and try to mount it to verify that it's unmountable (and
hence requires e2fsck to be run).

Patches 23-25 implement v2 of the e2fsck readahead functionality,
which promises to reduce fsck runtime by 10-30%.  You might want to
read the report: http://marc.info/?l=linux-ext4&m=140755433701165&w=2
("e2fsck readahead speedup performance report") for all the juicy
details!

I've tested these e2fsprogs changes against the -next branch as of
8/29.  The patches have been tested against the 'make check' suite and
some amount of e2fuzz testing.

Comments and questions are, as always, welcome.

--D

[PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal

[Darrick J. Wong]

When we're removing the internal journal (broken journal, turning it
off, or adding an external journal), zero s_jnl_blocks so that they
can't be picked up by accident later.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Cc: TR Reardon <thomas_reardon@hotmail.com>
---
 e2fsck/journal.c       |    1 +
 lib/ext2fs/mkjournal.c |    1 +
 misc/tune2fs.c         |    1 +
 3 files changed, 3 insertions(+)


diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index 16bd757..d12e317 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -549,6 +549,7 @@ static errcode_t e2fsck_journal_fix_bad_inode(e2fsck_t ctx,
 				       "filesystem is now ext2 only ***\n\n");
 			sb->s_feature_compat &= ~EXT3_FEATURE_COMPAT_HAS_JOURNAL;
 			sb->s_journal_inum = 0;
+			memset(sb->s_jnl_blocks, 0, sizeof(sb->s_jnl_blocks));
 			ctx->flags |= E2F_FLAG_JOURNAL_INODE;
 			ctx->fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
 			e2fsck_clear_recover(ctx, 1);
diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
index 85f86bf..0a7cd18 100644
--- a/lib/ext2fs/mkjournal.c
+++ b/lib/ext2fs/mkjournal.c
@@ -490,6 +490,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 	fs->super->s_journal_dev = st.st_rdev;
 	memcpy(fs->super->s_journal_uuid, jsb->s_uuid,
 	       sizeof(fs->super->s_journal_uuid));
+	memset(fs->super->s_jnl_blocks, 0, sizeof(fs->super->s_jnl_blocks));
 	fs->super->s_feature_compat |= EXT3_FEATURE_COMPAT_HAS_JOURNAL;
 	ext2fs_mark_super_dirty(fs);
 	return 0;
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index c22c8fd..7292ab1 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -377,6 +377,7 @@ static errcode_t remove_journal_inode(ext2_filsys fs)
 		return retval;
 	}
 	fs->super->s_journal_inum = 0;
+	memset(fs->super->s_jnl_blocks, 0, sizeof(fs->super->s_jnl_blocks));
 	ext2fs_mark_super_dirty(fs);
 
 	return 0;

Re: [PATCH 09/25] misc: zero s_jnl_blocks when removing internal journal

[Theodore Ts'o]

On Mon, Sep 08, 2014 at 04:12:35PM -0700, Darrick J. Wong wrote:
> When we're removing the internal journal (broken journal, turning it
> off, or adding an external journal), zero s_jnl_blocks so that they
> can't be picked up by accident later.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Cc: TR Reardon <thomas_reardon@hotmail.com>

Applied, thanks.

					- Ted