Re: journal recovery problems with metadata_csum, *non-64bit*

Re: journal recovery problems with metadata_csum, *non-64bit*

[Darrick J. Wong]

On Sun, Aug 10, 2014 at 06:35:33PM -0400, TR Reardon wrote:
> Ok, I found the problem in jbd2, and have a solution, though it's
> debatable what the ideal solution is.  For now, the simplest patch is
> below, though a similar patch in lib/ext2fs/kernel-jbd.h is required
> to get e2fsck back in sync.
> 
> The original c3900875 commit adding metadata_csum (ie
> journal_checksum_v2) to jbd2 added 2 extra bytes for the block
> checksums, in addition to re-allocating 2 bytes from the 4 bytes of
> flags.  However, a decision was made to only retain the lower 16-bits
> of the crc32c, and thus those extra 2 bytes were unneeded.  But those
> 2 extra bytes were never "deallocated" from journal_tag_bytes().

Hrmm... yes, I remember trying to push for full 32-bit checksums on journal
blocks, and our subsequent decision not to put in the two bytes.  Oops.

(This looks more like a coding error on my part.)

I suppose it would help to be able to use debugfs or something to create
journal transactions just to see if they'll replay correctly in the
kernel/e2fsck.  I've wondered for a while if the e2fsck jbd code ought to be
pushed into libext2fs (or libjbd2) to make this easier.  A long ago fuse2fs
patchbomb actually did this so that fuse2fs could at least replay the journal.

> Unfortunately, different code relies on JBD_TAG_SIZE32/64 constants
> directly rather than the journal_tag_bytes() utility function, in
> particular the recovery code which is common to e2fsck and jbd2.  This
> led different tools to think they were looking at a 64bit journal when
> actually it was 32bit.  Code that relied on journal_tag_bytes()
> remained safe, so the block iterators were fine, but any direct use of
> those constants [including the hideous greater-than comparison in
> read_tag_bytes()] went awry, and journal replay will fail.

Hmm... I thought recovery.c sets tag_bytes to journal_tag_bytes()?

(It's late, I'll have another look in the morning.)

