[PATCH v2 0/4] ext4, jbd2: improve aborting progress

[PATCH v2 0/4] ext4, jbd2: improve aborting progress

[zhangyi (F)]

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

[PATCH v2 1/4] jbd2: switch to use jbd2_journal_abort() when failed to submit the commit record

[zhangyi (F)]

We invloke jbd2_journal_abort() to abort the journal and record errno
in the jbd2 superblock when committing journal transaction besides the
failure on submitting the commit record. But there is no need for the
case and we can also invloke jbd2_journal_abort() instead of
__jbd2_journal_abort_hard().

Fixes: 818d276ceb83a ("ext4: Add the journal checksum feature")
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/jbd2/commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 132fb92098c7..87b88d910306 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -785,7 +785,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		err = journal_submit_commit_record(journal, commit_transaction,
 						 &cbh, crc32_sum);
 		if (err)
-			__jbd2_journal_abort_hard(journal);
+			jbd2_journal_abort(journal, err);
 	}
 
 	blk_finish_plug(&plug);
@@ -876,7 +876,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		err = journal_submit_commit_record(journal, commit_transaction,
 						&cbh, crc32_sum);
 		if (err)
-			__jbd2_journal_abort_hard(journal);
+			jbd2_journal_abort(journal, err);
 	}
 	if (cbh)
 		err = journal_wait_on_commit_record(journal, cbh);
-- 
2.17.2

Re: [PATCH v2 1/4] jbd2: switch to use jbd2_journal_abort() when failed to submit the commit record

[Jan Kara]

On Tue 03-12-19 17:27:53, zhangyi (F) wrote:
> We invloke jbd2_journal_abort() to abort the journal and record errno
> in the jbd2 superblock when committing journal transaction besides the
> failure on submitting the commit record. But there is no need for the
> case and we can also invloke jbd2_journal_abort() instead of
			^^^ invoke

> __jbd2_journal_abort_hard().
> 
> Fixes: 818d276ceb83a ("ext4: Add the journal checksum feature")
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Besides the spelling fix the patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/jbd2/commit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 132fb92098c7..87b88d910306 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -785,7 +785,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		err = journal_submit_commit_record(journal, commit_transaction,
>  						 &cbh, crc32_sum);
>  		if (err)
> -			__jbd2_journal_abort_hard(journal);
> +			jbd2_journal_abort(journal, err);
>  	}
>  
>  	blk_finish_plug(&plug);
> @@ -876,7 +876,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  		err = journal_submit_commit_record(journal, commit_transaction,
>  						&cbh, crc32_sum);
>  		if (err)
> -			__jbd2_journal_abort_hard(journal);
> +			jbd2_journal_abort(journal, err);
>  	}
>  	if (cbh)
>  		err = journal_wait_on_commit_record(journal, cbh);
> -- 
> 2.17.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 2/4] ext4, jbd2: ensure panic when journal aborting with zero errno

[zhangyi (F)]

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(-)

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

Re: [PATCH v2 2/4] ext4, jbd2: ensure panic when journal aborting with zero errno

[Jan Kara]

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 2/4] ext4, jbd2: ensure panic when journal aborting with zero errno

[zhangyi (F)]

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
>>

[PATCH v2 3/4] Partially revert "ext4: pass -ESHUTDOWN code to jbd2 layer"

[zhangyi (F)]

This partially reverts commit fb7c02445c497943e7296cd3deee04422b63acb8.

Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want to
allow jbd2 layer to distinguish shutdown journal abort from other error
cases, but this patch seems unnecessary because we distinguished those
cases well through a zero errno parameter when shutting down, thus the
jbd2 aborting peocess will not record the errno. So partially reverts
this commit and keep the proper locking.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/ioctl.c   |  4 ++--
 fs/jbd2/journal.c | 22 +++++++---------------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 0b7f316fd30f..f99eeba5767d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -597,13 +597,13 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg)
 		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
 		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal)) {
 			(void) ext4_force_commit(sb);
-			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
+			jbd2_journal_abort(sbi->s_journal, 0);
 		}
 		break;
 	case EXT4_GOING_FLAGS_NOLOGFLUSH:
 		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
 		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal))
-			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
+			jbd2_journal_abort(sbi->s_journal, 0);
 		break;
 	default:
 		return -EINVAL;
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index a78b07841080..f3f9e0b994ef 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1475,14 +1475,11 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
 void jbd2_journal_update_sb_errno(journal_t *journal)
 {
 	journal_superblock_t *sb = journal->j_superblock;
-	int errcode;
 
 	lock_buffer(journal->j_sb_buffer);
-	errcode = journal->j_errno;
-	if (errcode == -ESHUTDOWN)
-		errcode = 0;
-	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
-	sb->s_errno    = cpu_to_be32(errcode);
+	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n",
+		  journal->j_errno);
+	sb->s_errno = cpu_to_be32(journal->j_errno);
 
 	jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA);
 }
