ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

PATCH Superseded Joseph Qi 3 participants 8 messages May 12, 2015 – May 21, 2015 OCFS2

Share & tools

Joseph Qi

author

2015-05-12 01:53 UTC #0 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.

Signed-off-by Joseph Qi <[email protected]>

Cc joyce.xue <[email protected]>

---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

fs/ocfs2/journal.c +12 -1

index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
+		}
+	}
 }

#define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)

Signature · 1 line

1.8.4.3

Andrew Morton

2015-05-12 23:03 UTC #1 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 4 lines · On Tue, 12 May 2015 09:53:47 +0800 Joseph Qi <[email protected]> wrote:

jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.

This patch is internally inconsistent.

fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)

Quoted 3 lines

fs/ocfs2/journal.c +0 -0

@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)

The "-775,7 +775,17" means "the 7 lines at line 775 get turned into 17 lines". ie: 10 lines are added.

Quoted 7 lines

fs/ocfs2/journal.c +3 -1

 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {

fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 790

Quoted 10 lines

fs/ocfs2/journal.c +9 -0

+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
+		}
+	}
 }

But the patch deletes 1 line and adds 12, for a net addition of 11 lines, not 10.

So the patch doesn't apply. Changing the "17" to "18" fixes that.

Whatever tool you used for generating this patch needs slapping.

Junxiao Bi

2015-05-20 08:22 UTC #2 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

In reply to Joseph Qi

Quoted 19 lines · Joseph Qi

jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);

fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787

Quoted 10 lines

fs/ocfs2/journal.c +10 -0

+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);

Let fs go after journal lose affect seemed not safe, may we set fs read-only here?

Thanks,
Junxiao.

Quoted 6 lines

fs/ocfs2/journal.c +2 -0

+		}
+	}
 }
 
 #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)

Joseph Qi

author

2015-05-20 10:25 UTC #3 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 33 lines · Junxiao Bi

Quoted 19 lines · Joseph Qi
jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787
Quoted 10 lines
fs/ocfs2/journal.c +10 -0
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
Let fs go after journal lose affect seemed not safe, may we set fs read-only here?

Quoted 4 lines

Thanks,
Junxiao.

Do you mean journal can still be updated even if it is aborted?

Quoted 9 lines

Quoted 6 lines

fs/ocfs2/journal.c +2 -0

+		}
+	}
 }
 
 #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)

Junxiao Bi

2015-05-20 11:09 UTC #4 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 34 lines · Joseph Qi

Quoted 33 lines · Junxiao Bi
Quoted 19 lines · Joseph Qi
jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787
Quoted 10 lines
fs/ocfs2/journal.c +10 -0
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
Let fs go after journal lose affect seemed not safe, may we set fs read-only here?

Quoted 5 lines

Quoted 4 lines
Thanks,
Junxiao.
Do you mean journal can still be updated even if it is aborted?

No, journal will not be updated. After abort, journal api will return error, at last fs will be set read-only, but there is also api which didn't return error like ocfs2_journal_dirty(), so why not stop at the first time?

Thanks,
Junxiao.

Quoted 12 lines

Quoted 10 lines
Quoted 7 lines
fs/ocfs2/journal.c +2 -0
+		}
+	}
 }
 
 #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)

Joseph Qi

author

2015-05-21 00:49 UTC #5 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 39 lines · Junxiao Bi

Quoted 38 lines · Joseph Qi
Quoted 37 lines · Junxiao Bi
Quoted 19 lines · Joseph Qi
jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787
Quoted 10 lines
fs/ocfs2/journal.c +10 -0
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
Let fs go after journal lose affect seemed not safe, may we set fs read-only here?
Thanks,
Junxiao.

Quoted 8 lines

Quoted 1 line
Do you mean journal can still be updated even if it is aborted?
No, journal will not be updated. After abort, journal api will return error, at last fs will be set read-only, but there is also api which didn't return error like ocfs2_journal_dirty(), so why not stop at the first time?
Thanks,
Junxiao.

Agree with you.
But here bh can be anything like di, gd, ... As Joyce Xue reported, the problem also exists in ocfs2_abort_trigger. So I don't think we can get sb we needed.
Is there any other way except changing prototype of ocfs2_journal_dirty?

Quoted 16 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
fs/ocfs2/journal.c +2 -0
+		}
+	}
 }
 
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
#define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
.

Junxiao Bi

2015-05-21 02:09 UTC #6 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 46 lines · Joseph Qi

