Re: [PATCH v2 2/2] btrfs-progs: Avoid unnecessary block group item COW if the content hasn't changed

Nikolay Borisov
2019-07-08T13:08:04Z

On 8.07.19 г. 15:50 ч., Qu Wenruo wrote:
>
>
> On 2019/7/8 下午6:43, Nikolay Borisov wrote:
>>
>>
>> On 8.07.19 г. 10:33 ч., Qu Wenruo wrote:
>>> In write_one_cache_group() we always do COW to update BLOCK_GROUP_ITEM.
>>> However under a lot of cases, the cache->item is not changed at all.
>>>
>>> E.g:
>>> Transaction 123, block group [1M, 1M + 16M)
>>>
>>> tree block 1M + 0 get freed
>>> tree block 1M + 16K get allocated.
>>>
>>> Transaction 123 get committed.
>>>
>>> In this case, used space of block group [1M, 1M + 16M) doesn't changed
>>> at all, thus we don't need to do COW to update block group item.
>>>
>>> This patch will make write_one_cache_group() to do a read-only search
>>> first, then do COW if we really need to update block group item.
>>>
>>> This should reduce the btrfs_write_dirty_block_groups() and
>>> btrfs_run_delayed_refs() loop introduced in previous commit.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> I'm not sure how effective this is going to be
>
> The effectiveness is indeed low.
>
> For btrfs/013 test case, 64K page size, it reduces total number of
> delayed refs by less than 2% (5/300+)
>
> And similar result for total number of dirty block groups.
>
>> and isn't this premature
>> optimization, have you done any measurements?
>
> For the optimization part, I'd say it should be pretty safe.
> It just really skips unnecessary CoW.
>
> The only downside to me is the extra tree search, thus killing the
> "optimization" part.
>

If that's the case then I'd rather see the 2nd patch dropped. It adds
more code for no gain.

<snip>