Re: [PATCH v1 01/10] btrfs: implement a nowait option for tree searches

[Filipe Manana <fdmanana@gmail.com>]

On Fri, Sep 2, 2022 at 3:57 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/2/22 8:48 AM, Filipe Manana wrote:
> > On Fri, Sep 2, 2022 at 12:01 AM Stefan Roesch <shr@fb.com> wrote:
> >>
> >> From: Josef Bacik <josef@toxicpanda.com>
> >>
> >> For NOWAIT IOCB's we'll need a way to tell search to not wait on locks
> >> or anything.  Accomplish this by adding a path->nowait flag that will
> >> use trylocks and skip reading of metadata, returning -EWOULDBLOCK in
> >> either of these cases.  For now we only need this for reads, so only the
> >> read side is handled.  Add an ASSERT() to catch anybody trying to use
> >> this for writes so they know they'll have to implement the write side.
> >>
> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >> Signed-off-by: Stefan Roesch <shr@fb.com>
> >> ---
> >>  fs/btrfs/ctree.c   | 39 ++++++++++++++++++++++++++++++++++++---
> >>  fs/btrfs/ctree.h   |  1 +
> >>  fs/btrfs/locking.c | 23 +++++++++++++++++++++++
> >>  fs/btrfs/locking.h |  1 +
> >>  4 files changed, 61 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> >> index ebfa35fe1c38..052c768b2297 100644
> >> --- a/fs/btrfs/ctree.c
> >> +++ b/fs/btrfs/ctree.c
> >> @@ -1447,6 +1447,11 @@ read_block_for_search(struct btrfs_root *root, struct btrfs_path *p,
> >>                         return 0;
> >>                 }
> >>
> >> +               if (p->nowait) {
> >> +                       free_extent_buffer(tmp);
> >> +                       return -EWOULDBLOCK;
> >> +               }
> >> +
> >>                 if (unlock_up)
> >>                         btrfs_unlock_up_safe(p, level + 1);
> >>
> >> @@ -1467,6 +1472,8 @@ read_block_for_search(struct btrfs_root *root, struct btrfs_path *p,
> >>                         ret = -EAGAIN;
> >>
> >>                 goto out;
> >> +       } else if (p->nowait) {
> >> +               return -EWOULDBLOCK;
> >>         }
> >>
> >>         if (unlock_up) {
> >> @@ -1634,7 +1641,13 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
> >>                  * We don't know the level of the root node until we actually
> >>                  * have it read locked
> >>                  */
> >> -               b = btrfs_read_lock_root_node(root);
> >> +               if (p->nowait) {
> >> +                       b = btrfs_try_read_lock_root_node(root);
> >> +                       if (IS_ERR(b))
> >> +                               return b;
> >> +               } else {
> >> +                       b = btrfs_read_lock_root_node(root);
> >> +               }
> >>                 level = btrfs_header_level(b);
> >>                 if (level > write_lock_level)
> >>                         goto out;
> >> @@ -1910,6 +1923,13 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> >>         WARN_ON(p->nodes[0] != NULL);
> >>         BUG_ON(!cow && ins_len);
> >>
> >> +       /*
> >> +        * For now only allow nowait for read only operations.  There's no
> >> +        * strict reason why we can't, we just only need it for reads so I'm
> >> +        * only implementing it for reads right now.
> >> +        */
> >> +       ASSERT(!p->nowait || !cow);
> >> +
> >>         if (ins_len < 0) {
> >>                 lowest_unlock = 2;
> >>
> >> @@ -1936,7 +1956,12 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> >>
> >>         if (p->need_commit_sem) {
> >>                 ASSERT(p->search_commit_root);
> >> -               down_read(&fs_info->commit_root_sem);
> >> +               if (p->nowait) {
> >> +                       if (!down_read_trylock(&fs_info->commit_root_sem))
> >> +                               return -EAGAIN;
> >
> > Why EAGAIN here and everywhere else EWOULDBLOCK? See below.
>
> Is EWOULDBLOCK ever different from EAGAIN? But it should be used
> consistently, EAGAIN would be the return of choice for that.

Oh right, EWOULDBLOCK is defined as EAGAIN, same values.
It would be best to use the same everywhere, avoiding confusion...

>
> --
> Jens Axboe



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”