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

Nikolay Borisov
2019-07-08T10:44:00Z

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]);
>