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 г. 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 and isn't this premature
optimization, have you done any measurements?


> ---
>  extent-tree.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index 932af2c644bd..24d3a1ab3f25 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -1533,10 +1533,34 @@ static int write_one_cache_group(struct btrfs_trans_handle *trans,
>  	unsigned long bi;
>  	struct extent_buffer *leaf;
>  
> +	/* Do a read only check to see if we need to update BLOCK_GROUP_ITEM */
> +	ret = btrfs_search_slot(NULL, extent_root, &cache->key, path, 0, 0);
> +	if (ret < 0)
> +		goto fail;
> +	if (ret > 0) {
> +		ret = -ENOENT;
> +		error("failed to find block group %llu in extent tree",
> +			cache->key.objectid);
> +		goto fail;
> +	}
> +	leaf = path->nodes[0];
> +	bi = btrfs_item_ptr_offset(leaf, path->slots[0]);
> +	ret = memcmp_extent_buffer(leaf, &cache->item, bi, sizeof(cache->item));
> +	btrfs_release_path(path);
> +	/* No need to update */
> +	if (ret == 0)
> +		return ret;
> +
> +	/* Do the COW to update BLOCK_GROUP_ITEM */
>  	ret = btrfs_search_slot(trans, extent_root, &cache->key, path, 0, 1);
>  	if (ret < 0)
>  		goto fail;
> -	BUG_ON(ret);
> +	if (ret > 0) {
> +		ret = -ENOENT;
> +		error("failed to find block group %llu in extent tree",
> +			cache->key.objectid);
> +		goto fail;
> +	}
>  
>  	leaf = path->nodes[0];
>  	bi = btrfs_item_ptr_offset(leaf, path->slots[0]);
>