@@ -2100,20 +2097,15 @@ void __jbd2_journal_abort_hard(journal_t *journal)
  * but don't do any other IO. */
 static void __journal_abort_soft (journal_t *journal, int errno)
 {
-	int old_errno;
-
 	write_lock(&journal->j_state_lock);
-	old_errno = journal->j_errno;
-	if (!journal->j_errno || errno == -ESHUTDOWN)
-		journal->j_errno = errno;
-
 	if (journal->j_flags & JBD2_ABORT) {
 		write_unlock(&journal->j_state_lock);
-		if (!old_errno && old_errno != -ESHUTDOWN &&
-		    errno == -ESHUTDOWN)
-			jbd2_journal_update_sb_errno(journal);
 		return;
 	}
+
+	if (!journal->j_errno)
+		journal->j_errno = errno;
+
 	write_unlock(&journal->j_state_lock);
 
 	__jbd2_journal_abort_hard(journal);
-- 
2.17.2

Re: [PATCH v2 3/4] Partially revert "ext4: pass -ESHUTDOWN code to jbd2 layer"

[Jan Kara]

On Tue 03-12-19 17:27:55, zhangyi (F) wrote:
> This partially reverts commit fb7c02445c497943e7296cd3deee04422b63acb8.
> 
> Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want to
> allow jbd2 layer to distinguish shutdown journal abort from other error
> cases, but this patch seems unnecessary because we distinguished those
> cases well through a zero errno parameter when shutting down, thus the
> jbd2 aborting peocess will not record the errno. So partially reverts
> this commit and keep the proper locking.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Ted has written this so he'll have the definitive answer but I think the idea
of ESHUTDOWN is that if the filesystem was shutdown, we want ESHUTDOWN to
be recorded in the journal sb and not some other error. That's why there's
logic in __journal_abort_soft() making sure that ESHUTDOWN takes precedence
over any other error. Because it can happen that after EXT4_FLAGS_SHUTDOWN
flag is set, some other place records different error in journal
superblock before we get to jbd2_journal_abort() and record ESHUTDOWN...

								Honza

> ---
>  fs/ext4/ioctl.c   |  4 ++--
>  fs/jbd2/journal.c | 22 +++++++---------------
>  2 files changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 0b7f316fd30f..f99eeba5767d 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -597,13 +597,13 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg)
>  		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
>  		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal)) {
>  			(void) ext4_force_commit(sb);
> -			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
> +			jbd2_journal_abort(sbi->s_journal, 0);
>  		}
>  		break;
>  	case EXT4_GOING_FLAGS_NOLOGFLUSH:
>  		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
>  		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal))
> -			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
> +			jbd2_journal_abort(sbi->s_journal, 0);
>  		break;
>  	default:
>  		return -EINVAL;
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index a78b07841080..f3f9e0b994ef 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1475,14 +1475,11 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
>  void jbd2_journal_update_sb_errno(journal_t *journal)
>  {
>  	journal_superblock_t *sb = journal->j_superblock;
> -	int errcode;
>  
>  	lock_buffer(journal->j_sb_buffer);
> -	errcode = journal->j_errno;
> -	if (errcode == -ESHUTDOWN)
> -		errcode = 0;
> -	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
> -	sb->s_errno    = cpu_to_be32(errcode);
> +	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n",
> +		  journal->j_errno);
> +	sb->s_errno = cpu_to_be32(journal->j_errno);
>  
>  	jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA);
>  }
> @@ -2100,20 +2097,15 @@ void __jbd2_journal_abort_hard(journal_t *journal)
>   * but don't do any other IO. */
>  static void __journal_abort_soft (journal_t *journal, int errno)
>  {
> -	int old_errno;
> -
>  	write_lock(&journal->j_state_lock);
> -	old_errno = journal->j_errno;
> -	if (!journal->j_errno || errno == -ESHUTDOWN)
> -		journal->j_errno = errno;
> -
>  	if (journal->j_flags & JBD2_ABORT) {
>  		write_unlock(&journal->j_state_lock);
> -		if (!old_errno && old_errno != -ESHUTDOWN &&
> -		    errno == -ESHUTDOWN)
> -			jbd2_journal_update_sb_errno(journal);
>  		return;
>  	}
> +
> +	if (!journal->j_errno)
> +		journal->j_errno = errno;
> +
>  	write_unlock(&journal->j_state_lock);
>  
>  	__jbd2_journal_abort_hard(journal);
> -- 
> 2.17.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 3/4] Partially revert "ext4: pass -ESHUTDOWN code to jbd2 layer"

