Re: [PATCH v2] btrfs: Batch up release of reserved metadata for delayed items used for deletion

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Jun 17, 2022 at 02:36:30PM +0300, Nikolay Borisov wrote:
> With Filipe's recent rework of the delayed inode code one aspect which
> isn't batched is the release of the reserved metadata of delayed inode's
> delete items. With this patch on top of Filipe's rework and running the
> same test as provided in the description of a patch titled
> "btrfs: improve batch deletion of delayed dir index items" I observe
> the following change of the number of calls to btrfs_block_rsv_release:
> 
> Before this change:
> @block_rsv_release: 1004
> @btrfs_delete_delayed_items_total_time: 14602
> @delete_batches: 505
> 
> After:
> @block_rsv_release: 510
> @btrfs_delete_delayed_items_total_time: 13643
> @delete_batches: 507
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V2:
>  * Improved subject wording to make it more clear (Filipe)
> 
>  * Print the inode number in the tracepoint (Filipe)
> 
>  * More explicit referal to the test case in Filipe's initial patch to make it
>  more clear how those numbers are derived.
> 
>  fs/btrfs/delayed-inode.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index e1e856436ad5..6c06ddba5a7a 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -800,11 +800,13 @@ static int btrfs_batch_delete_items(struct btrfs_trans_handle *trans,
>  				    struct btrfs_path *path,
>  				    struct btrfs_delayed_item *item)
>  {
> +	struct btrfs_fs_info *fs_info = root->fs_info;
>  	struct btrfs_delayed_item *curr, *next;
>  	struct extent_buffer *leaf = path->nodes[0];
>  	LIST_HEAD(batch_list);
>  	int nitems, slot, last_slot;
>  	int ret;
> +	u64 total_reserved_size = item->bytes_reserved;
> 
>  	ASSERT(leaf != NULL);
> 
> @@ -841,14 +843,23 @@ static int btrfs_batch_delete_items(struct btrfs_trans_handle *trans,
>  		nitems++;
>  		curr = next;
>  		list_add_tail(&curr->tree_list, &batch_list);
> +		total_reserved_size += curr->bytes_reserved;
>  	}
> 
>  	ret = btrfs_del_items(trans, root, path, path->slots[0], nitems);
>  	if (ret)
>  		return ret;
> 
> +	/*
> +	 * Check btrfs_delayed_item_reserve_metadata() to see why we don't need
> +	 * to release/reserve qgroup space.
> +	 */
> +	trace_btrfs_space_reservation(fs_info, "delayed_item", 0,
> +				      total_reserved_size, 0);

Still using 0 instead of item->key.objectid, just like v1.

Also, I forgot to point out in v1, these calls should be done under a:

   if (!test_bit(BTRFS_FS_LOG_RECOVERING, &fs_info->flags))

(or total_reserved_size > 0)

Thanks.

> +	btrfs_block_rsv_release(fs_info, &fs_info->delayed_block_rsv,
> +				total_reserved_size, NULL);
> +
>  	list_for_each_entry_safe(curr, next, &batch_list, tree_list) {
> -		btrfs_delayed_item_release_metadata(root, curr);
>  		list_del(&curr->tree_list);
>  		btrfs_release_delayed_item(curr);
>  	}
> --
> 2.25.1
>