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

[PATCH v3 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 fix missing updating ESHUTDOWN problem in commit 818d276ceb8 "ext4:
Add the journal checksum feature", please revirew this series and give
some suggestions.

Changes since v2:
 - Fix spelling mistakes in the first patch.
 - Keep JBD2_REC_ERR and remove the last place that invoke
   jbd2_journal_abort() with 0 errno and the corresponding logic in
   __journal_abort_soft().
 - Fix missing updating errno in the jbd2 sb after jbd2 shutdown abort.

Thanks,
Yi.

zhangyi (F) (4):
  jbd2: switch to use jbd2_journal_abort() when failed to submit the
    commit record
  ext4, jbd2: ensure panic when aborting with zero errno
  jbd2: make sure ESHUTDOWN to be recorded in the journal superblock
  jbd2: clean __jbd2_journal_abort_hard() and __journal_abort_soft()

 fs/jbd2/checkpoint.c |   2 +-
 fs/jbd2/commit.c     |   4 +-
 fs/jbd2/journal.c    | 111 ++++++++++++++++---------------------------
 include/linux/jbd2.h |   1 -
 4 files changed, 45 insertions(+), 73 deletions(-)

-- 
2.17.2

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

[zhangyi (F)]

We invoke 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 invoke 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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

[PATCH v3 2/4] ext4, jbd2: ensure panic when 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 always record the proper errno in the
journal superblock.

Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock")
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/jbd2/checkpoint.c |  2 +-
 fs/jbd2/journal.c    | 15 ++++-----------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
index a1909066bde6..62cf497f18eb 100644
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -164,7 +164,7 @@ void __jbd2_log_wait_for_space(journal_t *journal)
 				       "journal space in %s\n", __func__,
 				       journal->j_devname);
 				WARN_ON(1);
-				jbd2_journal_abort(journal, 0);
+				jbd2_journal_abort(journal, -EIO);
 			}
 			write_lock(&journal->j_state_lock);
 		} else {
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 1c58859aa592..b2d6e7666d0f 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2118,12 +2118,10 @@ static void __journal_abort_soft (journal_t *journal, int errno)
 
 	__jbd2_journal_abort_hard(journal);
 
-	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);
-	}
+	jbd2_journal_update_sb_errno(journal);
+	write_lock(&journal->j_state_lock);
+	journal->j_flags |= JBD2_REC_ERR;
+	write_unlock(&journal->j_state_lock);
 }
 
 /**
@@ -2165,11 +2163,6 @@ static void __journal_abort_soft (journal_t *journal, int errno)
  * failure to disk.  ext3_error, for example, now uses this
  * functionality.
  *
- * Errors which originate from within the journaling layer will NOT
- * supply an errno; a null errno implies that absolutely no further
- * writes are done to the journal (unless there are any already in
- * progress).
- *
  */
 
 void jbd2_journal_abort(journal_t *journal, int errno)
-- 
2.17.2

[PATCH v3 3/4] jbd2: make sure ESHUTDOWN to be recorded in the journal superblock

[zhangyi (F)]

Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want
to allow jbd2 layer to distinguish shutdown journal abort from other
error cases. So the ESHUTDOWN should be taken precedence over any other
errno which has already been recoded after EXT4_FLAGS_SHUTDOWN is set,
but it only update errno in the journal suoerblock now if the old errno
is 0.

Fixes: fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/jbd2/journal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index b2d6e7666d0f..93be6e0311da 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2109,8 +2109,7 @@ static void __journal_abort_soft (journal_t *journal, int errno)
 
 	if (journal->j_flags & JBD2_ABORT) {
 		write_unlock(&journal->j_state_lock);
-		if (!old_errno && old_errno != -ESHUTDOWN &&
-		    errno == -ESHUTDOWN)
+		if (old_errno != -ESHUTDOWN && errno == -ESHUTDOWN)
 			jbd2_journal_update_sb_errno(journal);
 		return;
 	}
-- 
2.17.2

