ocfs2: clear journal dirty flag after shutdown journal

PATCH New Junxiao Bi 3 participants 5 messages Nov 16, 2018 – Nov 19, 2018 OCFS2

Share & tools

Junxiao Bi

author

2018-11-16 08:58 UTC #0 lore raw

PATCH 1/1 ocfs2: clear journal dirty flag after shutdown journal

Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption.

Signed-off-by Junxiao Bi <[email protected]>

---
 fs/ocfs2/journal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

fs/ocfs2/journal.c +4 -4

index b63c97f4318e..25d678c92fbb 100644
@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+
 	if (status == 0) {
 		/*
 		 * Do not toggle if flush was unsuccessful otherwise

@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
 	OCFS2_I(inode)->ip_open_count--;
 
 	/* unlock our journal */

Signature · 1 line

2.17.1

piaojun

2018-11-19 01:32 UTC #1 lore raw

PATCH 1/1 ocfs2: clear journal dirty flag after shutdown journal

Hi Junxiao,

I'm very interested in this bug, and it seems causing read-only if involving metadata. Could you help show how to reproduce this problem?

Thanks,
Jun

Quoted 38 lines · Junxiao Bi

Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption.
Signed-off-by Junxiao Bi <[email protected]>
---
 fs/ocfs2/journal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
fs/ocfs2/journal.c +4 -4
index b63c97f4318e..25d678c92fbb 100644
@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+
 	if (status == 0) {
 		/*
 		 * Do not toggle if flush was unsuccessful otherwise
@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
 	OCFS2_I(inode)->ip_open_count--;
 
 	/* unlock our journal */

Junxiao Bi

author

2018-11-19 01:45 UTC #2 lore raw

PATCH 1/1 ocfs2: clear journal dirty flag after shutdown journal

Hi Jun,

ocfs2_journal_shutdown() was only invoked during umount, jbd2 will force all uncommitted metadata to disk, there is no set fs readonly for io error in that case. To reproduce, you need cause storage return io error between ocfs2_journal_toggle_dirty() and jbd2_journal_destroy().

1020 if (status == 0) {
1021 /*

1022                  * Do not toggle if flush was unsuccessful otherwise
1023                  * will leave dirty metadata in a "clean" journal
1024                  */

1025 status = ocfs2_journal_toggle_dirty(osb, 0, 0);
1026 if (status < 0)
1027 mlog_errno(status);
1028 }
1029
1030 /* Shutdown the kernel journal system */
1031 jbd2_journal_destroy(journal->j_journal);

Thanks,
Junxiao.

Quoted 47 lines · piaojun

Hi Junxiao,
I'm very interested in this bug, and it seems causing read-only if involving metadata. Could you help show how to reproduce this problem?
Thanks,
Jun
Quoted 38 lines · Junxiao Bi
Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption.
Signed-off-by Junxiao Bi <[email protected]>
---
  fs/ocfs2/journal.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
fs/ocfs2/journal.c +4 -4
index b63c97f4318e..25d678c92fbb 100644
@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
  			mlog_errno(status);
  	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+
  	if (status == 0) {
  		/*
  		 * Do not toggle if flush was unsuccessful otherwise
@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
  			mlog_errno(status);
  	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
  	OCFS2_I(inode)->ip_open_count--;
 
  	/* unlock our journal */

jiangyiwen

2018-11-19 02:28 UTC #3 lore raw

PATCH 1/1 ocfs2: clear journal dirty flag after shutdown journal

In reply to Junxiao Bi

Quoted 16 lines · Junxiao Bi

Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption.
Signed-off-by Junxiao Bi <[email protected]>
---
 fs/ocfs2/journal.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
fs/ocfs2/journal.c +0 -0
index b63c97f4318e..25d678c92fbb 100644

fs/ocfs2/journal.c void ocfs2_journal_shutdown(struct ocfs2_super *osb) · line 1023

Quoted 7 lines

fs/ocfs2/journal.c +4 -0

@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+

Hi Junxiao,

I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I wrong or understand error ?

Thanks.
Yiwen.

Quoted 15 lines

fs/ocfs2/journal.c +0 -4

 	if (status == 0) {
 		/*
 		 * Do not toggle if flush was unsuccessful otherwise

@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
 			mlog_errno(status);
 	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
 	OCFS2_I(inode)->ip_open_count--;
 
 	/* unlock our journal */

Junxiao Bi

author

2018-11-19 02:43 UTC #4 lore raw

PATCH 1/1 ocfs2: clear journal dirty flag after shutdown journal

Quoted 26 lines · jiangyiwen

Quoted 16 lines · Junxiao Bi
Dirty flag of the journal should be cleared at the last stage of umount, if do it before jbd2_journal_destroy(), then some metadata in uncommitted transaction could be lost due to io error, but as dirty flag of journal was already cleared, we can't find that until run a full fsck. This may cause system panic or other corruption.
Signed-off-by Junxiao Bi <[email protected]>
---
  fs/ocfs2/journal.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)
fs/ocfs2/journal.c +0 -0
index b63c97f4318e..25d678c92fbb 100644
fs/ocfs2/journal.c void ocfs2_journal_shutdown(struct ocfs2_super *osb) · line 1023
Quoted 7 lines
fs/ocfs2/journal.c +4 -0
@@ -1017,6 +1017,10 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
  			mlog_errno(status);
  	}
 
+	/* Shutdown the kernel journal system */
+	jbd2_journal_destroy(journal->j_journal);
+	journal->j_journal = NULL;
+
Hi Junxiao,

Quoted 3 lines

I feel this adjustment doesn't make any sense. When jbd2_journal_destroy() is done, it still call ocfs2_journal_toggle_dirty() to clean dirty flag. Am I wrong or understand error ?

Oh, missed checking the return value of jbd2_journal_destroy(), if failed, should not call ocfs2_journal_toggle_dirty(). Thanks for pointing this.

Thanks,

Junxiao.

Quoted 20 lines

Thanks.
Yiwen.

Quoted 15 lines

fs/ocfs2/journal.c +0 -4

  	if (status == 0) {
  		/*
  		 * Do not toggle if flush was unsuccessful otherwise

@@ -1027,10 +1031,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
  			mlog_errno(status);
  	}
 
-	/* Shutdown the kernel journal system */
-	jbd2_journal_destroy(journal->j_journal);
-	journal->j_journal = NULL;
-
  	OCFS2_I(inode)->ip_open_count--;
 
  	/* unlock our journal */