Re: [PATCH] btrfs: Batch up release of delayed inode metadata

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Jun 16, 2022 at 04:35:45PM +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 batch on top of his rework I see 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

Placing here some numbers without any context is puzzling. Either paste
the test and bpftrace script here, or mention it's for the same test and
bpftrace script from the changelog of another patch (mentioning its subject).

> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  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);

The tracepoint should include the number of the associated inode (item->key.objectid),
just like every where else, so that during debugging we can match the release with
the respective reservation call.

Also the subject is misleading: it's not the release of delayed inode metadata,
it's the release of reserved space for delayed items used for deletion.

Otherwise it looks fine, 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
>