[zhangyi (F)]

On 2019/12/3 20:23, Jan Kara wrote:
> On Tue 03-12-19 17:27:55, zhangyi (F) wrote:
>> This partially reverts commit fb7c02445c497943e7296cd3deee04422b63acb8.
>>
>> Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want to
>> allow jbd2 layer to distinguish shutdown journal abort from other error
>> cases, but this patch seems unnecessary because we distinguished those
>> cases well through a zero errno parameter when shutting down, thus the
>> jbd2 aborting peocess will not record the errno. So partially reverts
>> this commit and keep the proper locking.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> 
> Ted has written this so he'll have the definitive answer but I think the idea
> of ESHUTDOWN is that if the filesystem was shutdown, we want ESHUTDOWN to
> be recorded in the journal sb and not some other error. That's why there's
> logic in __journal_abort_soft() making sure that ESHUTDOWN takes precedence
> over any other error. Because it can happen that after EXT4_FLAGS_SHUTDOWN
> flag is set, some other place records different error in journal
> superblock before we get to jbd2_journal_abort() and record ESHUTDOWN...
> 

Thanks for the explanation, I understand now, we need to prevent record
errno in journal superblock and keep the filesystem just as it is after
EXT4_FLAGS_SHUTDOWN is set. So there is a bug now because it only
overwrite errno if it was 0, I will fix it together.

Thanks,
Yi

> 
>> ---
>>  fs/ext4/ioctl.c   |  4 ++--
>>  fs/jbd2/journal.c | 22 +++++++---------------
>>  2 files changed, 9 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>> index 0b7f316fd30f..f99eeba5767d 100644
>> --- a/fs/ext4/ioctl.c
>> +++ b/fs/ext4/ioctl.c
>> @@ -597,13 +597,13 @@ static int ext4_shutdown(struct super_block *sb, unsigned long arg)
>>  		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
>>  		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal)) {
>>  			(void) ext4_force_commit(sb);
>> -			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
>> +			jbd2_journal_abort(sbi->s_journal, 0);
>>  		}
>>  		break;
>>  	case EXT4_GOING_FLAGS_NOLOGFLUSH:
>>  		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
>>  		if (sbi->s_journal && !is_journal_aborted(sbi->s_journal))
>> -			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
>> +			jbd2_journal_abort(sbi->s_journal, 0);
>>  		break;
>>  	default:
>>  		return -EINVAL;
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index a78b07841080..f3f9e0b994ef 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -1475,14 +1475,11 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
>>  void jbd2_journal_update_sb_errno(journal_t *journal)
>>  {
>>  	journal_superblock_t *sb = journal->j_superblock;
>> -	int errcode;
>>  
>>  	lock_buffer(journal->j_sb_buffer);
>> -	errcode = journal->j_errno;
>> -	if (errcode == -ESHUTDOWN)
>> -		errcode = 0;
>> -	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
>> -	sb->s_errno    = cpu_to_be32(errcode);
>> +	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n",
>> +		  journal->j_errno);
>> +	sb->s_errno = cpu_to_be32(journal->j_errno);
>>  
>>  	jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA);
>>  }
>> @@ -2100,20 +2097,15 @@ void __jbd2_journal_abort_hard(journal_t *journal)
>>   * but don't do any other IO. */
>>  static void __journal_abort_soft (journal_t *journal, int errno)
>>  {
>> -	int old_errno;
>> -
>>  	write_lock(&journal->j_state_lock);
>> -	old_errno = journal->j_errno;
>> -	if (!journal->j_errno || errno == -ESHUTDOWN)
>> -		journal->j_errno = errno;
>> -
>>  	if (journal->j_flags & JBD2_ABORT) {
>>  		write_unlock(&journal->j_state_lock);
>> -		if (!old_errno && old_errno != -ESHUTDOWN &&
>> -		    errno == -ESHUTDOWN)
>> -			jbd2_journal_update_sb_errno(journal);
>>  		return;
>>  	}
>> +
>> +	if (!journal->j_errno)
>> +		journal->j_errno = errno;
>> +
>>  	write_unlock(&journal->j_state_lock);
>>  
>>  	__jbd2_journal_abort_hard(journal);
>> -- 
>> 2.17.2
>>

[PATCH v2 4/4] jbd2: clean __jbd2_journal_abort_hard() and __journal_abort_soft()

[zhangyi (F)]

__jbd2_journal_abort_hard() has never been used, now we can merge
__jbd2_journal_abort_hard() and __journal_abort_soft() these two
functions into jbd2_journal_abort() and remove them.

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c    | 93 ++++++++++++++++----------------------------
 include/linux/jbd2.h |  1 -
 2 files changed, 33 insertions(+), 61 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index f3f9e0b994ef..1171dfac3861 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -96,7 +96,6 @@ EXPORT_SYMBOL(jbd2_journal_release_jbd_inode);
 EXPORT_SYMBOL(jbd2_journal_begin_ordered_truncate);
 EXPORT_SYMBOL(jbd2_inode_cache);
 
-static void __journal_abort_soft (journal_t *journal, int errno);
 static int jbd2_journal_create_slab(size_t slab_size);
 
 #ifdef CONFIG_JBD2_DEBUG
@@ -805,7 +804,7 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
 					"at offset %lu on %s\n",
 			       __func__, blocknr, journal->j_devname);
 			err = -EIO;