> As far as I can tell, metadata_csum + journal checksum has never
> worked for 32bit filesystems. By a little bit of padding luck, 64bit
> worked fine.
`
D'oh. :(

FWIW, 64bit is recommended for metadata_csum since it enables full 32-bit
bitmap checksums.

> Now, as to the solution: depends on whether one feels that existing
> in-the-wild journals matter. The original commit was May 2012, are we
> past early-adopters now?  If this patch is taken, you shrink the

Definitely not past the early adopter stage.  The e2fsprogs code will be in
1.43, which means that most people can't use metadata_csum yet.

So, thank you very much for helping us to smoke test. :)

> journal block tags to the intended size but in-the-wild journals will
> be broken.  But they already are, so...?  This opens up the
> possibility of now using those extra 2 bytes and retaining full 32-bit
> crc32c for the block tags.  If going that route, debugs/logdump needs
> a fix in addition to changes to jbd2.

In theory the only wild journals should be on test FSes anyway, so breaking
them isn't the end of the world.

But as you point out, the space is already getting used because journal_csum_v2
is the gate for the extra 2 bytes to be turned on, so I guess we could just use
the extra 2 bytes and store the full checksum.

> FWIW, the "JBD2: Out of memory during recovery." error in
> fs/jbd2/recovery.c was opaque at best and should be changed to always
> include the block# that caused the problem.

recovery.c line 611, correct?

--D
> 
> +Reardon
> 
> ---
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 67b8e30..dc27d09 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2166,15 +2166,11 @@ int jbd2_journal_blocks_per_page(struct inode *inode)
>  size_t journal_tag_bytes(journal_t *journal)
>  {
>         journal_block_tag_t tag;
> -       size_t x = 0;
> -
> -       if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_CSUM_V2))
> -               x += sizeof(tag.t_checksum);
> 
>         if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_64BIT))
> -               return x + JBD2_TAG_SIZE64;
> +               return JBD2_TAG_SIZE64;
>         else
> -               return x + JBD2_TAG_SIZE32;
> +               return JBD2_TAG_SIZE32;
>  }
> 
>  /*

journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[Darrick J. Wong]

Hi all,

Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the the
"journal csum v2" feature flag is turned on, block tag records are being
written with two extra bytes of space because we don't need to execute
"x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
then perform incorrect size comparisons, leading to BUG() being called.  In
64-bit mode, there's enough padding that bad things won't happen.

This is a remnant of the days when I tried to enlarge journal_block_tag_t to
hold the full 32-bit checksum for a data block that's stored in the journal.
Back in 2011, we decided (though sadly I can't find the link; I think we might
have discussed this in the concall) that it was better not to change the size
of journal_block_tag_t than it was to make it bigger so that it could hold the
full checksum.

A simple fix for the problem has been proposed by Mr. Reardon which fixes
journal_tag_bytes() and leaves everything else unchanged.  However, that is
technically a disk format change since the size of journal_block_tag_t on disk
changes, albeit only for people running experimental metadata_csum filesystems.
Since we've been allocating disk space for the enlarged checksum this whole
time, if we apply that patch, anyone with an unclean 64bit FS will not be able
to recover the journal.  (Anyone with an unclean 32-bit FS has been broken the
whole time, and still will be.)

The other thing we could do is actually use the two bytes to store the high
16-bits of the checksum, fix the jbd2 helper functions to reflect that reality
(so that they don't BUG()), and change the checksum verify routine to pass the
block if either the full checksum matches, or if the lower 16 bits match and
the upper 16 bits are zero.  With this route, anybody with an uncleanly
unmounted FS could still recover the journal, since we're not changing the size
of anything.

Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
using metadata_csum on a production filesystem, so at this point we can
probably change the disk format without too many repercussions.  If you make
sure to cleanly unmount the filesystem when changing kernel/e2fsprogs versions,
there will be no problems.

So, uh... comments?  How should we proceed?

--D

On Mon, Aug 11, 2014 at 12:10:25AM -0700, Darrick J. Wong wrote:
> On Sun, Aug 10, 2014 at 06:35:33PM -0400, TR Reardon wrote:
> > Ok, I found the problem in jbd2, and have a solution, though it's
> > debatable what the ideal solution is.  For now, the simplest patch is
> > below, though a similar patch in lib/ext2fs/kernel-jbd.h is required
> > to get e2fsck back in sync.
> > 
> > The original c3900875 commit adding metadata_csum (ie
> > journal_checksum_v2) to jbd2 added 2 extra bytes for the block
> > checksums, in addition to re-allocating 2 bytes from the 4 bytes of
> > flags.  However, a decision was made to only retain the lower 16-bits
> > of the crc32c, and thus those extra 2 bytes were unneeded.  But those
> > 2 extra bytes were never "deallocated" from journal_tag_bytes().
> 
> Hrmm... yes, I remember trying to push for full 32-bit checksums on journal
> blocks, and our subsequent decision not to put in the two bytes.  Oops.
> 
> (This looks more like a coding error on my part.)
> 
> I suppose it would help to be able to use debugfs or something to create
> journal transactions just to see if they'll replay correctly in the
> kernel/e2fsck.  I've wondered for a while if the e2fsck jbd code ought to be
> pushed into libext2fs (or libjbd2) to make this easier.  A long ago fuse2fs
> patchbomb actually did this so that fuse2fs could at least replay the journal.
> 
> > Unfortunately, different code relies on JBD_TAG_SIZE32/64 constants
> > directly rather than the journal_tag_bytes() utility function, in
> > particular the recovery code which is common to e2fsck and jbd2.  This
> > led different tools to think they were looking at a 64bit journal when
> > actually it was 32bit.  Code that relied on journal_tag_bytes()
> > remained safe, so the block iterators were fine, but any direct use of
> > those constants [including the hideous greater-than comparison in
> > read_tag_bytes()] went awry, and journal replay will fail.
> 
> Hmm... I thought recovery.c sets tag_bytes to journal_tag_bytes()?
> 
> (It's late, I'll have another look in the morning.)
> 
> > As far as I can tell, metadata_csum + journal checksum has never
> > worked for 32bit filesystems. By a little bit of padding luck, 64bit
> > worked fine.
> `
> D'oh. :(
> 
> FWIW, 64bit is recommended for metadata_csum since it enables full 32-bit
> bitmap checksums.
> 
> > Now, as to the solution: depends on whether one feels that existing
> > in-the-wild journals matter. The original commit was May 2012, are we
> > past early-adopters now?  If this patch is taken, you shrink the
> 
> Definitely not past the early adopter stage.  The e2fsprogs code will be in
> 1.43, which means that most people can't use metadata_csum yet.
> 
> So, thank you very much for helping us to smoke test. :)
> 
> > journal block tags to the intended size but in-the-wild journals will
> > be broken.  But they already are, so...?  This opens up the
> > possibility of now using those extra 2 bytes and retaining full 32-bit
> > crc32c for the block tags.  If going that route, debugs/logdump needs
> > a fix in addition to changes to jbd2.
> 
> In theory the only wild journals should be on test FSes anyway, so breaking
> them isn't the end of the world.
> 
> But as you point out, the space is already getting used because journal_csum_v2
> is the gate for the extra 2 bytes to be turned on, so I guess we could just use
> the extra 2 bytes and store the full checksum.
> 
> > FWIW, the "JBD2: Out of memory during recovery." error in
> > fs/jbd2/recovery.c was opaque at best and should be changed to always
> > include the block# that caused the problem.
> 
> recovery.c line 611, correct?
> 
> --D
> > 
> > +Reardon
> > 
> > ---
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 67b8e30..dc27d09 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -2166,15 +2166,11 @@ int jbd2_journal_blocks_per_page(struct inode *inode)
> >  size_t journal_tag_bytes(journal_t *journal)
> >  {
> >         journal_block_tag_t tag;
> > -       size_t x = 0;
> > -
> > -       if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_CSUM_V2))
> > -               x += sizeof(tag.t_checksum);
> > 
> >         if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_64BIT))
> > -               return x + JBD2_TAG_SIZE64;
> > +               return JBD2_TAG_SIZE64;
> >         else
> > -               return x + JBD2_TAG_SIZE32;
> > +               return JBD2_TAG_SIZE32;
> >  }
> > 
> >  /*
> --
> 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: journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[TR Reardon]

On 8/11/14, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> Hi all,
>
> Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the
> the
> "journal csum v2" feature flag is turned on, block tag records are being
> written with two extra bytes of space because we don't need to execute
> "x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
> then perform incorrect size comparisons, leading to BUG() being called.  In
> 64-bit mode, there's enough padding that bad things won't happen.
>
> This is a remnant of the days when I tried to enlarge journal_block_tag_t
> to
> hold the full 32-bit checksum for a data block that's stored in the
> journal.
> Back in 2011, we decided (though sadly I can't find the link; I think we
> might
> have discussed this in the concall) that it was better not to change the
> size
> of journal_block_tag_t than it was to make it bigger so that it could hold
> the
> full checksum.
>
> A simple fix for the problem has been proposed by Mr. Reardon which fixes
> journal_tag_bytes() and leaves everything else unchanged.  However, that is
> technically a disk format change since the size of journal_block_tag_t on
> disk
> changes, albeit only for people running experimental metadata_csum
> filesystems.
> Since we've been allocating disk space for the enlarged checksum this whole
> time, if we apply that patch, anyone with an unclean 64bit FS will not be
> able
> to recover the journal.  (Anyone with an unclean 32-bit FS has been broken
> the
> whole time, and still will be.)
>
> The other thing we could do is actually use the two bytes to store the high
> 16-bits of the checksum, fix the jbd2 helper functions to reflect that
> reality
> (so that they don't BUG()), and change the checksum verify routine to pass
> the
> block if either the full checksum matches, or if the lower 16 bits match
> and
> the upper 16 bits are zero.  With this route, anybody with an uncleanly
> unmounted FS could still recover the journal, since we're not changing the
> size
> of anything.
>
> Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
> using metadata_csum on a production filesystem, so at this point we can
> probably change the disk format without too many repercussions.  If you
> make
> sure to cleanly unmount the filesystem when changing kernel/e2fsprogs
> versions,
> there will be no problems.
>
> So, uh... comments?  How should we proceed?
>
> --D

My only concern is that legacy applies to in-the-wild kernels, not
just journals.  Any post 3.4 kernel has this problem, which will be
exposed as soon as e2fsprogs 1.43 is released.  A common (enough) use
case might be, say, a 2TB USB drive, being unplugged and replugged
across machines  without proper shutdown.  Old machines will think
they recognize the journal but don't actually, and replay something
destructive.

In other words, this is a future-retro problem.  The problem is not so
much with existing fs experimenting with metadata_csum, but a future
properly-journaled drive being plugged into an legacy faulty machine.
Those incompat flags are supposed to protect against just this
scenario, but won't.  So perhaps you'll need make a new
"INCOMPAT_CSUM_V3" flag to protect?  Actually, dwelling on this a bit
today, I think whether or not you retain those 2 extra checksum bytes
in final fix, you ought use a new flag for the format.

+Reardon

Re: journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[Darrick J. Wong]

On Tue, Aug 12, 2014 at 01:39:35AM -0400, TR Reardon wrote:
> On 8/11/14, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > Hi all,
> >
> > Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the
> > the
> > "journal csum v2" feature flag is turned on, block tag records are being
> > written with two extra bytes of space because we don't need to execute
> > "x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
> > then perform incorrect size comparisons, leading to BUG() being called.  In
> > 64-bit mode, there's enough padding that bad things won't happen.
> >
> > This is a remnant of the days when I tried to enlarge journal_block_tag_t
> > to
> > hold the full 32-bit checksum for a data block that's stored in the
> > journal.
> > Back in 2011, we decided (though sadly I can't find the link; I think we
> > might
> > have discussed this in the concall) that it was better not to change the
> > size
> > of journal_block_tag_t than it was to make it bigger so that it could hold
> > the
> > full checksum.
> >
> > A simple fix for the problem has been proposed by Mr. Reardon which fixes
> > journal_tag_bytes() and leaves everything else unchanged.  However, that is
> > technically a disk format change since the size of journal_block_tag_t on
> > disk
> > changes, albeit only for people running experimental metadata_csum
> > filesystems.
> > Since we've been allocating disk space for the enlarged checksum this whole
> > time, if we apply that patch, anyone with an unclean 64bit FS will not be
> > able
> > to recover the journal.  (Anyone with an unclean 32-bit FS has been broken
> > the
> > whole time, and still will be.)
> >
> > The other thing we could do is actually use the two bytes to store the high
> > 16-bits of the checksum, fix the jbd2 helper functions to reflect that
> > reality
> > (so that they don't BUG()), and change the checksum verify routine to pass
> > the
> > block if either the full checksum matches, or if the lower 16 bits match
> > and
> > the upper 16 bits are zero.  With this route, anybody with an uncleanly
> > unmounted FS could still recover the journal, since we're not changing the
> > size
> > of anything.
> >
> > Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
> > using metadata_csum on a production filesystem, so at this point we can
> > probably change the disk format without too many repercussions.  If you
> > make
> > sure to cleanly unmount the filesystem when changing kernel/e2fsprogs
> > versions,
> > there will be no problems.
> >
> > So, uh... comments?  How should we proceed?
> >
> > --D
> 
> My only concern is that legacy applies to in-the-wild kernels, not
> just journals.  Any post 3.4 kernel has this problem, which will be
> exposed as soon as e2fsprogs 1.43 is released.  A common (enough) use
> case might be, say, a 2TB USB drive, being unplugged and replugged
> across machines  without proper shutdown.  Old machines will think
> they recognize the journal but don't actually, and replay something
> destructive.
> 
> In other words, this is a future-retro problem.  The problem is not so
> much with existing fs experimenting with metadata_csum, but a future
> properly-journaled drive being plugged into an legacy faulty machine.
> Those incompat flags are supposed to protect against just this
> scenario, but won't.  So perhaps you'll need make a new
> "INCOMPAT_CSUM_V3" flag to protect?  Actually, dwelling on this a bit
> today, I think whether or not you retain those 2 extra checksum bytes
> in final fix, you ought use a new flag for the format.

Definitely we need a new bit to keep the old kernels out; I wanted others to
weigh in on whether we ought to shrink the struct or put the two bytes to use.

That said... journal recovery is not so well tested, so I'm currently busy
building out debugfs to create journal transactions so we can test recovery
both with and without *_csum.

--D
> 
> +Reardon
> --
> 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: journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[Andreas Dilger]

Doesn't a larger journal_tag_bytes() size mean more overhead for
the journal?

Cheers, Andreas

> On Aug 12, 2014, at 19:08, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> 
>> On Tue, Aug 12, 2014 at 01:39:35AM -0400, TR Reardon wrote:
>>> On 8/11/14, Darrick J. Wong <darrick.wong@oracle.com> wrote:
>>> Hi all,
>>> 
>>> Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the
>>> the
>>> "journal csum v2" feature flag is turned on, block tag records are being
>>> written with two extra bytes of space because we don't need to execute
>>> "x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
>>> then perform incorrect size comparisons, leading to BUG() being called.  In
>>> 64-bit mode, there's enough padding that bad things won't happen.
>>> 
>>> This is a remnant of the days when I tried to enlarge journal_block_tag_t
>>> to
>>> hold the full 32-bit checksum for a data block that's stored in the
>>> journal.
>>> Back in 2011, we decided (though sadly I can't find the link; I think we
>>> might
>>> have discussed this in the concall) that it was better not to change the
>>> size
>>> of journal_block_tag_t than it was to make it bigger so that it could hold
>>> the
>>> full checksum.
>>> 
>>> A simple fix for the problem has been proposed by Mr. Reardon which fixes
>>> journal_tag_bytes() and leaves everything else unchanged.  However, that is
>>> technically a disk format change since the size of journal_block_tag_t on
>>> disk
>>> changes, albeit only for people running experimental metadata_csum
>>> filesystems.
>>> Since we've been allocating disk space for the enlarged checksum this whole
>>> time, if we apply that patch, anyone with an unclean 64bit FS will not be
>>> able
>>> to recover the journal.  (Anyone with an unclean 32-bit FS has been broken
>>> the
>>> whole time, and still will be.)
>>> 
>>> The other thing we could do is actually use the two bytes to store the high
>>> 16-bits of the checksum, fix the jbd2 helper functions to reflect that
>>> reality
>>> (so that they don't BUG()), and change the checksum verify routine to pass
>>> the
>>> block if either the full checksum matches, or if the lower 16 bits match
>>> and
>>> the upper 16 bits are zero.  With this route, anybody with an uncleanly
>>> unmounted FS could still recover the journal, since we're not changing the
>>> size
>>> of anything.
>>> 
>>> Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
>>> using metadata_csum on a production filesystem, so at this point we can
>>> probably change the disk format without too many repercussions.  If you
>>> make
>>> sure to cleanly unmount the filesystem when changing kernel/e2fsprogs
>>> versions,
>>> there will be no problems.
>>> 
>>> So, uh... comments?  How should we proceed?
>>> 
>>> --D
>> 
>> My only concern is that legacy applies to in-the-wild kernels, not
>> just journals.  Any post 3.4 kernel has this problem, which will be
>> exposed as soon as e2fsprogs 1.43 is released.  A common (enough) use
>> case might be, say, a 2TB USB drive, being unplugged and replugged
>> across machines  without proper shutdown.  Old machines will think
>> they recognize the journal but don't actually, and replay something
>> destructive.
>> 
>> In other words, this is a future-retro problem.  The problem is not so
>> much with existing fs experimenting with metadata_csum, but a future
>> properly-journaled drive being plugged into an legacy faulty machine.
>> Those incompat flags are supposed to protect against just this
>> scenario, but won't.  So perhaps you'll need make a new
>> "INCOMPAT_CSUM_V3" flag to protect?  Actually, dwelling on this a bit
>> today, I think whether or not you retain those 2 extra checksum bytes
>> in final fix, you ought use a new flag for the format.
> 
> Definitely we need a new bit to keep the old kernels out; I wanted others to
> weigh in on whether we ought to shrink the struct or put the two bytes to use.
> 
> That said... journal recovery is not so well tested, so I'm currently busy
> building out debugfs to create journal transactions so we can test recovery
> both with and without *_csum.
> 
> --D
>> 
>> +Reardon
>> --
>> 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

Re: journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[Darrick J. Wong]

On Wed, Aug 13, 2014 at 11:35:50PM +0200, Andreas Dilger wrote:
> Doesn't a larger journal_tag_bytes() size mean more overhead for
> the journal?

Yes.  If we move to a 16-byte block tag:

typedef struct journal_block_tag3_s
{
	__u32		t_blocknr;	/* The on-disk block number */
	__u32		t_flags;	/* See below */
	__u32		t_blocknr_high; /* most-significant high 32bits. */
	__u32		t_checksum;	/* crc32c(uuid+seq+block) */
} journal_block_tag3_t;