[PATCH v3 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    | 103 ++++++++++++++++++-------------------------
 include/linux/jbd2.h |   1 -
 2 files changed, 42 insertions(+), 62 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 93be6e0311da..e59d9b6e4596 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 */
@@ -2065,64 +2064,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)
-{
-	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 != -ESHUTDOWN && errno == -ESHUTDOWN)
-			jbd2_journal_update_sb_errno(journal);
-		return;
-	}
-	write_unlock(&journal->j_state_lock);
-
-	__jbd2_journal_abort_hard(journal);
-
-	jbd2_journal_update_sb_errno(journal);
-	write_lock(&journal->j_state_lock);
-	journal->j_flags |= JBD2_REC_ERR;
-	write_unlock(&journal->j_state_lock);
-}
-
 /**
  * void jbd2_journal_abort () - Shutdown the journal immediately.
  * @journal: the journal to shutdown.
@@ -2166,7 +2107,47 @@ static void __journal_abort_soft (journal_t *journal, int errno)
 
 void jbd2_journal_abort(journal_t *journal, int errno)
 {
-	__journal_abort_soft(journal, errno);
+	transaction_t *transaction;
+
+	/*
+	 * ESHUTDOWN always takes precedence because a file system check
+	 * caused by any other journal abort error is not required after
+	 * a shutdown triggered.
+	 */
+	write_lock(&journal->j_state_lock);
+	if (journal->j_flags & JBD2_ABORT) {
+		int old_errno = journal->j_errno;
+
+		write_unlock(&journal->j_state_lock);
+		if (old_errno != -ESHUTDOWN && errno == -ESHUTDOWN) {
+			journal->j_errno = errno;
+			jbd2_journal_update_sb_errno(journal);
+		}
+		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;
+	journal->j_errno = errno;
+	transaction = journal->j_running_transaction;
+	if (transaction)
+		__jbd2_log_start_commit(journal, transaction->t_tid);
+	write_unlock(&journal->j_state_lock);
+
+	/*
+	 * Record errno to the journal super block, so that fsck and jbd2
+	 * layer could realise that a filesystem check is needed.
+	 */
+	jbd2_journal_update_sb_errno(journal);
+
+	write_lock(&journal->j_state_lock);
+	journal->j_flags |= JBD2_REC_ERR;
+	write_unlock(&journal->j_state_lock);
 }
 
 /**
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 603fbc4e2f70..e3e271bfb0e7 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1402,7 +1402,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

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

[Jan Kara]

On Wed 04-12-19 20:46:12, 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 always record the proper errno in the
> journal superblock.
> 
> Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock")
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Looks good to me. You can add:

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

								Honza

> ---
>  fs/jbd2/checkpoint.c |  2 +-
>  fs/jbd2/journal.c    | 15 ++++-----------
>  2 files changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index a1909066bde6..62cf497f18eb 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -164,7 +164,7 @@ void __jbd2_log_wait_for_space(journal_t *journal)
>  				       "journal space in %s\n", __func__,
>  				       journal->j_devname);
>  				WARN_ON(1);
> -				jbd2_journal_abort(journal, 0);
> +				jbd2_journal_abort(journal, -EIO);
>  			}
>  			write_lock(&journal->j_state_lock);
>  		} else {
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 1c58859aa592..b2d6e7666d0f 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2118,12 +2118,10 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>  
>  	__jbd2_journal_abort_hard(journal);
>  
> -	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);
> -	}
> +	jbd2_journal_update_sb_errno(journal);
> +	write_lock(&journal->j_state_lock);
> +	journal->j_flags |= JBD2_REC_ERR;
> +	write_unlock(&journal->j_state_lock);
>  }
>  
>  /**
> @@ -2165,11 +2163,6 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>   * failure to disk.  ext3_error, for example, now uses this
>   * functionality.
>   *
> - * Errors which originate from within the journaling layer will NOT
> - * supply an errno; a null errno implies that absolutely no further
> - * writes are done to the journal (unless there are any already in
> - * progress).
> - *
>   */
>  
>  void jbd2_journal_abort(journal_t *journal, int errno)
> -- 
> 2.17.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 3/4] jbd2: make sure ESHUTDOWN to be recorded in the journal superblock

[Jan Kara]

