Re: [PATCH] btrfs: make btrfs_dirty_inode() to always reserve metadata space

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Nikolay Borisov <nborisov@suse.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: make btrfs_dirty_inode() to always reserve metadata space
Date: Tue, 12 Jan 2021 20:29:04 +0800	[thread overview]
Message-ID: <ad973dfe-0ff8-72c2-9b22-770999f3f379@gmx.com> (raw)
In-Reply-To: <e68db844-ec96-0ab4-1654-bd8cea6e78e0@suse.com>

On 2021/1/12 下午8:19, Nikolay Borisov wrote:
>
>
> On 8.01.21 г. 7:36 ч., Qu Wenruo wrote:
>> There are several qgroup flush related bugs fixed recently, all of them
>> are caused by the fact that we can trigger qgroup metadata space
>> reservation holding a transaction handle.
>>
>> Thankfully the only situation to trigger above reservation is
>> btrfs_dirty_inode().
>>
>> Currently btrfs_dirty_inode() will try join transactio first, then
>> update the inode.
>> If btrfs_update_inode() fails with -ENOSPC, then it retry to start
>> transaction to reserve metadata space.
>>
>> This not only forces us to reserve metadata space with a transaction
>> handle hold, but can't handle other errors like -EDQUOT.
>>
>> This patch will make btrfs_dirty_inode() to call
>> btrfs_start_transaction() directly without first try joining then
>> starting, so that in try_flush_qgroup() we won't hold a trans handle.
>>
>> This will slow down btrfs_dirty_inode() but my fstests doesn't show too
>> much different for most test cases, thus it may be worthy to skip such
>> performance "optimization".
>
> btrfs_dirty_inode is called everytime file_update_time is called or
> atime is updated. Given the default mount option is to use lazy a_time
> I'd say we needn't worry about it. file_update_time, however, is called
> within the write path so this change potentially makes file write more
> expensive. So instead of testing with fstests it needs proper
> benchmarking with fio.

Thanks for the benchmark tool.

One of my biggest question is what tool should be used to properly
benchmark the patch.

Although I don't have extra physical machine, at least I can test with a
VM with cache=none mode using a dedicated SSD for this case.

Will update the patch to include the new benchmark result.

Thanks,
Qu

>
> <snip>
>