[Ocfs2-devel] discuss about jbd2 assertion in defragment path - Heming Zhao via Ocfs2-devel

From: Heming Zhao via Ocfs2-devel <ocfs2-devel@oss.oracle.com>
To: joseph.qi@linux.alibaba.com, ocfs2-devel@oss.oracle.com
Cc: jack@suse.com
Subject: [Ocfs2-devel] discuss about jbd2 assertion in defragment path
Date: Fri, 10 Feb 2023 18:04:57 +0800	[thread overview]
Message-ID: <20230210100457.xv5wchadnd7kkazb@c73> (raw)

Hello Joseph,

I am sorry to wake up a long time ago thread.

All mails of this thread (my patch is [1]):
[1] https://oss.oracle.com/pipermail/ocfs2-devel/2022-May/000101.html
[2] https://oss.oracle.com/pipermail/ocfs2-devel/2022-June/000105.html
[3] https://oss.oracle.com/pipermail/ocfs2-devel/2022-June/000109.html
[4] https://oss.oracle.com/pipermail/ocfs2-devel/2022-June/000217.html

I re-checked ocfs2 defragmentation & jbd2 flow recently, I still think my
patch [1] is right. At least, the fixing code is correct, the patch commit
log needs to polish.

This bug has the same root cause of commit 7f27ec978b0e ("ocfs2: call
ocfs2_journal_access_di() before ocfs2_journal_dirty() in ocfs2_write_end_nolock()").
For this bug, jbd2_journal_restart() is called by ocfs2_split_extent() during
defragmenting, and it's not about "not enough credits" issue you ever said in [2].

I explain my thinking again in this mail.

the crash call flow:

ocfs2_defrag_extent //caller call it in while() loop.
 + handle = ocfs2_start_trans(osb, credits)
 + __ocfs2_move_extent
 |  + ocfs2_journal_access_di //[a]
 |  + ocfs2_split_extent      //[b]
 |  |  + if                   //[b.1]
 |  |  |   ocfs2_replace_extent_rec/ocfs2_split_and_insert
 |  |  + else
 |  |     ocfs2_try_to_merge_extent
 |  |
 |  + ocfs2_journal_dirty     //[c]
 |
 + ocfs2_commit_trans(osb, handle) //<== complete this handle

In my viewpoint, ocfs2_split_extent() is journal self-service function. I still
belive the two lines ([a] & [c]) in __ocfs2_move_extent() are totally useless.
In ocfs2_split_extent(), the code from the first code line to "if-else" code
area ([b.1]) doesn't need any journal protection, and we also could see there
are only read operations.

If we worry about data corruption after removing [a] & [c], (e.g: my eyes missed
some journal operations from [a] to [b.1]), we could only delete [c]. So the
fixed code seems (only remove line [c]):

ocfs2_defrag_extent
 + handle = ocfs2_start_trans(osb, credits)
 + __ocfs2_move_extent
 |  + ocfs2_journal_access_di //[a]  <-- keep it, but remove pair dirty action
 |  + ocfs2_split_extent      //[b]
 |     + if                   //[b.1]
 |     |   ocfs2_replace_extent_rec/ocfs2_split_and_insert
 |     + else
 |        ocfs2_try_to_merge_extent
 |
 + ocfs2_commit_trans(osb, handle)

Thanks,
Heming

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel