Re: [PATCH 04/12] btrfs: add assertions when deleting batches of delayed items

[Anand Jain <anand.jain@oracle.com>]

On 5/31/22 20:36, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There are a few impossible cases that btrfs_batch_delete_items() tries to
> deal with:
> 
> 1) Getting a path pointing to a NULL leaf;
> 2) The leaf slot is pointing beyond the last item in the leaf;
> 3) We can't find a single item to delete.
> 
> The first case is impossible because the given path was returned by a
> successful call to btrfs_search_slot(). Replace the BUG_ON() with an
> ASSERT for this.
> 
> The second case is impossible because we are always called when a delayed
> item matches an item in the given leaf. So add an ASSERT() for that and
> if that condition is not satisfied, trigger a warning and return an error.
> 
> The third case is impossible exactly because of the same reason as the
> second case. The given delayed item matches one item in the leaf, so we
> know that our batch always has at least one item. Add an ASSERT to check
> that, trigger a warning if that expectation fails and return an error.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

LGTM

Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/delayed-inode.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 66779ab3ed4a..bb9955e74a51 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -790,20 +790,23 @@ static int btrfs_batch_delete_items(struct btrfs_trans_handle *trans,
>   				    struct btrfs_delayed_item *item)
>   {
>   	struct btrfs_delayed_item *curr, *next;
> -	struct extent_buffer *leaf;
> +	struct extent_buffer *leaf = path->nodes[0];
>   	struct btrfs_key key;
>   	struct list_head head;
>   	int nitems, i, last_item;
>   	int ret = 0;
>   
> -	BUG_ON(!path->nodes[0]);
> -
> -	leaf = path->nodes[0];
> +	ASSERT(leaf != NULL);
>   
>   	i = path->slots[0];
>   	last_item = btrfs_header_nritems(leaf) - 1;
> -	if (i > last_item)
> -		return -ENOENT;	/* FIXME: Is errno suitable? */
> +	/*
> +	 * Our caller always gives us a path pointing to an existing item, so
> +	 * this can not happen.
> +	 */
> +	ASSERT(i <= last_item);
> +	if (WARN_ON(i > last_item))
> +		return -ENOENT;
>   
>   	next = item;
>   	INIT_LIST_HEAD(&head);
> @@ -830,8 +833,13 @@ static int btrfs_batch_delete_items(struct btrfs_trans_handle *trans,
>   		btrfs_item_key_to_cpu(leaf, &key, i);
>   	}
>   
> -	if (!nitems)
> -		return 0;
> +	/*
> +	 * Our caller always gives us a path pointing to an existing item, so
> +	 * this can not happen.
> +	 */
> +	ASSERT(nitems >= 1);
> +	if (nitems < 1)
> +		return -ENOENT;
>   
>   	ret = btrfs_del_items(trans, root, path, path->slots[0], nitems);
>   	if (ret)