Re: [PATCH v2 11/12] btrfs: add assert to search functions

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Sep 8, 2022 at 1:26 AM Stefan Roesch <shr@fb.com> wrote:
>
> This adds warnings to search functions, which should not have the nowait
> flag set when called.

This could be more clear, by saying btree search functions which are
not used for the buffered IO
and direct IO paths, which are the only users of nowait btree searches.

Also the subject: "btrfs: add assert to search functions"

Mentions assert, but the code adds warnings, which are not the same.
It could also be more clear like:   "btrfs: assert nowait mode is not
used for some btree search functions''


>
> Signed-off-by: Stefan Roesch <shr@fb.com>
> ---
>  fs/btrfs/ctree.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 71b238364939..9caf0f87cbcb 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2165,6 +2165,9 @@ int btrfs_search_old_slot(struct btrfs_root *root, const struct btrfs_key *key,
>         lowest_level = p->lowest_level;
>         WARN_ON(p->nodes[0] != NULL);
>
> +       if (WARN_ON_ONCE(p->nowait == 1))

This doesn't follow the existing code style, which is to treat path
members as booleans, and just do:

WARN_ON_ONCE(p->nowait)

I.e., no explicit " == 1"

As this is a developer thing, I would use ASSERT() instead.

For release builds that typically have CONFIG_BTRFS_ASSERT not set
(like Ubuntu and Debian), it would
still allow the search to continue, which is fine from a functional
perspective, since not respecting nowait
semantics is just a performance thing.

Thanks.


> +               return -EINVAL;
> +
>         if (p->search_commit_root) {
>                 BUG_ON(time_seq);
>                 return btrfs_search_slot(NULL, root, key, p, 0, 0);
> @@ -4465,6 +4468,9 @@ int btrfs_search_forward(struct btrfs_root *root, struct btrfs_key *min_key,
>         int ret = 1;
>         int keep_locks = path->keep_locks;
>
> +       if (WARN_ON_ONCE(path->nowait == 1))
> +               return -EINVAL;
> +
>         path->keep_locks = 1;
>  again:
>         cur = btrfs_read_lock_root_node(root);
> @@ -4645,6 +4651,9 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>         int ret;
>         int i;
>
> +       if (WARN_ON_ONCE(path->nowait == 1))
> +               return -EINVAL;
> +
>         nritems = btrfs_header_nritems(path->nodes[0]);
>         if (nritems == 0)
>                 return 1;
> --
> 2.30.2
>