Quoted 45 lines · Junxiao Bi
Quoted 34 lines · Joseph Qi
Quoted 33 lines · Junxiao Bi
Quoted 19 lines · Joseph Qi
jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787
Quoted 10 lines
fs/ocfs2/journal.c +10 -0
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
Let fs go after journal lose affect seemed not safe, may we set fs read-only here?
Quoted 5 lines
Quoted 4 lines
Thanks,
Junxiao.
Do you mean journal can still be updated even if it is aborted?
No, journal will not be updated. After abort, journal api will return error, at last fs will be set read-only, but there is also api which didn't return error like ocfs2_journal_dirty(), so why not stop at the first time?

Quoted 6 lines

Quoted 2 lines
Thanks,
Junxiao.
Agree with you.
But here bh can be anything like di, gd, ... As Joyce Xue reported, the problem also exists in ocfs2_abort_trigger. So I don't think we can get sb we needed.

Yes, right.

Quoted 1 line

Is there any other way except changing prototype of ocfs2_journal_dirty?

I can't see how to do that. From me, if jbd2_journal_dirty_metadata() return an error, there seemed a bug, BUG_ON for a bug seemed acceptable since not easy to set fs read-only from here.
Did you ever watch this BUG_ON triggered not due to a bug?

Thanks,
Junxiao.

Quoted 19 lines

Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/ocfs2/journal.c +2 -0
+		}
+	}
 }
 
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
#define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
.

Joseph Qi

author

2015-05-21 03:54 UTC #7 lore raw

PATCH v2 1/1 ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

Quoted 53 lines · Junxiao Bi

Quoted 46 lines · Joseph Qi
Quoted 45 lines · Junxiao Bi
Quoted 34 lines · Joseph Qi
Quoted 33 lines · Junxiao Bi
Quoted 19 lines · Joseph Qi
jbd2_journal_dirty_metadata may fail. Currently it cannot take care of non zero return value and just BUG in ocfs2_journal_dirty. This patch is aborting the handle and journal instead of BUG.
Signed-off-by Joseph Qi <[email protected]>
Cc joyce.xue <[email protected]>
---
 fs/ocfs2/journal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
fs/ocfs2/journal.c +0 -1
index ff53192..eefca1e 100644
@@ -775,7 +775,17 @@ void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh)
 	trace_ocfs2_journal_dirty((unsigned long long)bh->b_blocknr);
 
 	status = jbd2_journal_dirty_metadata(handle, bh);
-	BUG_ON(status);
fs/ocfs2/journal.c void ocfs2_journal_dirty(handle_t *handle, struct buffer_head *bh) · line 787
Quoted 10 lines
fs/ocfs2/journal.c +10 -0
+	if (status) {
+		mlog_errno(status);
+		if (!is_handle_aborted(handle)) {
+			journal_t *journal = handle->h_transaction->t_journal;
+
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction and journal.");
+			handle->h_err = status;
+			jbd2_journal_abort_handle(handle);
+			jbd2_journal_abort(journal, status);
Let fs go after journal lose affect seemed not safe, may we set fs read-only here?
Quoted 5 lines
Quoted 4 lines
Thanks,
Junxiao.
Do you mean journal can still be updated even if it is aborted?
No, journal will not be updated. After abort, journal api will return error, at last fs will be set read-only, but there is also api which didn't return error like ocfs2_journal_dirty(), so why not stop at the first time?
Quoted 6 lines
Quoted 2 lines
Thanks,
Junxiao.
Agree with you.
But here bh can be anything like di, gd, ... As Joyce Xue reported, the problem also exists in ocfs2_abort_trigger. So I don't think we can get sb we needed.

Quoted 8 lines

Yes, right.
Quoted 1 line
Is there any other way except changing prototype of ocfs2_journal_dirty?
I can't see how to do that. From me, if jbd2_journal_dirty_metadata() return an error, there seemed a bug, BUG_ON for a bug seemed acceptable since not easy to set fs read-only from here.
Did you ever watch this BUG_ON triggered not due to a bug?

Yes, one case is jbd2_journal_dirty_metadata returns -EROFS. Another is it returns -EINVAL once transaction has changed. ocfs2 will extends transaction once credits not enough. But I think ocfs2 should take care of this case when calling jbd2_journal_restart.

Quoted 27 lines

Thanks,
Junxiao.
Quoted 15 lines
Quoted 15 lines
Quoted 13 lines
Quoted 11 lines
Quoted 8 lines
fs/ocfs2/journal.c +2 -0
+		}
+	}
 }
 
 #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
Quoted 4 lines
Quoted 2 lines
.
.