On Wed 04-12-19 20:46:13, zhangyi (F) wrote:
> Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want
> to allow jbd2 layer to distinguish shutdown journal abort from other
> error cases. So the ESHUTDOWN should be taken precedence over any other
> errno which has already been recoded after EXT4_FLAGS_SHUTDOWN is set,
> but it only update errno in the journal suoerblock now if the old errno
> is 0.
> 
> Fixes: fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Yeah, I think this is correct if I understand the logic correctly but I'd
like Ted to have a look at this. Anyway, feel free to add:

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

								Honza

> ---
>  fs/jbd2/journal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index b2d6e7666d0f..93be6e0311da 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2109,8 +2109,7 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>  
>  	if (journal->j_flags & JBD2_ABORT) {
>  		write_unlock(&journal->j_state_lock);
> -		if (!old_errno && old_errno != -ESHUTDOWN &&
> -		    errno == -ESHUTDOWN)
> +		if (old_errno != -ESHUTDOWN && errno == -ESHUTDOWN)
>  			jbd2_journal_update_sb_errno(journal);
>  		return;
>  	}
> -- 
> 2.17.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Wed 04-12-19 20:46:14, zhangyi (F) wrote:
> __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>

Looks good to me. You can add:

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

								Honza

> ---
>  fs/jbd2/journal.c    | 103 ++++++++++++++++++-------------------------
>  include/linux/jbd2.h |   1 -
>  2 files changed, 42 insertions(+), 62 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 93be6e0311da..e59d9b6e4596 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 */
> @@ -2065,64 +2064,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)
> -{
> -	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 != -ESHUTDOWN && errno == -ESHUTDOWN)
> -			jbd2_journal_update_sb_errno(journal);
> -		return;
> -	}
> -	write_unlock(&journal->j_state_lock);
> -
> -	__jbd2_journal_abort_hard(journal);
> -
> -	jbd2_journal_update_sb_errno(journal);
> -	write_lock(&journal->j_state_lock);
> -	journal->j_flags |= JBD2_REC_ERR;
> -	write_unlock(&journal->j_state_lock);
> -}
> -
>  /**
>   * void jbd2_journal_abort () - Shutdown the journal immediately.
>   * @journal: the journal to shutdown.
> @@ -2166,7 +2107,47 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>  
>  void jbd2_journal_abort(journal_t *journal, int errno)
>  {
> -	__journal_abort_soft(journal, errno);
> +	transaction_t *transaction;
> +
> +	/*
> +	 * ESHUTDOWN always takes precedence because a file system check
> +	 * caused by any other journal abort error is not required after
> +	 * a shutdown triggered.
> +	 */
> +	write_lock(&journal->j_state_lock);
> +	if (journal->j_flags & JBD2_ABORT) {
> +		int old_errno = journal->j_errno;
> +
> +		write_unlock(&journal->j_state_lock);
> +		if (old_errno != -ESHUTDOWN && errno == -ESHUTDOWN) {
> +			journal->j_errno = errno;
> +			jbd2_journal_update_sb_errno(journal);
> +		}
> +		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;
> +	journal->j_errno = errno;
> +	transaction = journal->j_running_transaction;
> +	if (transaction)
> +		__jbd2_log_start_commit(journal, transaction->t_tid);
> +	write_unlock(&journal->j_state_lock);
> +
> +	/*
> +	 * Record errno to the journal super block, so that fsck and jbd2
> +	 * layer could realise that a filesystem check is needed.
> +	 */
> +	jbd2_journal_update_sb_errno(journal);
> +
> +	write_lock(&journal->j_state_lock);
> +	journal->j_flags |= JBD2_REC_ERR;
> +	write_unlock(&journal->j_state_lock);
>  }
>  
>  /**
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 603fbc4e2f70..e3e271bfb0e7 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1402,7 +1402,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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 3/4] jbd2: make sure ESHUTDOWN to be recorded in the journal superblock

[zhangyi (F)]

On 2019/12/5 1:05, Jan Kara wrote:
> On Wed 04-12-19 20:46:13, zhangyi (F) wrote:
>> Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want
>> to allow jbd2 layer to distinguish shutdown journal abort from other
>> error cases. So the ESHUTDOWN should be taken precedence over any other
>> errno which has already been recoded after EXT4_FLAGS_SHUTDOWN is set,
>> but it only update errno in the journal suoerblock now if the old errno
>> is 0.
>>
>> Fixes: fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> 
> Yeah, I think this is correct if I understand the logic correctly but I'd
> like Ted to have a look at this. Anyway, feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 

Thanks for review.

Hi Ted, do you have time to look at this patch?

Thanks,
Yi.

> 
>> ---
>>  fs/jbd2/journal.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index b2d6e7666d0f..93be6e0311da 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -2109,8 +2109,7 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>>  
>>  	if (journal->j_flags & JBD2_ABORT) {
>>  		write_unlock(&journal->j_state_lock);
>> -		if (!old_errno && old_errno != -ESHUTDOWN &&
>> -		    errno == -ESHUTDOWN)
>> +		if (old_errno != -ESHUTDOWN && errno == -ESHUTDOWN)
>>  			jbd2_journal_update_sb_errno(journal);
>>  		return;
>>  	}
>> -- 
>> 2.17.2
>>

Re: [PATCH v3 3/4] jbd2: make sure ESHUTDOWN to be recorded in the journal superblock

[zhangyi (F)]

On 2019/12/5 9:23, zhangyi (F) wrote:
> On 2019/12/5 1:05, Jan Kara wrote:
>> On Wed 04-12-19 20:46:13, zhangyi (F) wrote:
>>> Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want
>>> to allow jbd2 layer to distinguish shutdown journal abort from other
>>> error cases. So the ESHUTDOWN should be taken precedence over any other
>>> errno which has already been recoded after EXT4_FLAGS_SHUTDOWN is set,
>>> but it only update errno in the journal suoerblock now if the old errno
>>> is 0.
>>>
>>> Fixes: fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
>>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>>
>> Yeah, I think this is correct if I understand the logic correctly but I'd
>> like Ted to have a look at this. Anyway, feel free to add:
>>
>> Reviewed-by: Jan Kara <jack@suse.cz>
>>
> 
> Thanks for review.
> 
> Hi Ted, do you have time to look at this patch?
> 

Genteel ping.

Thanks,
Yi.

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

[Theodore Y. Ts'o]

On Wed, Dec 04, 2019 at 08:46:11PM +0800, zhangyi (F) wrote:
> We invoke 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 invoke 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>
> Reviewed-by: Jan Kara <jack@suse.cz>

Applied, thanks.

					- Ted

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

[Theodore Y. Ts'o]

On Wed, Dec 04, 2019 at 01:52:13PM +0100, Jan Kara wrote:
> On Wed 04-12-19 20:46:12, 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 always record the proper errno in the
> > journal superblock.
> > 
> > Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock")
> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> 
> Looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Applied, thanks.

					- Ted

Re: [PATCH v3 3/4] jbd2: make sure ESHUTDOWN to be recorded in the journal superblock

[Theodore Y. Ts'o]

On Wed, Dec 04, 2019 at 06:05:28PM +0100, Jan Kara wrote:
> On Wed 04-12-19 20:46:13, zhangyi (F) wrote:
> > Commit fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer") want
> > to allow jbd2 layer to distinguish shutdown journal abort from other
> > error cases. So the ESHUTDOWN should be taken precedence over any other
> > errno which has already been recoded after EXT4_FLAGS_SHUTDOWN is set,
> > but it only update errno in the journal suoerblock now if the old errno
> > is 0.
> > 
> > Fixes: fb7c02445c49 ("ext4: pass -ESHUTDOWN code to jbd2 layer")
> > Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
> 
> Yeah, I think this is correct if I understand the logic correctly but I'd
> like Ted to have a look at this. Anyway, feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Yep, this looks sane.  Thanks, applied.

				- Ted

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

[Theodore Y. Ts'o]

On Wed, Dec 04, 2019 at 08:46:14PM +0800, zhangyi (F) wrote:
> __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>

Thanks, applied.

					- Ted