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

[Nikolay Borisov <nborisov@suse.com>]

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>