Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb
@ 2019-11-26 14:45 zhangyi (F)
  2019-11-29 14:46 ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: zhangyi (F) @ 2019-11-26 14:45 UTC (permalink / raw)
  To: linux-ext4; +Cc: jack, tytso, adilger.kernel, yi.zhang, liangyun2

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 there is one exception, if jbd2
thread failed to submit commit record, it abort journal through
invoking __jbd2_journal_abort_hard() without set this flag, so we can
no longer panic. Fix this by 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>
Cc: <stable@vger.kernel.org>
---
 fs/ext4/super.c      |  4 ++--
 fs/jbd2/journal.c    | 46 +++++++++++++++++++++++++++++---------------
 include/linux/jbd2.h |  4 +++-
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index dd654e53ba3d..76cde5fb8207 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_FINISHED))
 			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_FINISHED))
 			return;
 		panic("EXT4-fs panic from previous error\n");
 	}
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 1c58859aa592..eb5e60df0da4 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2072,13 +2072,7 @@ int jbd2_journal_wipe(journal_t *journal, int write)
  * 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)
+static void __jbd2_journal_abort(journal_t *journal)
 {
 	transaction_t *transaction;
 
@@ -2096,8 +2090,33 @@ void __jbd2_journal_abort_hard(journal_t *journal)
 	write_unlock(&journal->j_state_lock);
 }
 