Notice that this fixes the alignment problem due to the v2 bug -- tag 2 starts
with a 4-byte field at offset 14.

For a 32-bit FS with 4K blocks and a 32768 block journal, I found through
experimentation that if I fill the journal with nothing but journal blocks
(i.e. no revoke blocks), overhead increases by 264 blocks, or 0.8%.  For a
64-bit FS with the same parameters, the increase is 136 blocks, or 0.4%.

I'm hoping that a <1% increase won't discourage people. :)

--D
> 
> Cheers, Andreas
> 
> > On Aug 12, 2014, at 19:08, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> > 
> >> On Tue, Aug 12, 2014 at 01:39:35AM -0400, TR Reardon wrote:
> >>> On 8/11/14, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >>> Hi all,
> >>> 
> >>> Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the
> >>> the
> >>> "journal csum v2" feature flag is turned on, block tag records are being
> >>> written with two extra bytes of space because we don't need to execute
> >>> "x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
> >>> then perform incorrect size comparisons, leading to BUG() being called.  In
> >>> 64-bit mode, there's enough padding that bad things won't happen.
> >>> 
> >>> This is a remnant of the days when I tried to enlarge journal_block_tag_t
> >>> to
> >>> hold the full 32-bit checksum for a data block that's stored in the
> >>> journal.
> >>> Back in 2011, we decided (though sadly I can't find the link; I think we
> >>> might
> >>> have discussed this in the concall) that it was better not to change the
> >>> size
> >>> of journal_block_tag_t than it was to make it bigger so that it could hold
> >>> the
> >>> full checksum.
> >>> 
> >>> A simple fix for the problem has been proposed by Mr. Reardon which fixes
> >>> journal_tag_bytes() and leaves everything else unchanged.  However, that is
> >>> technically a disk format change since the size of journal_block_tag_t on
> >>> disk
> >>> changes, albeit only for people running experimental metadata_csum
> >>> filesystems.
> >>> Since we've been allocating disk space for the enlarged checksum this whole
> >>> time, if we apply that patch, anyone with an unclean 64bit FS will not be
> >>> able
> >>> to recover the journal.  (Anyone with an unclean 32-bit FS has been broken
> >>> the
> >>> whole time, and still will be.)
> >>> 
> >>> The other thing we could do is actually use the two bytes to store the high
> >>> 16-bits of the checksum, fix the jbd2 helper functions to reflect that
> >>> reality
> >>> (so that they don't BUG()), and change the checksum verify routine to pass
> >>> the
> >>> block if either the full checksum matches, or if the lower 16 bits match
> >>> and
> >>> the upper 16 bits are zero.  With this route, anybody with an uncleanly
> >>> unmounted FS could still recover the journal, since we're not changing the
> >>> size
> >>> of anything.
> >>> 
> >>> Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
> >>> using metadata_csum on a production filesystem, so at this point we can
> >>> probably change the disk format without too many repercussions.  If you
> >>> make
> >>> sure to cleanly unmount the filesystem when changing kernel/e2fsprogs
> >>> versions,
> >>> there will be no problems.
> >>> 
> >>> So, uh... comments?  How should we proceed?
> >>> 
> >>> --D
> >> 
> >> My only concern is that legacy applies to in-the-wild kernels, not
> >> just journals.  Any post 3.4 kernel has this problem, which will be
> >> exposed as soon as e2fsprogs 1.43 is released.  A common (enough) use
> >> case might be, say, a 2TB USB drive, being unplugged and replugged
> >> across machines  without proper shutdown.  Old machines will think
> >> they recognize the journal but don't actually, and replay something
> >> destructive.
> >> 
> >> In other words, this is a future-retro problem.  The problem is not so
> >> much with existing fs experimenting with metadata_csum, but a future
> >> properly-journaled drive being plugged into an legacy faulty machine.
> >> Those incompat flags are supposed to protect against just this
> >> scenario, but won't.  So perhaps you'll need make a new
> >> "INCOMPAT_CSUM_V3" flag to protect?  Actually, dwelling on this a bit
> >> today, I think whether or not you retain those 2 extra checksum bytes
> >> in final fix, you ought use a new flag for the format.
> > 
> > Definitely we need a new bit to keep the old kernels out; I wanted others to
> > weigh in on whether we ought to shrink the struct or put the two bytes to use.
> > 
> > That said... journal recovery is not so well tested, so I'm currently busy
> > building out debugfs to create journal transactions so we can test recovery
> > both with and without *_csum.
> > 
> > --D
> >> 
> >> +Reardon
> >> --
> >> 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

Re: journal_checksum_v2 on-disk format change? (was: Re: journal recovery problems with metadata_csum, *non-64bit*)

[Darrick J. Wong]

On Wed, Aug 13, 2014 at 03:53:58PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 13, 2014 at 11:35:50PM +0200, Andreas Dilger wrote:
> > Doesn't a larger journal_tag_bytes() size mean more overhead for
> > the journal?
> 
> Yes.  If we move to a 16-byte block tag:
> 
> typedef struct journal_block_tag3_s
> {
> 	__u32		t_blocknr;	/* The on-disk block number */
> 	__u32		t_flags;	/* See below */
> 	__u32		t_blocknr_high; /* most-significant high 32bits. */
> 	__u32		t_checksum;	/* crc32c(uuid+seq+block) */
> } journal_block_tag3_t;
> 
> Notice that this fixes the alignment problem due to the v2 bug -- tag 2 starts
> with a 4-byte field at offset 14.
> 
> For a 32-bit FS with 4K blocks and a 32768 block journal, I found through
> experimentation that if I fill the journal with nothing but journal blocks
> (i.e. no revoke blocks), overhead increases by 264 blocks, or 0.8%.  For a
> 64-bit FS with the same parameters, the increase is 136 blocks, or 0.4%.
> 
> I'm hoping that a <1% increase won't discourage people. :)

