[Ocfs2-devel] [PATCH 2/2] ocfs2: add error handling path when jbd2 enter ABORT status

[Joseph Qi]

On 5/4/23 4:02 PM, Heming Zhao wrote:
> On Thu, May 04, 2023 at 03:34:49PM +0800, Joseph Qi wrote:
>>
>>
>> On 5/4/23 2:21 PM, Heming Zhao wrote:
>>> On Thu, May 04, 2023 at 10:27:46AM +0800, Joseph Qi wrote:
>>>>
>>>>
>>>> On 4/30/23 11:13 AM, Heming Zhao wrote:
>>>>> fstest generic cases 347 361 628 629 trigger a same issue:
>>>>> When jbd2 enter ABORT status, ocfs2 ignores it and keep going to commit
>>>>> journal.
>>>>
>>>> What's the end user impact?
>>>
>>> There are two impacts:
>>>
>>> 1. this issue makes unmount command hanging.
>>>
>>> No one likes or accepts filesystem unmount failure when underlying devices meets
>>> error. For comparison, other FSs (e.g. ext4, xfs, gfs2, ..) could do unmount
>>> successfully.
>>>
>>
>> So umount hang should be in the patch description, right?
> 
> OK.
> 
>>
>>> 2. for developers to verify their patch doesn't make any regression.
>>>
>>> fstest is a famious & important fs testsuite.
>>> (Yes, I know ocfs2 has itself testsuite ocfs2_test.)
>>>
>>> Current status, there are many test cases (about 9 in my env) cause fstest
>>> hanging and blocking fstest to run. I did test for gfs2 on latest tumbleweed,
>>> gfs2 only has 1 hanging case.
>>>
>>> In my view, ocfs2 developers or maintainers at least make ocfs2 to finish the
>>> testsuite. Long-term target is to make ocfs2 to pass all the testsuite.
>>>
>>> On kernel 6.2.9-1, the fstest 'quick' test group result:
>>> (the result based on my two patches [1/2] & [2/2])
>>> ```
>>> ./check -g quick -T -b -s ocfs2 -e generic/081 -e generic/648
>>>
>>> Failures: generic/003 generic/013 generic/015 generic/040 generic/041
>>> generic/062 generic/082 generic/104 generic/107 generic/228 generic/244
>>> generic/266 generic/272 generic/277 generic/281 generic/322 generic/329
>>> generic/331 generic/336 generic/343 generic/376 generic/379 generic/380
>>> generic/383 generic/384 generic/385 generic/386 generic/400 generic/410
>>> generic/424 generic/441 generic/448 generic/449 generic/471 generic/479
>>> generic/493 generic/495 generic/510 generic/537 generic/552 generic/563
>>> generic/578 generic/594 generic/607 generic/620 generic/628 generic/630
>>> generic/636 generic/702
>>> Failed 49 of 568 tests
>>> ```
>>>
>>>>
>>>> JBD2_ABORT indicates a fatal error happens, either in jounral layer or
>>>> filesystem. And we should not commit any further transactions.
>>>> It seems that we may unify the behavior like:
>>>>
>>>> if (is_journal_aborted(journal))
>>>> 	return -EROFS;
>>>>
>>>>
>>> IIUC, do you mean add above code in ocfs2_commit_cache or in
>>> ocfs2_commit_thread?
>>
>> Yes, break the loop in ocfs2_commit_thread() in case of journal abort.
>> Actually we've handled this case before, but just limit the print. But
>> it seems not enough now.
> 
> I wrote in my previous mail. Follow your idea, The code should be:
> 
> if (is_journal_aborted(journal)) {
> 	ocfs2_error(osb->sb, "jbd2 status: ABORT.\n"); //this line is important.
> 	return -EROFS;
> }
> 
> Only return -EROFS then break loop in ocfs2_commit_thread() is not enough.
> Without ocfs2_error(), ocfs2 could accept new IOs from userspace, then the new
> IOs will trigger IO error then trigger RO status. This flow is wrong, we should
> mark RO as early as possible when JBD ABORT happens. In my view, the best place
> is my [2/2] patch code which in ocfs2_commit_cache().
> 

Agree, but ocfs2_abort() is more appropriate here, see
ocfs2_start_trans(). But ocfs2_abort() may panic system, I'm afraid it
has to change to read-only accordingly.

>> BTW, the basic rule here is, we don't want to change journal to prevent
>> other nodes corrupting the filesystem.
> 
> If my memory is correct, every node has itself special journal partition.
> If the HA stack assigns the same (before fenced) nodeid to JBD ABORTed machine, 
> No other node could corrupting the fs.
> 
I don't think it's a good idea to modify journal even flush fails. Why
not catch EROFS in commit thread and break? Now we can only expect
umount continues with error, right?