-/* Soft abort: record the abort error status in the journal superblock,
- * but don't do any other IO. */
+/*
+ * Mark journal abort finished when the errno in the sb has been recorded
+ * or no need to record.
+ */
+static void __jbd2_journal_finish_abort(journal_t *journal)
+{
+	write_lock(&journal->j_state_lock);
+	journal->j_flags |= JBD2_ABORT_FINISHED;
+	write_unlock(&journal->j_state_lock);
+}
+
+/*
+ * 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)
+{
+	/* Nothing need to be recorded, mark it as finished directly */
+	__jbd2_journal_abort(journal);
+	__jbd2_journal_finish_abort(journal);
+}
+
+/*
+ * 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;
@@ -2116,14 +2135,11 @@ static void __journal_abort_soft (journal_t *journal, int errno)
 	}
 	write_unlock(&journal->j_state_lock);
 
-	__jbd2_journal_abort_hard(journal);
+	__jbd2_journal_abort(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);
-	}
+	__jbd2_journal_finish_abort(journal);
 }
 
 /**
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 603fbc4e2f70..870f7f2f912c 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1248,7 +1248,9 @@ 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_FINISHED		0x080	/* Abort finished, the errno
+						 * in the sb has been recorded
+						 * if necessary */
 
 /*
  * Function declarations for the journaling transaction and buffer
-- 
2.17.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb
  2019-11-26 14:45 [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb zhangyi (F)
@ 2019-11-29 14:46 ` Jan Kara
  2019-11-30  3:24   ` zhangyi (F)
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2019-11-29 14:46 UTC (permalink / raw)
  To: zhangyi (F); +Cc: linux-ext4, jack, tytso, adilger.kernel, liangyun2

On Tue 26-11-19 22:45:37, 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 there is one exception, if jbd2
> thread failed to submit commit record, it abort journal through
> invoking __jbd2_journal_abort_hard() without set this flag, so we can
> no longer panic. Fix this by 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>
> Cc: <stable@vger.kernel.org>

Thanks for the patch. This indeed looks like a bug. I was trying hard to
understand why are we actually using __jbd2_journal_abort_hard() in
fs/jbd2/commit.c in the first place. And after some digging, I think it is
an oversight and we should just use jbd2_journal_abort(). The calls have been
introduced by commit 818d276ceb83a "ext4: Add the journal checksum
feature". Before that commit, we were just using jbd2_journal_abort() when
writing commit block failed. And when we use jbd2_journal_abort() from
everywhere, that will also deal with the problem you've found.

Also as a nice cleanup we could then just drop __jbd2_journal_abort_hard(),
__jbd2_journal_abort_soft() and have all the functionality in a single
function jbd2_journal_abort().

								Honza

> ---
>  fs/ext4/super.c      |  4 ++--
>  fs/jbd2/journal.c    | 46 +++++++++++++++++++++++++++++---------------
>  include/linux/jbd2.h |  4 +++-
>  3 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index dd654e53ba3d..76cde5fb8207 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_FINISHED))
>  			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_FINISHED))
>  			return;
>  		panic("EXT4-fs panic from previous error\n");
>  	}
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 1c58859aa592..eb5e60df0da4 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -2072,13 +2072,7 @@ int jbd2_journal_wipe(journal_t *journal, int write)
>   * 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)
> +static void __jbd2_journal_abort(journal_t *journal)
>  {
>  	transaction_t *transaction;
>  
> @@ -2096,8 +2090,33 @@ void __jbd2_journal_abort_hard(journal_t *journal)
>  	write_unlock(&journal->j_state_lock);
>  }
>  
> -/* Soft abort: record the abort error status in the journal superblock,
> - * but don't do any other IO. */
> +/*
> + * Mark journal abort finished when the errno in the sb has been recorded
> + * or no need to record.
> + */
> +static void __jbd2_journal_finish_abort(journal_t *journal)
> +{
> +	write_lock(&journal->j_state_lock);
> +	journal->j_flags |= JBD2_ABORT_FINISHED;
> +	write_unlock(&journal->j_state_lock);
> +}
> +
> +/*
> + * 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)
> +{
> +	/* Nothing need to be recorded, mark it as finished directly */
> +	__jbd2_journal_abort(journal);
> +	__jbd2_journal_finish_abort(journal);
> +}
> +
> +/*
> + * 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;
> @@ -2116,14 +2135,11 @@ static void __journal_abort_soft (journal_t *journal, int errno)
>  	}
>  	write_unlock(&journal->j_state_lock);
>  
> -	__jbd2_journal_abort_hard(journal);
> +	__jbd2_journal_abort(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);
> -	}
> +	__jbd2_journal_finish_abort(journal);
>  }
>  
>  /**
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 603fbc4e2f70..870f7f2f912c 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1248,7 +1248,9 @@ 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_FINISHED		0x080	/* Abort finished, 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb
  2019-11-29 14:46 ` Jan Kara
@ 2019-11-30  3:24   ` zhangyi (F)
  2019-11-30 14:50     ` zhangyi (F)
  0 siblings, 1 reply; 4+ messages in thread
From: zhangyi (F) @ 2019-11-30  3:24 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, jack, tytso, adilger.kernel, liangyun2

On 2019/11/29 22:46, Jan Kara wrote:
> On Tue 26-11-19 22:45:37, 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 there is one exception, if jbd2
>> thread failed to submit commit record, it abort journal through
>> invoking __jbd2_journal_abort_hard() without set this flag, so we can
>> no longer panic. Fix this by 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>
>> Cc: <stable@vger.kernel.org>
> 
> Thanks for the patch. This indeed looks like a bug. I was trying hard to
> understand why are we actually using __jbd2_journal_abort_hard() in
> fs/jbd2/commit.c in the first place. And after some digging, I think it is
> an oversight and we should just use jbd2_journal_abort(). The calls have been
> introduced by commit 818d276ceb83a "ext4: Add the journal checksum
> feature". Before that commit, we were just using jbd2_journal_abort() when
> writing commit block failed. And when we use jbd2_journal_abort() from
> everywhere, that will also deal with the problem you've found.
> 
> Also as a nice cleanup we could then just drop __jbd2_journal_abort_hard(),
> __jbd2_journal_abort_soft() and have all the functionality in a single
> function jbd2_journal_abort().
>

Indeed, it seems that we also need to record the errno if we failed to
submit commit block, I will remove __jbd2_journal_abort_hard() and combine
them in my next iteration.

Thanks,
Yi.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb
  2019-11-30  3:24   ` zhangyi (F)
@ 2019-11-30 14:50     ` zhangyi (F)
  0 siblings, 0 replies; 4+ messages in thread
From: zhangyi (F) @ 2019-11-30 14:50 UTC (permalink / raw)
  To: Jan Kara, tytso; +Cc: linux-ext4, jack, adilger.kernel, liangyun2

On 2019/11/30 11:24, zhangyi (F) wrote:
> On 2019/11/29 22:46, Jan Kara wrote:
>> On Tue 26-11-19 22:45:37, 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 there is one exception, if jbd2
>>> thread failed to submit commit record, it abort journal through
>>> invoking __jbd2_journal_abort_hard() without set this flag, so we can
>>> no longer panic. Fix this by 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>
>>> Cc: <stable@vger.kernel.org>
>>
>> Thanks for the patch. This indeed looks like a bug. I was trying hard to
>> understand why are we actually using __jbd2_journal_abort_hard() in
>> fs/jbd2/commit.c in the first place. And after some digging, I think it is
>> an oversight and we should just use jbd2_journal_abort(). The calls have been
>> introduced by commit 818d276ceb83a "ext4: Add the journal checksum
>> feature". Before that commit, we were just using jbd2_journal_abort() when
>> writing commit block failed. And when we use jbd2_journal_abort() from
>> everywhere, that will also deal with the problem you've found.
>>
>> Also as a nice cleanup we could then just drop __jbd2_journal_abort_hard(),
>> __jbd2_journal_abort_soft() and have all the functionality in a single
>> function jbd2_journal_abort().
>>
> 
> Indeed, it seems that we also need to record the errno if we failed to
> submit commit block, I will remove __jbd2_journal_abort_hard() and combine
> them in my next iteration.
> 

Hi Ted and Jan,
I am confusing about the commit fb7c02445c49 "ext4: pass
-ESHUTDOWN code to jbd2 layer" when I trying to cleanup the
__journal_abort_soft() and __jbd2_journal_abort_hard().

Before this commit, we will not record the errno if we shutdown the
filesystem no matter it has been aborted or not, so the errno will not
change.
After this commit, we record 0 to "sb->s_errno" for the first
jbd2_journal_abort(-ESHUTDOWN), and we also do not update the errno
if it has been aborted and record a no-zero errno because of the
follow checking.

+       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;
+       }

So the only modification of this patch is:
1) fix the lock;
2) set journal->j_errno = -ESHUTDOWN and JBD2_REC_ERR flag when we
   invoke jbd2_journal_abort(-ESHUTDOWN). These two modifications
   do not relate to the git log you mentioned.

I guess do you want to mean
  if (old_errno && old_errno != -ESHUTDOWN && errno == -ESHUTDOWN) ?

If so, why we need to overwrite the last aborted errno to 0,
if the filesystem was already aborted for some reasons, will
it cover up the issue? Am I miss something?

Thanks,
Yi.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-26 14:45 [PATCH] ext4, jbd2: ensure panic when there is no need to record errno in the jbd2 sb zhangyi (F)
2019-11-29 14:46 ` Jan Kara
2019-11-30  3:24   ` zhangyi (F)
2019-11-30 14:50     ` zhangyi (F)

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git