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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2019/7/8 下午9:07, Nikolay Borisov wrote:
>
>
> 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.

Makes sense. I'm OK to drop it.

Thanks,
Qu
>
> <snip>
>