Message-ID: <8c14e5b0-5229-4611-b8e6-434c6eb34ee9@163.com>
Date: Fri, 18 Oct 2024 09:48:17 +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

> 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.



在 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