-			__journal_abort_soft(journal, err);
+			jbd2_journal_abort(journal, err);
 		}
 	} else {
 		*retp = blocknr; /* +journal->j_blk_offset */
@@ -2062,62 +2061,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
 	return err;
 }
 
-/*
- * Journal abort has very specific semantics, which we describe
- * for journal abort.
- *
- * Two internal functions, which provide abort to the jbd layer
- * itself are here.
- */
-
-/*
- * Quick version for internal journal use (doesn't lock the journal).
- * Aborts hard --- we mark the abort as occurred, but do _nothing_ else,
- * and don't attempt to make any other journal updates.
- */
-void __jbd2_journal_abort_hard(journal_t *journal)
-{
-	transaction_t *transaction;
-
-	if (journal->j_flags & JBD2_ABORT)
-		return;
-
-	printk(KERN_ERR "Aborting journal on device %s.\n",
-	       journal->j_devname);
-
-	write_lock(&journal->j_state_lock);
-	journal->j_flags |= JBD2_ABORT;
-	transaction = journal->j_running_transaction;
-	if (transaction)
-		__jbd2_log_start_commit(journal, transaction->t_tid);
-	write_unlock(&journal->j_state_lock);
-}
-
-/* Soft abort: record the abort error status in the journal superblock,
- * but don't do any other IO. */
-static void __journal_abort_soft (journal_t *journal, int errno)
-{
-	write_lock(&journal->j_state_lock);
-	if (journal->j_flags & JBD2_ABORT) {
-		write_unlock(&journal->j_state_lock);
-		return;
-	}
-
-	if (!journal->j_errno)
-		journal->j_errno = errno;
-
-	write_unlock(&journal->j_state_lock);
-
-	__jbd2_journal_abort_hard(journal);
-
-	if (errno)
-		jbd2_journal_update_sb_errno(journal);
-
-	write_lock(&journal->j_state_lock);
-	journal->j_flags |= JBD2_ABORT_DONE;
-	write_unlock(&journal->j_state_lock);
-}
-
 /**
  * void jbd2_journal_abort () - Shutdown the journal immediately.
  * @journal: the journal to shutdown.
@@ -2163,10 +2106,40 @@ static void __journal_abort_soft (journal_t *journal, int errno)
  * progress).
  *
  */
-
 void jbd2_journal_abort(journal_t *journal, int errno)
 {
-	__journal_abort_soft(journal, errno);
+	transaction_t *transaction;
+
+	write_lock(&journal->j_state_lock);
+	if (journal->j_flags & JBD2_ABORT) {
+		write_unlock(&journal->j_state_lock);
+		return;
+	}
+
+	/*
+	 * Mark the abort as occurred and start current running transaction
+	 * to release all journaled buffer.
+	 */
+	pr_err("Aborting journal on device %s.\n", journal->j_devname);
+
+	journal->j_flags |= JBD2_ABORT;
+	transaction = journal->j_running_transaction;
+	if (transaction)
+		__jbd2_log_start_commit(journal, transaction->t_tid);
+
+	/*
+	 * Record errno to the journal super block, so that fsck and jbd2
+	 * layer could realise that a filesystem check is needed.
+	 */
+	if (errno) {
+		journal->j_errno = errno;
+
+		write_unlock(&journal->j_state_lock);
+		jbd2_journal_update_sb_errno(journal);
+		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 71cab887fb98..b6cabb685b59 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1403,7 +1403,6 @@ extern int	   jbd2_journal_skip_recovery	(journal_t *);
 extern void	   jbd2_journal_update_sb_errno(journal_t *);
 extern int	   jbd2_journal_update_sb_log_tail	(journal_t *, tid_t,
 				unsigned long, int);
-extern void	   __jbd2_journal_abort_hard	(journal_t *);
 extern void	   jbd2_journal_abort      (journal_t *, int);
 extern int	   jbd2_journal_errno      (journal_t *);
 extern void	   jbd2_journal_ack_err    (journal_t *);
-- 
2.17.2