Re: [PATCH] btrfs: Introduce btrfs_search_backwards function

[Marcos Paulo de Souza <mpdesouza@suse.de>]

On Wed, 2021-07-28 at 15:10 +0200, David Sterba wrote:
> On Mon, Jul 26, 2021 at 11:03:17AM -0300, Marcos Paulo de Souza
> wrote:
> > It's a common practice to start a search using offset (u64)-1,
> > which is
> > the u64 maximum value, meaning that we want the search_slot
> > function to
> > be set in the last item with the same objectid and type.
> > 
> > Once we are in this position, it's a matter to start a search
> > backwards
> > by calling btrfs_previous_item, which will check if we'll need to
> > go to
> > a previous leaf and other necessary checks, only to be sure that we
> > are
> > in last offset of the same object and type. If the item is found,
> > convert the ondisk structure to the current endianness of the
> > machine
> > running the code.
> > 
> > The new btrfs_search_backwards function does the all these
> > procedures when
> > necessary, and can be used to avoid code duplication.
> > 
> > No functional changes.
> > 
> > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> > ---
> >  fs/btrfs/ctree.c   | 23 +++++++++++++++++++++++
> >  fs/btrfs/ctree.h   |  6 ++++++
> >  fs/btrfs/ioctl.c   | 30 ++++++++----------------------
> >  fs/btrfs/super.c   | 26 ++++++--------------------
> >  fs/btrfs/volumes.c |  7 +------
> >  5 files changed, 44 insertions(+), 48 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> > index 394fec1d3fd9..2991ee845813 100644
> > --- a/fs/btrfs/ctree.c
> > +++ b/fs/btrfs/ctree.c
> > @@ -2100,6 +2100,29 @@ int btrfs_search_slot_for_read(struct
> > btrfs_root *root,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Execute search and call btrfs_previous_item to traverse
> > backwards if the item
> > + * was not found. If found, convert the stored item to the correct
> > endianness.
> > + *
> > + * Return 0 if found, 1 if not found and < 0 if error.
> > + */
> > +int btrfs_search_backwards(struct btrfs_root *root,
> > +				struct btrfs_key *key,
> > +				struct btrfs_key *found_key,
> 
> Is it necessary to have 2 keys? All calls pass the same one, so
> either
> this should be just one or you have other patches that make use of
> two
> distinct keys?

Yes, in these cases yes, but I can see sometimes other places in the
btrfs codebase where the key used convert to cpu is different from the
key used in the search path, so I wanted to make it flexible to future
users.

I believe that we can extent this in the future if needed, so
I'll send a v2 using only one key argument, and use the same argument
to convert to the correct endianness.

Thanks,
  Marcos

> 
> > -		ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> > +		ret = btrfs_search_backwards(root, &key, &key, path);
> 
> 						   &key, &key