[Ocfs2-devel] [PATCH] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

[Ocfs2-devel] [PATCH] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

[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 instead of BUG.

Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/journal.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index ff53192..4482420 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -775,7 +775,15 @@ 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)) {
+			handle->h_err = status;
+			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
+					"Aborting transaction.");
+			jbd2_journal_abort_handle(handle);
+		}
+	}
 }

 #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
-- 
1.8.4.3

[Ocfs2-devel] [PATCH] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

[Xue jiufei]

Hi Joseph,
On 2015/4/28 20:23, Joseph Qi 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 instead of BUG.
> 
> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
> ---
>  fs/ocfs2/journal.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index ff53192..4482420 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -775,7 +775,15 @@ 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)) {
> +			handle->h_err = status;
> +			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
> +					"Aborting transaction.");
> +			jbd2_journal_abort_handle(handle);
The buffers dirtied before are still committed to disk while handle is aborted
and may cause some inconsistency.
Maybe the journal should also be set aborted if jbd2_journal_dirty_metadata
fails like ext4?
> +		}
> +	}
>  }
> 
>  #define OCFS2_DEFAULT_COMMIT_INTERVAL	(HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
> 

[Ocfs2-devel] [PATCH] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

[Joseph Qi]

On 2015/4/29 10:28, Xue jiufei wrote:
> Hi Joseph,
> On 2015/4/28 20:23, Joseph Qi 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 instead of BUG.
>>
>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>> ---
>>  fs/ocfs2/journal.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index ff53192..4482420 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -775,7 +775,15 @@ 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)) {
>> +			handle->h_err = status;
>> +			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
>> +					"Aborting transaction.");
>> +			jbd2_journal_abort_handle(handle);
> The buffers dirtied before are still committed to disk while handle is aborted
> and may cause some inconsistency.
> Maybe the journal should also be set aborted if jbd2_journal_dirty_metadata
> fails like ext4?

You are right. I'll take this and send a new version.
Thanks.
--Joseph

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

[Ocfs2-devel] [PATCH] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails

[Joseph Qi]

On 2015/4/29 10:28, Xue jiufei wrote:
> Hi Joseph,
> On 2015/4/28 20:23, Joseph Qi 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 instead of BUG.
>>
>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>> ---
>>  fs/ocfs2/journal.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index ff53192..4482420 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -775,7 +775,15 @@ 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)) {
>> +			handle->h_err = status;
>> +			mlog(ML_ERROR, "jbd2_journal_dirty_metadata failed. "
>> +					"Aborting transaction.");
>> +			jbd2_journal_abort_handle(handle);
> The buffers dirtied before are still committed to disk while handle is aborted
> and may cause some inconsistency.
> Maybe the journal should also be set aborted if jbd2_journal_dirty_metadata
> fails like ext4?
Maybe aborting the handle and journal is also not enough.
Though the journal is aborted, the caller still returns success. Then
the user space thinks the operation is successful, but the metadata
cannot be committed to disk due to aborted journal.
Do we also have to return the error in ocfs2_journal_dirty and then
handle it in caller? If so, it must be huge work:)

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