On 9/8/22 3:15 AM, Filipe Manana wrote:
> >
> On Thu, Sep 8, 2022 at 1:26 AM Stefan Roesch <shr@xxxxxx> 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''
>
I rephrased the commit message.
>
>>
>> Signed-off-by: Stefan Roesch <shr@xxxxxx>
>> ---
>> 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.
>
The next version of the patch series will use ASSERT.
> 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
>>
