[Ocfs2-devel] [PATCH V2 2/2] ocfs2: clear journal dirty flag after shutdown journal

[Joseph Qi]

On 18/11/20 07:26, Junxiao Bi wrote:
> Hi Joseph,
> 
> On 11/19/18 8:34 PM, Joseph Qi wrote:
>> Hi Junxiao,
>>
>> On 18/11/19 16:07, Junxiao Bi wrote:
>>> Dirty flag of the journal should be cleared at the last stage of umount,
>>> if do it before jbd2_journal_destroy(), then some metadata in uncommitted
>>> transaction could be lost due to io error, but as dirty flag of journal
>>> was already cleared, we can't find that until run a full fsck. This may
>>> cause system panic or other corruption.
>>>
>>> Signed-off-by: Junxiao Bi <junxiao.bi at oracle.com>
>>> Cc: Yiwen Jiang <jiangyiwen at huawei.com>
>>> Cc: Jun Piao <piaojun at huawei.com>
>>> ---
>>>   fs/ocfs2/journal.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>>   V1 -> V2:
>>>   pointed by Yiwen, need check return value of jbd2_journal_destroy
>>>
>>> diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
>>> index 13f8e097babf..b51bb873441f 100644
>>> --- a/fs/ocfs2/journal.c
>>> +++ b/fs/ocfs2/journal.c
>>> @@ -986,7 +986,8 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>>               mlog_errno(status);
>>>       }
>>>   -    if (status == 0) {
>>> +    /* Shutdown the kernel journal system */
>>> +    if (!jbd2_journal_destroy(journal->j_journal) && !status) {
>>>           /*
>>>            * Do not toggle if flush was unsuccessful otherwise
>>>            * will leave dirty metadata in a "clean" journal
>>> @@ -995,9 +996,6 @@ void ocfs2_journal_shutdown(struct ocfs2_super *osb)
>>>           if (status < 0)
>>>               mlog_errno(status);
>>>       }
>>> -
>>> -    /* Shutdown the kernel journal system */
>>> -    jbd2_journal_destroy(journal->j_journal);
>> Now we will write journal inode after journal has been destroyed.
>> I wonder if it the right way as expected.
> 
> The destroyed journal here was managed by jbd2 and located in the data section of ocfs2 journal inode, after clean up the data, clear flag in the inode, this seemed right way to go.
> 
It makes sense.

Reviewed-by: Joseph Qi <jiangqi903 at gmail.com>

> Thanks,
> 
> Junxiao.
> 
>>
>> Thanks,
>> Joseph
>>
>>>       journal->j_journal = NULL;
>>>         OCFS2_I(inode)->ip_open_count--;
>>>