| Message ID | 20220908002616.3189675-12-shr@fb.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io-uring/btrfs: support async buffered writes | expand |
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 >
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 >>
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)) + 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;
This adds warnings to search functions, which should not have the nowait flag set when called. Signed-off-by: Stefan Roesch <shr@fb.com> --- fs/btrfs/ctree.c | 9 +++++++++ 1 file changed, 9 insertions(+)