On Tue, Jan 14, 2020 at 11:31:19AM +0100, Jan Kara wrote: > Thanks for the patch! Just some small comments below: > > On Sat 11-01-20 10:25:42, Kai Li wrote: > > Fixes: 85e0c4e89c1b "jbd2: if the journal is aborted then don't allow update of the log tail" > > This tag should come at the bottom of the changelog (close to your > Signed-off-by). > > > If journal is dirty when mount, it will be replayed but jbd2 sb > > log tail cannot be updated to mark a new start because > > journal->j_flags has already been set with JBD2_ABORT first > > in journal_init_common. > > When a new transaction is committed, it will be recorded in block 1 > > first(journal->j_tail is set to 1 in journal_reset). If emergency > > restart again before journal super block is updated unfortunately, > > the new recorded trans will not be replayed in the next mount. > > It is danerous which may lead to metadata corruption for file system. > > I'd slightly rephrase the text here so that it is more easily readable and > correct some grammar mistakes. Something like: > > If the journal is dirty when the filesystem is mounted, jbd2 will replay > the journal but the journal superblock will not be updated by > journal_reset() because JBD2_ABORT flag is still set (it was set in > journal_init_common()). This is problematic because when a new transaction > is then committed, it will be recorded in block 1 (journal->j_tail was set > to 1 in journal_reset()). If unclean shutdown happens again before the > journal superblock is updated, the new recorded transaction will not be > replayed during the next mount (because of stale sb->s_start and > sb->s_sequence values) which can lead to filesystem corruption. > > Otherwise the patch looks good to me so feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> > > (again this is added to the bottom of the changelog like the 'Fixes' tag or > 'Signed-off-by' tag). Thanks, applied with a fixed up commit description. - Ted
