[5/7] ext4: pass -ESHUTDOWN code to jbd2 layer

Message ID 20180220023038.19883-6-tytso@mit.edu
State New
Headers show
Series
  • ext4: fix up shutdown handling
Related show

Commit Message

Theodore Y. Ts'o Feb. 20, 2018, 2:30 a.m.
Previously the jbd2 layer assumed that a file system check would be
required after a journal abort.  In the case of the deliberate file
system shutdown, this should not be necessary.  Allow the jbd2 layer
to distinguish between these two cases by using the ESHUTDOWN errno.

Also add proper locking to __journal_abort_soft().

Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
---
 fs/ext4/ioctl.c   |  4 ++--
 fs/jbd2/journal.c | 25 +++++++++++++++++++------
 2 files changed, 21 insertions(+), 8 deletions(-)

Comments

Jan Kara March 6, 2018, 5:10 p.m. | #1
On Mon 19-02-18 21:30:36, Theodore Ts'o wrote:
> Previously the jbd2 layer assumed that a file system check would be
> required after a journal abort.  In the case of the deliberate file
> system shutdown, this should not be necessary.  Allow the jbd2 layer
> to distinguish between these two cases by using the ESHUTDOWN errno.
> 
> Also add proper locking to __journal_abort_soft().
> 
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> Cc: stable@vger.kernel.org
> ---

Maybe a cleaner api would be to have a separate JBD2 function like
jbd2_journal_abort_clean() that would just abort the journal without
setting an error to it.

> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 3fbf48ec2188..efa0c72a0b9f 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2105,12 +2108,22 @@ 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)
>  {
> -	if (journal->j_flags & JBD2_ABORT)
> -		return;
> +	int old_errno;
>  
> -	if (!journal->j_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;
> +	}
> +	write_unlock(&journal->j_state_lock);
> +

Is it really correct that once the filesystem gets shutdown you clear the
previous error from the journal? Because if we hit some real fs corruption,
the journal gets aborted, and then someone calls ext4_shutdown(), we'd
clear that error which looks like a bug to me because that shutdown hardly
fixes the fs corruption...

								Honza
Theodore Y. Ts'o March 22, 2018, 3:26 p.m. | #2
On Tue, Mar 06, 2018 at 06:10:41PM +0100, Jan Kara wrote:
> Is it really correct that once the filesystem gets shutdown you clear the
> previous error from the journal? Because if we hit some real fs corruption,
> the journal gets aborted, and then someone calls ext4_shutdown(), we'd
> clear that error which looks like a bug to me because that shutdown hardly
> fixes the fs corruption...

That's not what the code does.  If journal->j_errno is set, then we
won't clear it, for precisely what concern you've articulated.

      	    	    	      	   	   - Ted

Patch

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 16d3d1325f5b..9ac33a7cbd32 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -493,13 +493,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, 0);
+			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
 		}
 		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, 0);
+			jbd2_journal_abort(sbi->s_journal, -ESHUTDOWN);
 		break;
 	default:
 		return -EINVAL;
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 3fbf48ec2188..efa0c72a0b9f 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1483,12 +1483,15 @@  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;
 
 	read_lock(&journal->j_state_lock);
-	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n",
-		  journal->j_errno);
-	sb->s_errno    = cpu_to_be32(journal->j_errno);
+	errcode = journal->j_errno;
 	read_unlock(&journal->j_state_lock);
+	if (errcode == -ESHUTDOWN)
+		errcode = 0;
+	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
+	sb->s_errno    = cpu_to_be32(errcode);
 
 	jbd2_write_superblock(journal, REQ_SYNC | REQ_FUA);
 }
@@ -2105,12 +2108,22 @@  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)
 {
-	if (journal->j_flags & JBD2_ABORT)
-		return;
+	int old_errno;
 
-	if (!journal->j_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;
+	}
+	write_unlock(&journal->j_state_lock);
+
 	__jbd2_journal_abort_hard(journal);
 
 	if (errno) {