Re: [PATCH 02/21] btrfs: add WARN_ON() on incorrect lock range

Boris Burkov
2023-03-08T19:28:54Z

On Thu, Mar 02, 2023 at 04:24:47PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Add a WARN_ON(start > end) to make sure that the locking happens on the
> correct range and no incorrect nodes (with state->start > state->end)
> are added to the tree.

Looks good, naturally. Quick question about it: do you think that
checking this invariant applies to other extent bit setting operations?

Perhaps it could also make sense to refactor
btrfs_debug_check_extent_io_range s.t. it compiles regardless of
CONFIG_BTRFS_DEBUG, and that it's possible for a caller to opt in to
checking even when the debug setting isn't set. (Perhaps with a
_checked() variant of the set_extent_bit function or something)

>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
> fs/btrfs/extent-io-tree.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/btrfs/extent-io-tree.c b/fs/btrfs/extent-io-tree.c
> index 29a225836e28..482721dd1eba 100644
> --- a/fs/btrfs/extent-io-tree.c
> +++ b/fs/btrfs/extent-io-tree.c
> @@ -1710,6 +1710,7 @@ int try_lock_extent(struct extent_io_tree *tree, u64 start, u64 end,
> int err;
> u64 failed_start;
>
> + WARN_ON(start > end);
> err = __set_extent_bit(tree, start, end, EXTENT_LOCKED, &failed_start,
> NULL, cached, NULL, GFP_NOFS);
> if (err == -EEXIST) {
> @@ -1732,6 +1733,7 @@ int lock_extent(struct extent_io_tree *tree, u64 start, u64 end,
> int err;
> u64 failed_start;
>
> + WARN_ON(start > end);
> err = __set_extent_bit(tree, start, end, EXTENT_LOCKED, &failed_start,
> &failed_state, cached_state, NULL, GFP_NOFS);
> while (err == -EEXIST) {
> --
> 2.39.2
>