Re: [PATCH 05/12] btrfs: deal with deletion errors when deleting delayed items

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

On 5/31/22 20:36, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Currently, btrfs_delete_delayed_items() ignores any errors returned from
> btrfs_batch_delete_items(). This looks fishy but it's not a problem at
> the moment because:
> 
> 1) Two of the errors returned from btrfs_batch_delete_items() are for
>     impossible cases, cases where a delayed item does not match any item
>     in the leaf the path points to - btrfs_delete_delayed_items() always
>     calls btrfs_batch_delete_items() with a path that points to a leaf
>     that contains an item matching a delayed item;
> 
> 2) btrfs_batch_delete_items() may return an error from btrfs_del_items(),
>     in which case it does not release the delayed items of the batch.
> 
>     At the moment this is harmless because btrfs_del_items() actually is
>     always able to delete items, even if it returns an error - when it
>     returns an error it's because it ended up with a leaf mostly empty
>     (less than 1/3 full) and failed to migrate items from that leaf into
>     its neighbour leaves - this is not critical, as all the items were
>     deleted, we just left the tree a bit unbalanced, but it's still a
>     valid tree and causes no harm, and future operations on the tree will
>     eventually balance it.
> 
>     So even if we get an error from btrfs_del_items(), the delayed items
>     will not be released but the next time we run delayed items we will
>     find out, at btrfs_delete_delayed_items(), that they are not present
>     in the tree anymore and then release them.
> 
> This is all a bit subtle, and it's certainly prone to be a disaster in
> case btrfs_del_items() changes one day and may return errors before being
> able to delete all the requested items, in which case we could leave the
> filesystem in an inconsistent state as we would commit a transaction
> despite a failure from deleting items from the tree.
> 
> So make btrfs_delete_delayed_items() check for any erros from the call to
> btrfs_batch_delete_items().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

LGTM
Reviewed-by: Anand Jain <anand.jain@oracle.com>



> ---
>   fs/btrfs/delayed-inode.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index bb9955e74a51..3c6368c29bb9 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -889,7 +889,9 @@ static int btrfs_delete_delayed_items(struct btrfs_trans_handle *trans,
>   			goto delete_fail;
>   	}
>   
> -	btrfs_batch_delete_items(trans, root, path, curr);
> +	ret = btrfs_batch_delete_items(trans, root, path, curr);
> +	if (ret)
> +		goto delete_fail;
>   	btrfs_release_path(path);
>   	mutex_unlock(&node->mutex);
>   	goto do_again;