Re: [PATCH] ext4/jbd2: Fix jbd2_journal_destory() for umount path

[Theodore Ts'o <tytso@mit.edu>]

On Thu, Feb 25, 2016 at 08:28:27PM +0900, OGAWA Hirofumi wrote:
> On umount path, jbd2_journal_destroy() writes latest transaction ID
> (->j_tail_sequence) to be used at next mount.
> 
> The bug is that ->j_tail_sequence is not holding latest transaction ID
> in some cases. So, at next mount, there is chance to conflict with
> remaining (not overwritten yet) transactions.
> 
> 	mount (id=10)
> 	write transaction (id=11)
> 	write transaction (id=12)
> 	umount (id=10) <= the bug doesn't write latest ID
> 
> 	mount (id=10)
> 	write transaction (id=11)
> 	crash
> 
> 	recovery
> 	transaction (id=11)
> 	transaction (id=12) <= valid transaction ID, but old commit
> 	                       must not replay
> 
> Like above, this bug become the cause of recovery failure, or FS
> corruption.
> 
> So why ->j_tail_sequence doesn't point latest ID?
> 
> Because if checkpoint transactions was reclaimed by memory pressure
> (i.e. bdev_try_to_free_page()), then ->j_tail_sequence is not updated.
> (And another case is, __jbd2_journal_clean_checkpoint_list() is called
> with empty transaction.)
> 
> So in above cases, ->j_tail_sequence is not pointing latest
> transaction ID at umount path. Plus, REQ_FLUSH for checkpoint is not
> done too.
> 
> So, to fix this problem with minimum changes, this patch updates
> ->j_tail_sequence, and issue REQ_FLUSH.  (With more complex changes,
> some optimizations would be possible to avoid unnecessary REQ_FLUSH
> for example though.)
> 
> BTW,
> 
> 	journal->j_tail_sequence =
> 		++journal->j_transaction_sequence;
> 
> Increment of ->j_transaction_sequence seems to be unnecessary, but
> ext3 does this.
> 
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Thanks, applied.

					- Ted