Message-ID: <5c8a866f-cc22-47d7-b1bb-d20abbf9c1a8@163.com>
Date: Fri, 18 Oct 2024 21:45:20 +0800
From: liubaolin <liubaolin12138@....com>
To: Jan Kara <jack@...e.cz>
Cc: tytso@....edu, adilger.kernel@...ger.ca, zhangshida@...inos.cn,
 longzhi@...gfor.com.cn, linux-ext4@...r.kernel.org,
 linux-kernel@...r.kernel.org, Baolin Liu <liubaolin@...inos.cn>
Subject: Re: [PATCH v1] ext4: fix a assertion failure due to ungranted bh
 dirting

> I'm sorry to bother you again. I checked the V2 patch you submitted.
> It seems my email was missed.
> "Baolin Liu liubaolin@...inos.cn" is my work email, not "liubaolin12138@....com."
> I apologize for the inconvenience, but please change it to "Baolin Liu <liubaolin@...inos.cn>" .
> Thank you.



在 2024/10/18 20:37, Jan Kara 写道:
> On Fri 18-10-24 19:34:31, liubaolin wrote:
>>> Sorry, I saw the patch you submitted.
>>> I would like to request a modification to the commit message.
>>> I use the email 'Baolin Liu liubaolin12138@....com' for community communication.
>>> However, my work email is 'Baolin Liu liubaolin@...inos.cn'.
>>>
>>> So I would like to ask you to modify the commit message as follows:
>>> From:
>>> Reported-by: Baolin Liu liubaolin12138@....com
>>> Reported-by: Zhi Long longzhi@...gfor.com.cn
>>> To:
>>> Reported-and-tested-by: Baolin Liu liubaolin@...inos.cn
>>> Reported-and-tested-by: Zhi Long longzhi@...gfor.com.cn
>>>
>>> Could you please make the modification? Thank you.
> 
> OK, I've sent v2 with updated tags.
> 
> 								Honza
> 
>> 在 2024/10/18 17:14, Jan Kara 写道:
>>> On Fri 18-10-24 09:48:17, liubaolin wrote:
>>>>> Hello, I am very sorry.
>>>>> I did not previously understand the approach of your patch to solve the issue.
>>>>> Yesterday, I intentionally injected faults during the quick reproduction
>>>>> test, and indeed, after applying your patch, the crash issue was
>>>>> resolved and did not occur again.
>>>>> I finally understood your approach to solving the problem. Please disregard my previous email.
>>>>> Thank you for helping me solve this crash issue in a better way.
>>>>> I still need to improve my skills in file systems, and I truly appreciate your guidance.
>>>
>>> Great! Thanks for testing. I'll send the patch for inclusion then.
>>>
>>> 								Honza
>>>
>>>> 在 2024/10/16 21:38, liubaolin 写道:
>>>>>> Hello,
>>>>>> I reviewed the patch attached in your email. The issue you mentioned
>>>>>> about clearing buffer_new(bh) in write_end_fn() is indeed a bug.
>>>>>> However, this patch does not resolve the crash issue we encountered.
>>>>>>
>>>>>> Let me explain my analysis in detail below.
>>>>>> The crash occurs in the function jbd2_journal_dirty_metadata().
>>>>>>
>>>>>> ext4_block_write_begin() -> ext4_journalled_zero_new_buffers() ->
>>>>>> write_end_fn()
>>>>>>    -> ext4_dirty_journalled_data() -> ext4_handle_dirty_metadata() ->
>>>>>> __ext4_handle_dirty_metadata()
>>>>>>    -> jbd2_journal_dirty_metadata()
>>>>>>
>>>>>> In the function jbd2_journal_dirty_metadata(), there is the
>>>>>> following condition:
>>>>>> —---------------------------------------------------------------------------------------------------
>>>>>>           if (data_race(jh->b_transaction != transaction &&
>>>>>>               jh->b_next_transaction != transaction)) {
>>>>>>                   spin_lock(&jh->b_state_lock);
>>>>>>                   J_ASSERT_JH(jh, jh->b_transaction == transaction ||
>>>>>>                                   jh->b_next_transaction == transaction);
>>>>>>                   spin_unlock(&jh->b_state_lock);
>>>>>>           }
>>>>>> ----------------------------------------------------------------------------------------------------
>>>>>> By analyzing the vmcore, I found that both jh->b_transaction and jh-
>>>>>>> b_next_transaction are NULL.
>>>>>> Through code analysis, I discovered that the
>>>>>> __jbd2_journal_file_buffer() function adds the corresponding
>>>>>> transaction of bh to jh->b_transaction.
>>>>>> Normally, this is accessed through do_journal_get_write_access(),
>>>>>> which can call __jbd2_journal_file_buffer().
>>>>>> The detailed function call process is as follows:
>>>>>> do_journal_get_write_access() -> ext4_journal_get_write_access() ->
>>>>>> __ext4_journal_get_write_access()
>>>>>>    -> jbd2_journal_get_write_access() -> do_get_write_access() ->
>>>>>> __jbd2_journal_file_buffer()
>>>>>>
>>>>>>
>>>>>> Therefore, resolving the crash issue requires obtaining write access
>>>>>> before calling the jbd2_journal_dirty_metadata() function.
>>>>>> The comment at the definition of the jbd2_journal_dirty_metadata()
>>>>>> function also states:     'The buffer must have previously had
>>>>>> jbd2_journal_get_write_access().'
>>>>>>
>>>>>> In the ext4_block_write_begin() function, if get_block() encounters
>>>>>> an error, then neither bh->b_this_page nor the subsequent bh calls
>>>>>> do_journal_get_write_access().
>>>>>> If bh->b_this_page and the subsequent bh are in the new state, it
>>>>>> will lead to a crash when reaching the jbd2_journal_dirty_metadata()
>>>>>> function.
>>>>>>
>>>>>> So, there are two ways to resolve this crash issue:
>>>>>> 1、Call do_journal_get_write_access() on bh that is not handled due
>>>>>> to get_block() error.
>>>>>>       The patch modification is in the attachment 0001-ext4-fix-a-
>>>>>> assertion-failure-due-to-ungranted-bh-dir.patch.
>>>>>>
>>>>>> 2、Call clear_buffer_new() on bh that is not handled due to
>>>>>> get_block() error.
>>>>>>       The patch modification is in the attachment 0001-ext4-fix-a-
>>>>>> assertion-failure-due-to-bh-not-clear-new.patch.
>>>>>>
>>>>>> Additionally, I have found a method to quickly reproduce this crash
>>>>>> issue.
>>>>>> For details, please refer to the email I previously sent you:
>>>>>> “https://lore.kernel.org/all/bd41c24b-7325-4584-
>>>>>> a965-392a32e32c74@....com/”.
>>>>>> I have verified that this quick reproduction method works for both
>>>>>> solutions to resolve the issue.
>>>>>>
>>>>>> Please continue to consider which method is better to resolve this
>>>>>> issue. If you think that using clear_buffer_new() is a better
>>>>>> solution, I can resend the patch via git send-mail.
>>>>>
>>>>>
>>>>>
>>>>> 在 2024/10/16 18:33, Jan Kara 写道:
>>>>>> Hello,
>>>>>>
>>>>>> On Fri 11-10-24 12:08:58, Baolin Liu wrote:
>>>>>>> Greetings，
>>>>>>>
>>>>>>> This problem is reproduced by our customer using their own testing tool
>>>>>>> “run_bug”. When I consulted with a client, the testing tool “run_bug”
>>>>>>> used a variety of background programs to benchmark (including memory
>>>>>>> pressure, cpu pressure, file cycle manipulation, fsstress Stress testing
>>>>>>> tool, postmark program，and so on).
>>>>>>>
>>>>>>> The recurrence probability is relatively low.
>>>>>>
>>>>>> OK, thanks for asking!
>>>>>>
>>>>>>> In response to your query, in ext4_block_write_begin, the new state will
>>>>>>> be clear before get block, and the bh that failed get_block will not be
>>>>>>> set to new. However, when the page size is greater than the
>>>>>>> block size, a
>>>>>>> page will contain multiple bh.
>>>>>>
>>>>>> True. I wanted to argue that the buffer_new bit should be either
>>>>>> cleared in
>>>>>> ext4_block_write_begin() (in case of error) or in
>>>>>> ext4_journalled_write_end() (in case of success) but actually
>>>>>> ext4_journalled_write_end() misses the clearing. So I think the better
>>>>>> solution is like the attached patch. I'll submit it once testing finishes
>>>>>> but it would be great if you could test that it fixes your problems as
>>>>>> well. Thanks!
>>>>>>
>>>>>>                                   Honza
>>>>
>>

Powered by blists - more mailing lists