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

[Stefan Roesch <shr@fb.com>]

On 9/8/22 3:15 AM, Filipe Manana wrote:
> > 
> 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''
> 

I rephrased the commit message.

> 
>>
>> 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.
> 

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
>>