Re: [PATCH] block_dump: don't put the last refcount when marking inode dirty

From: "zhangyi (F)" <yi.zhang@huawei.com>
To: Jan Kara <jack@suse.cz>
Cc: <linux-fsdevel@vger.kernel.org>, <linux-ext4@vger.kernel.org>,
	<tytso@mit.edu>, <viro@zeniv.linux.org.uk>,
	<linfeilong@huawei.com>, Ye Bin <yebin10@huawei.com>
Subject: Re: [PATCH] block_dump: don't put the last refcount when marking inode dirty
Date: Thu, 4 Mar 2021 21:37:42 +0800	[thread overview]
Message-ID: <5f72dc70-9fb0-0d3b-dc31-f60d35929991@huawei.com> (raw)
In-Reply-To: <20210301112102.GD25026@quack2.suse.cz>

On 2021/3/1 19:21, Jan Kara wrote:
> On Fri 26-02-21 18:31:03, zhangyi (F) wrote:
>> There is an AA deadlock problem when using block_dump on ext4 file
>> system with data=journal mode.
>>
>>   watchdog: BUG: soft lockup - CPU#19 stuck for 22s! [jbd2/pmem0-8:1002]
>>   CPU: 19 PID: 1002 Comm: jbd2/pmem0-8
>>   RIP: 0010:queued_spin_lock_slowpath+0x60/0x3b0
>>   ...
>>   Call Trace:
>>    _raw_spin_lock+0x57/0x70
>>    jbd2_journal_invalidatepage+0x166/0x680
>>    __ext4_journalled_invalidatepage+0x8c/0x120
>>    ext4_journalled_invalidatepage+0x12/0x40
>>    truncate_cleanup_page+0x10e/0x1c0
>>    truncate_inode_pages_range+0x2c8/0xec0
>>    truncate_inode_pages_final+0x41/0x90
>>    ext4_evict_inode+0x254/0xac0
>>    evict+0x11c/0x2f0
>>    iput+0x20e/0x3a0
>>    dentry_unlink_inode+0x1bf/0x1d0
>>    __dentry_kill+0x14c/0x2c0
>>    dput+0x2bc/0x630
>>    block_dump___mark_inode_dirty.cold+0x5c/0x111
>>    __mark_inode_dirty+0x678/0x6b0
>>    mark_buffer_dirty+0x16e/0x1d0
>>    __jbd2_journal_temp_unlink_buffer+0x127/0x1f0
>>    __jbd2_journal_unfile_buffer+0x24/0x80
>>    __jbd2_journal_refile_buffer+0x12f/0x1b0
>>    jbd2_journal_commit_transaction+0x244b/0x3030
>>
>> The problem is a race between jbd2 committing data buffer and user
>> unlink the file concurrently. The jbd2 will get jh->b_state_lock and
>> redirty the inode's data buffer and inode itself. If block_dump is
>> enabled, it will try to find inode's dentry and invoke the last dput()
>> after the inode was unlinked. Then the evict procedure will unmap
>> buffer and get jh->b_state_lock again in journal_unmap_buffer(), and
>> finally lead to deadlock. It works fine if block_dump is not enabled
>> because the last evict procedure is not invoked in jbd2 progress and
>> the jh->b_state_lock will also prevent inode use after free.
>>
>> jbd2                                xxx
>>                                     vfs_unlink
>>                                      ext4_unlink
>> jbd2_journal_commit_transaction
>> **get jh->b_state_lock**
>> jbd2_journal_refile_buffer
>>  mark_buffer_dirty
>>   __mark_inode_dirty
>>    block_dump___mark_inode_dirty
>>     d_find_alias
>>                                      d_delete
>>                                       unhash
>>     dput  //put the last refcount
>>      evict
>>       journal_unmap_buffer
>>        **get jh->b_state_lock again**
>>
>> In most cases of where invoking mark_inode_dirty() will get inode's
>> refcount and the last iput may not happen, but it's not safe. After
>> checking the block_dump code, it only want to dump the file name of the
>> dirty inode, so there is no need to get and put denrty, and dump an
>> unhashed dentry is also fine. This patch remove the dget() && dput(),
>> print the dentry name directly.
>>
>> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
>> Signed-off-by: yebin (H) <yebin10@huawei.com>
> 
> Hrm, ok. Honestly, I wanted to just delete that code for a long time. IMO
> tracepoints (and we have one in __mark_inode_dirty) are much more useful
> for tracing anyway. This code exists only because it was there much before
> tracepoints existed... Do you have a strong reason why are you using
> block_dump instead of tracepoint trace_writeback_mark_inode_dirty() for
> your monitoring?
> 

Hi, Jan. We just do some stress tests and find this issue, I'm not sure who
are still using this old debug interface and gather it may need time. Could
we firstly fix this issue, and then delete this code if no opposed?

Thanks,
Yi.