* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
@ 2015-05-12 1:53 Joseph Qi
2015-05-12 23:03 ` Andrew Morton
2015-05-20 8:22 ` Junxiao Bi
0 siblings, 2 replies; 8+ messages in thread
From: Joseph Qi @ 2015-05-12 1:53 UTC (permalink / raw
To: ocfs2-devel
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 <joseph.qi@huawei.com>
Cc: joyce.xue <xuejiufei@huawei.com>
---
fs/ocfs2/journal.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index ff53192..eefca1e 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -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)
--
1.8.4.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-12 1:53 [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails Joseph Qi
@ 2015-05-12 23:03 ` Andrew Morton
2015-05-20 8:22 ` Junxiao Bi
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2015-05-12 23:03 UTC (permalink / raw
To: ocfs2-devel
On Tue, 12 May 2015 09:53:47 +0800 Joseph Qi <joseph.qi@huawei.com> 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.
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -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.
> 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);
> + }
> + }
> }
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-12 1:53 [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails Joseph Qi
2015-05-12 23:03 ` Andrew Morton
@ 2015-05-20 8:22 ` Junxiao Bi
2015-05-20 10:25 ` Joseph Qi
1 sibling, 1 reply; 8+ messages in thread
From: Junxiao Bi @ 2015-05-20 8:22 UTC (permalink / raw
To: ocfs2-devel
On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>
> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
> Cc: joyce.xue <xuejiufei@huawei.com>
> ---
> fs/ocfs2/journal.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
> index ff53192..eefca1e 100644
> --- a/fs/ocfs2/journal.c
> +++ b/fs/ocfs2/journal.c
> @@ -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);
Let fs go after journal lose affect seemed not safe, may we set fs
read-only here?
Thanks,
Junxiao.
> + }
> + }
> }
>
> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-20 8:22 ` Junxiao Bi
@ 2015-05-20 10:25 ` Joseph Qi
2015-05-20 11:09 ` Junxiao Bi
0 siblings, 1 reply; 8+ messages in thread
From: Joseph Qi @ 2015-05-20 10:25 UTC (permalink / raw
To: ocfs2-devel
On 2015/5/20 16:22, Junxiao Bi wrote:
> On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>>
>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>> Cc: joyce.xue <xuejiufei@huawei.com>
>> ---
>> fs/ocfs2/journal.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>> index ff53192..eefca1e 100644
>> --- a/fs/ocfs2/journal.c
>> +++ b/fs/ocfs2/journal.c
>> @@ -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);
> Let fs go after journal lose affect seemed not safe, may we set fs
> read-only here?
>
> Thanks,
> Junxiao.
>
>
Do you mean journal can still be updated even if it is aborted?
>> + }
>> + }
>> }
>>
>> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-20 10:25 ` Joseph Qi
@ 2015-05-20 11:09 ` Junxiao Bi
2015-05-21 0:49 ` Joseph Qi
0 siblings, 1 reply; 8+ messages in thread
From: Junxiao Bi @ 2015-05-20 11:09 UTC (permalink / raw
To: ocfs2-devel
On 05/20/2015 06:25 PM, Joseph Qi wrote:
> On 2015/5/20 16:22, Junxiao Bi wrote:
>> On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>>>
>>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>>> Cc: joyce.xue <xuejiufei@huawei.com>
>>> ---
>>> fs/ocfs2/journal.c | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>> index ff53192..eefca1e 100644
>>> --- a/fs/ocfs2/journal.c
>>> +++ b/fs/ocfs2/journal.c
>>> @@ -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);
>> Let fs go after journal lose affect seemed not safe, may we set fs
>> read-only here?
>>
>> 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.
>
>>> + }
>>> + }
>>> }
>>>
>>> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>>>
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-20 11:09 ` Junxiao Bi
@ 2015-05-21 0:49 ` Joseph Qi
2015-05-21 2:09 ` Junxiao Bi
0 siblings, 1 reply; 8+ messages in thread
From: Joseph Qi @ 2015-05-21 0:49 UTC (permalink / raw
To: ocfs2-devel
On 2015/5/20 19:09, Junxiao Bi wrote:
> On 05/20/2015 06:25 PM, Joseph Qi wrote:
>> On 2015/5/20 16:22, Junxiao Bi wrote:
>>> On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>>>>
>>>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>>>> Cc: joyce.xue <xuejiufei@huawei.com>
>>>> ---
>>>> fs/ocfs2/journal.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>>> index ff53192..eefca1e 100644
>>>> --- a/fs/ocfs2/journal.c
>>>> +++ b/fs/ocfs2/journal.c
>>>> @@ -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);
>>> Let fs go after journal lose affect seemed not safe, may we set fs
>>> read-only here?
>>>
>>> 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.
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?
>>
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>>>>
>>>
>>>
>>>
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-21 0:49 ` Joseph Qi
@ 2015-05-21 2:09 ` Junxiao Bi
2015-05-21 3:54 ` Joseph Qi
0 siblings, 1 reply; 8+ messages in thread
From: Junxiao Bi @ 2015-05-21 2:09 UTC (permalink / raw
To: ocfs2-devel
On 05/21/2015 08:49 AM, Joseph Qi wrote:
> On 2015/5/20 19:09, Junxiao Bi wrote:
>> On 05/20/2015 06:25 PM, Joseph Qi wrote:
>>> On 2015/5/20 16:22, Junxiao Bi wrote:
>>>> On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>>>>>
>>>>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>>>>> Cc: joyce.xue <xuejiufei@huawei.com>
>>>>> ---
>>>>> fs/ocfs2/journal.c | 12 +++++++++++-
>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>>>> index ff53192..eefca1e 100644
>>>>> --- a/fs/ocfs2/journal.c
>>>>> +++ b/fs/ocfs2/journal.c
>>>>> @@ -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);
>>>> Let fs go after journal lose affect seemed not safe, may we set fs
>>>> read-only here?
>>>>
>>>> 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.
> 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.
> 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.
>
>>>
>>>>> + }
>>>>> + }
>>>>> }
>>>>>
>>>>> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails
2015-05-21 2:09 ` Junxiao Bi
@ 2015-05-21 3:54 ` Joseph Qi
0 siblings, 0 replies; 8+ messages in thread
From: Joseph Qi @ 2015-05-21 3:54 UTC (permalink / raw
To: ocfs2-devel
On 2015/5/21 10:09, Junxiao Bi wrote:
> On 05/21/2015 08:49 AM, Joseph Qi wrote:
>> On 2015/5/20 19:09, Junxiao Bi wrote:
>>> On 05/20/2015 06:25 PM, Joseph Qi wrote:
>>>> On 2015/5/20 16:22, Junxiao Bi wrote:
>>>>> On 05/12/2015 09:53 AM, 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 and journal instead of BUG.
>>>>>>
>>>>>> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
>>>>>> Cc: joyce.xue <xuejiufei@huawei.com>
>>>>>> ---
>>>>>> fs/ocfs2/journal.c | 12 +++++++++++-
>>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>>>>> index ff53192..eefca1e 100644
>>>>>> --- a/fs/ocfs2/journal.c
>>>>>> +++ b/fs/ocfs2/journal.c
>>>>>> @@ -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);
>>>>> Let fs go after journal lose affect seemed not safe, may we set fs
>>>>> read-only here?
>>>>>
>>>>> 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.
>> 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.
>
>> 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.
> Thanks,
> Junxiao.
>
>
>>
>>>>
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> #define OCFS2_DEFAULT_COMMIT_INTERVAL (HZ * JBD2_DEFAULT_MAX_COMMIT_AGE)
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-21 3:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-12 1:53 [Ocfs2-devel] [PATCH v2] ocfs2: do not BUG if jbd2_journal_dirty_metadata fails Joseph Qi
2015-05-12 23:03 ` Andrew Morton
2015-05-20 8:22 ` Junxiao Bi
2015-05-20 10:25 ` Joseph Qi
2015-05-20 11:09 ` Junxiao Bi
2015-05-21 0:49 ` Joseph Qi
2015-05-21 2:09 ` Junxiao Bi
2015-05-21 3:54 ` Joseph Qi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.