| Message ID | 20191203092756.26129-3-yi.zhang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ext4, jbd2: improve aborting progress | expand |
On Tue 03-12-19 17:27:54, zhangyi (F) wrote: > JBD2_REC_ERR flag used to indicate the errno has been updated when jbd2 > aborted, and then __ext4_abort() and ext4_handle_error() can invoke > panic if ERRORS_PANIC is specified. But if the journal has been aborted > with zero errno, jbd2_journal_abort() didn't set this flag so we can > no longer panic. Fix this by rename JBD2_REC_ERR to JBD2_ABORT_DONE and > set such flag even if there is no need to record errno in the jbd2 super > block. > > Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock") > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> OK, makes sense. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Although I'd note that after your patch 1, there is only a single place that will call jbd2_journal_abort() with 0 errno - namely one place in fs/jbd2/checkpoint.c and I think it would make sense for that call site to just pass -EIO and we can completely drop the checks whether errno != 0. Honza > --- > fs/ext4/super.c | 4 ++-- > fs/jbd2/journal.c | 10 +++++----- > include/linux/jbd2.h | 3 ++- > 3 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index dd654e53ba3d..25b0c883bd15 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -482,7 +482,7 @@ static void ext4_handle_error(struct super_block *sb) > sb->s_flags |= SB_RDONLY; > } else if (test_opt(sb, ERRORS_PANIC)) { > if (EXT4_SB(sb)->s_journal && > - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) > + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) > return; > panic("EXT4-fs (device %s): panic forced after error\n", > sb->s_id); > @@ -701,7 +701,7 @@ void __ext4_abort(struct super_block *sb, const char *function, > } > if (test_opt(sb, ERRORS_PANIC) && !system_going_down()) { > if (EXT4_SB(sb)->s_journal && > - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) > + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) > return; > panic("EXT4-fs panic from previous error\n"); > } > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c > index 1c58859aa592..a78b07841080 100644 > --- a/fs/jbd2/journal.c > +++ b/fs/jbd2/journal.c > @@ -2118,12 +2118,12 @@ static void __journal_abort_soft (journal_t *journal, int errno) > > __jbd2_journal_abort_hard(journal); > > - if (errno) { > + if (errno) > jbd2_journal_update_sb_errno(journal); > - write_lock(&journal->j_state_lock); > - journal->j_flags |= JBD2_REC_ERR; > - write_unlock(&journal->j_state_lock); > - } > + > + write_lock(&journal->j_state_lock); > + journal->j_flags |= JBD2_ABORT_DONE; > + write_unlock(&journal->j_state_lock); > } > > /** > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 603fbc4e2f70..71cab887fb98 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -1248,7 +1248,8 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3) > #define JBD2_ABORT_ON_SYNCDATA_ERR 0x040 /* Abort the journal on file > * data write error in ordered > * mode */ > -#define JBD2_REC_ERR 0x080 /* The errno in the sb has been recorded */ > +#define JBD2_ABORT_DONE 0x080 /* Abort done, the errno in the sb has been > + * recorded if necessary */ > > /* > * Function declarations for the journaling transaction and buffer > -- > 2.17.2 >
On 2019/12/3 20:10, Jan Kara wrote: > On Tue 03-12-19 17:27:54, zhangyi (F) wrote: >> JBD2_REC_ERR flag used to indicate the errno has been updated when jbd2 >> aborted, and then __ext4_abort() and ext4_handle_error() can invoke >> panic if ERRORS_PANIC is specified. But if the journal has been aborted >> with zero errno, jbd2_journal_abort() didn't set this flag so we can >> no longer panic. Fix this by rename JBD2_REC_ERR to JBD2_ABORT_DONE and >> set such flag even if there is no need to record errno in the jbd2 super >> block. >> >> Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock") >> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> > > OK, makes sense. You can add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Although I'd note that after your patch 1, there is only a single place > that will call jbd2_journal_abort() with 0 errno - namely one place in > fs/jbd2/checkpoint.c and I think it would make sense for that call site to > just pass -EIO and we can completely drop the checks whether errno != 0. > Thanks for review. I think a zero errno designed for the case that no further changes to the journal, and the journal on the disk is consistent and can recover well, so we don't want to record the errno and mark the filesystem error. But now it looks that we don't have such strong cases (shutdown is an exception) and pass none-zero errno is also OK for every jbd2_journal_abort(). Thanks, Yi. >> --- >> fs/ext4/super.c | 4 ++-- >> fs/jbd2/journal.c | 10 +++++----- >> include/linux/jbd2.h | 3 ++- >> 3 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index dd654e53ba3d..25b0c883bd15 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -482,7 +482,7 @@ static void ext4_handle_error(struct super_block *sb) >> sb->s_flags |= SB_RDONLY; >> } else if (test_opt(sb, ERRORS_PANIC)) { >> if (EXT4_SB(sb)->s_journal && >> - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) >> + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) >> return; >> panic("EXT4-fs (device %s): panic forced after error\n", >> sb->s_id); >> @@ -701,7 +701,7 @@ void __ext4_abort(struct super_block *sb, const char *function, >> } >> if (test_opt(sb, ERRORS_PANIC) && !system_going_down()) { >> if (EXT4_SB(sb)->s_journal && >> - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) >> + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) >> return; >> panic("EXT4-fs panic from previous error\n"); >> } >> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c >> index 1c58859aa592..a78b07841080 100644 >> --- a/fs/jbd2/journal.c >> +++ b/fs/jbd2/journal.c >> @@ -2118,12 +2118,12 @@ static void __journal_abort_soft (journal_t *journal, int errno) >> >> __jbd2_journal_abort_hard(journal); >> >> - if (errno) { >> + if (errno) >> jbd2_journal_update_sb_errno(journal); >> - write_lock(&journal->j_state_lock); >> - journal->j_flags |= JBD2_REC_ERR; >> - write_unlock(&journal->j_state_lock); >> - } >> + >> + write_lock(&journal->j_state_lock); >> + journal->j_flags |= JBD2_ABORT_DONE; >> + write_unlock(&journal->j_state_lock); >> } >> >> /** >> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h >> index 603fbc4e2f70..71cab887fb98 100644 >> --- a/include/linux/jbd2.h >> +++ b/include/linux/jbd2.h >> @@ -1248,7 +1248,8 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3) >> #define JBD2_ABORT_ON_SYNCDATA_ERR 0x040 /* Abort the journal on file >> * data write error in ordered >> * mode */ >> -#define JBD2_REC_ERR 0x080 /* The errno in the sb has been recorded */ >> +#define JBD2_ABORT_DONE 0x080 /* Abort done, the errno in the sb has been >> + * recorded if necessary */ >> >> /* >> * Function declarations for the journaling transaction and buffer >> -- >> 2.17.2 >>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index dd654e53ba3d..25b0c883bd15 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -482,7 +482,7 @@ static void ext4_handle_error(struct super_block *sb) sb->s_flags |= SB_RDONLY; } else if (test_opt(sb, ERRORS_PANIC)) { if (EXT4_SB(sb)->s_journal && - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) return; panic("EXT4-fs (device %s): panic forced after error\n", sb->s_id); @@ -701,7 +701,7 @@ void __ext4_abort(struct super_block *sb, const char *function, } if (test_opt(sb, ERRORS_PANIC) && !system_going_down()) { if (EXT4_SB(sb)->s_journal && - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) + !(EXT4_SB(sb)->s_journal->j_flags & JBD2_ABORT_DONE)) return; panic("EXT4-fs panic from previous error\n"); } diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c index 1c58859aa592..a78b07841080 100644 --- a/fs/jbd2/journal.c +++ b/fs/jbd2/journal.c @@ -2118,12 +2118,12 @@ static void __journal_abort_soft (journal_t *journal, int errno) __jbd2_journal_abort_hard(journal); - if (errno) { + if (errno) jbd2_journal_update_sb_errno(journal); - write_lock(&journal->j_state_lock); - journal->j_flags |= JBD2_REC_ERR; - write_unlock(&journal->j_state_lock); - } + + write_lock(&journal->j_state_lock); + journal->j_flags |= JBD2_ABORT_DONE; + write_unlock(&journal->j_state_lock); } /** diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 603fbc4e2f70..71cab887fb98 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -1248,7 +1248,8 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3) #define JBD2_ABORT_ON_SYNCDATA_ERR 0x040 /* Abort the journal on file * data write error in ordered * mode */ -#define JBD2_REC_ERR 0x080 /* The errno in the sb has been recorded */ +#define JBD2_ABORT_DONE 0x080 /* Abort done, the errno in the sb has been + * recorded if necessary */ /* * Function declarations for the journaling transaction and buffer
JBD2_REC_ERR flag used to indicate the errno has been updated when jbd2 aborted, and then __ext4_abort() and ext4_handle_error() can invoke panic if ERRORS_PANIC is specified. But if the journal has been aborted with zero errno, jbd2_journal_abort() didn't set this flag so we can no longer panic. Fix this by rename JBD2_REC_ERR to JBD2_ABORT_DONE and set such flag even if there is no need to record errno in the jbd2 super block. Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock") Signed-off-by: zhangyi (F) <yi.zhang@huawei.com> --- fs/ext4/super.c | 4 ++-- fs/jbd2/journal.c | 10 +++++----- include/linux/jbd2.h | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-)