Hi,
This series originally aim to fix ext4_handle_error() and ext4_abort()
cannot panic because of we invoke __jbd2_journal_abort_hard() when we
failed to submit commit record without setting JBD2_REC_ERR flag.
I add patch 1 and patch 4 to switch to use jbd2_journal_abort() and do
some cleanup job at this iteration as Jan suggested. I also add patch 3
to partially revert commit 818d276ceb8 "ext4: Add the journal checksum
feature" because it seems unnecessary, but I am not quite sure. please
revirew this series and give some suggestions.
Thanks,
Yi.
zhangyi (F) (4):
jbd2: switch to use jbd2_journal_abort() when failed to submit the
commit record
ext4, jbd2: ensure panic when journal aborting with zero errno
Partially revert "ext4: pass -ESHUTDOWN code to jbd2 layer"
jbd2: clean __jbd2_journal_abort_hard() and __journal_abort_soft()
fs/ext4/ioctl.c | 4 +-
fs/ext4/super.c | 4 +-
fs/jbd2/commit.c | 4 +-
fs/jbd2/journal.c | 108 +++++++++++++++----------------------------
include/linux/jbd2.h | 4 +-
5 files changed, 45 insertions(+), 79 deletions(-)
--
2.17.2
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) <[email protected]>
---
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 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) <[email protected]>
OK, makes sense. You can add:
Reviewed-by: Jan Kara <[email protected]>
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
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
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) <[email protected]>
>
> OK, makes sense. You can add:
>
> Reviewed-by: Jan Kara <[email protected]>
>
> 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
>>