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

[Joseph Qi]

Hi, Sorry about the late reply.
This thread is indeed a long time ago:(
It seems that I said the two ocfs2_journal_access_di() are for different
buffer head. Anyway, I have to recall the discussion before and get back
to you.

Thanks,
Joseph

On 2/10/23 6:04 PM, Heming Zhao wrote:
> 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