Nuts, those figures are for 1k block filesystems.  For a FS with 4k blocks,
storing 16384 blocks in the journal required this many blocks:

16450 for 64bit journal_csum_v3
16450 for 32bit journal_csum_v3 (assuming I reuse the tag3 structure as-is)

16442 for 64bit journal_csum_v2
16426 for 32bit journal_csum_v2

16434 for 64bit journal_csum_v1
16418 for 32bit journal_csum_v1

16434 for 64bit none
16418 for 32bit none

So for 32-bit FSes, the extra overhead to go from no journal checksumming at
all to v3 checksums is 0.2%.  For 64-bit FSes, the extra overhead is 0.1%.

The overhead to move a 32-bit v2 FS to a v3 FS is 0.1%, and for a 64-bit FS
it's 0.05%.

Also, for those watching at home, I fixed the infinite loop on journal block
checksum failure that someone complained about months ago.

--D

> 
> --D
> > 
> > Cheers, Andreas
> > 
> > > On Aug 12, 2014, at 19:08, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> > > 
> > >> On Tue, Aug 12, 2014 at 01:39:35AM -0400, TR Reardon wrote:
> > >>> On 8/11/14, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >>> Hi all,
> > >>> 
> > >>> Mr. Reardon has discovered that due to a bug in journal_tag_bytes(), if the
> > >>> the
> > >>> "journal csum v2" feature flag is turned on, block tag records are being
> > >>> written with two extra bytes of space because we don't need to execute
> > >>> "x += sizeof(tag.t_checksum);".  In 32-bit mode, other parts of the journal
> > >>> then perform incorrect size comparisons, leading to BUG() being called.  In
> > >>> 64-bit mode, there's enough padding that bad things won't happen.
> > >>> 
> > >>> This is a remnant of the days when I tried to enlarge journal_block_tag_t
> > >>> to
> > >>> hold the full 32-bit checksum for a data block that's stored in the
> > >>> journal.
> > >>> Back in 2011, we decided (though sadly I can't find the link; I think we
> > >>> might
> > >>> have discussed this in the concall) that it was better not to change the
> > >>> size
> > >>> of journal_block_tag_t than it was to make it bigger so that it could hold
> > >>> the
> > >>> full checksum.
> > >>> 
> > >>> A simple fix for the problem has been proposed by Mr. Reardon which fixes
> > >>> journal_tag_bytes() and leaves everything else unchanged.  However, that is
> > >>> technically a disk format change since the size of journal_block_tag_t on
> > >>> disk
> > >>> changes, albeit only for people running experimental metadata_csum
> > >>> filesystems.
> > >>> Since we've been allocating disk space for the enlarged checksum this whole
> > >>> time, if we apply that patch, anyone with an unclean 64bit FS will not be
> > >>> able
> > >>> to recover the journal.  (Anyone with an unclean 32-bit FS has been broken
> > >>> the
> > >>> whole time, and still will be.)
> > >>> 
> > >>> The other thing we could do is actually use the two bytes to store the high
> > >>> 16-bits of the checksum, fix the jbd2 helper functions to reflect that
> > >>> reality
> > >>> (so that they don't BUG()), and change the checksum verify routine to pass
> > >>> the
> > >>> block if either the full checksum matches, or if the lower 16 bits match
> > >>> and
> > >>> the upper 16 bits are zero.  With this route, anybody with an uncleanly
> > >>> unmounted FS could still recover the journal, since we're not changing the
> > >>> size
> > >>> of anything.
> > >>> 
> > >>> Fortunately, journal tag blocks are fairly ephemeral and nobody ought to be
> > >>> using metadata_csum on a production filesystem, so at this point we can
> > >>> probably change the disk format without too many repercussions.  If you
> > >>> make
> > >>> sure to cleanly unmount the filesystem when changing kernel/e2fsprogs
> > >>> versions,
> > >>> there will be no problems.
> > >>> 
> > >>> So, uh... comments?  How should we proceed?
> > >>> 
> > >>> --D
> > >> 
> > >> My only concern is that legacy applies to in-the-wild kernels, not
> > >> just journals.  Any post 3.4 kernel has this problem, which will be
> > >> exposed as soon as e2fsprogs 1.43 is released.  A common (enough) use
> > >> case might be, say, a 2TB USB drive, being unplugged and replugged
> > >> across machines  without proper shutdown.  Old machines will think
> > >> they recognize the journal but don't actually, and replay something
> > >> destructive.
> > >> 
> > >> In other words, this is a future-retro problem.  The problem is not so
> > >> much with existing fs experimenting with metadata_csum, but a future
> > >> properly-journaled drive being plugged into an legacy faulty machine.
> > >> Those incompat flags are supposed to protect against just this
> > >> scenario, but won't.  So perhaps you'll need make a new
> > >> "INCOMPAT_CSUM_V3" flag to protect?  Actually, dwelling on this a bit
> > >> today, I think whether or not you retain those 2 extra checksum bytes
> > >> in final fix, you ought use a new flag for the format.
> > > 
> > > Definitely we need a new bit to keep the old kernels out; I wanted others to
> > > weigh in on whether we ought to shrink the struct or put the two bytes to use.
> > > 
> > > That said... journal recovery is not so well tested, so I'm currently busy
> > > building out debugfs to create journal transactions so we can test recovery
> > > both with and without *_csum.
> > > 
> > > --D
> > >> 
> > >> +Reardon
> > >> --
> > >> 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
> --
> 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