Re: [PATCH 1/3] btrfs: introduce btrfs_find_inode

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Aug 04, 2022 at 05:28:24PM +0200, David Sterba wrote:
> On Thu, Jul 21, 2022 at 04:50:04PM +0300, Nikolay Borisov wrote:
> > This function holds common code for searching the root's inode rb tree.
> > It will be used to reduce code duplication in future patches.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >  fs/btrfs/ctree.h |  1 +
> >  fs/btrfs/inode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 0ae7f6530da1..fc0a0ab01761 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3311,6 +3311,7 @@ int btrfs_set_inode_index(struct btrfs_inode *dir, u64 *index);
> >  int btrfs_unlink_inode(struct btrfs_trans_handle *trans,
> >  		       struct btrfs_inode *dir, struct btrfs_inode *inode,
> >  		       const char *name, int name_len);
> > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid);
> >  int btrfs_add_link(struct btrfs_trans_handle *trans,
> >  		   struct btrfs_inode *parent_inode, struct btrfs_inode *inode,
> >  		   const char *name, int name_len, int add_backref, u64 index);
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 5fc831a8eba1..c11169ba28b2 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -4587,6 +4587,50 @@ static noinline int may_destroy_subvol(struct btrfs_root *root)
> >  	return ret;
> >  }
> >  
> > +/**
> > + * btrfs_find_inode - returns the rb_node pointing to the inode with an ino
> > + * equal or larger than @objectid
> 
> Please use the simplified format that we have in btrfs.
> 
> > + *
> > + * @root:      root which is going to be searched for an inode
> > + * @objectid:  ino being searched for, if no exact match can be found the
> > + *             function returns the first largest inode
> > + *
> > + * Returns the rb_node pointing to the specified inode or returns NULL if no
> > + * match is found.
> > + *
> > + */
> > +struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
> 
> Const arguments for int types does not make sense.

It makes sense to me, as much as declaring local variables as const, and I don't
recall you ever complain about local const variables before (I do it often, and
I'm not the only one).

Once I read the const part, I can tell for sure that nowhere in the function the
value of the argument is changed.

It happens often that large functions use an int argument as if it was a local
variable and change its value later on, which makes reading the code often a bit
more time consuming and often leads to mistakest too.


> The root can be made
> const, compile tested, no complaints.
> 
> > +{
> > +	struct rb_node *node = root->inode_tree.rb_node;
> > +	struct rb_node *prev = NULL;
> > +	struct btrfs_inode *entry;
> > +
> > +	lockdep_assert_held(&root->inode_lock);
> > +
> > +	while (node) {
> > +		prev = node;
> > +		entry = rb_entry(node, struct btrfs_inode, rb_node);
> > +
> > +		if (objectid < btrfs_ino(entry))
> > +			node = node->rb_left;
> > +		else if (objectid > btrfs_ino(entry))
> > +			node = node->rb_right;
> > +		else
> > +			break;
> > +	}
> > +
> > +	if (!node) {
> > +		while (prev) {
> > +			entry = rb_entry(prev, struct btrfs_inode, rb_node);
> > +			if (objectid <= btrfs_ino(entry))
> > +				return prev;
> > +			prev = rb_next(prev);
> > +		}
> > +	}
> > +
> > +	return node;
> > +}
> > +
> >  /* Delete all dentries for inodes belonging to the root */
> >  static void btrfs_prune_dentries(struct btrfs_root *root)
> >  {
> > -- 
> > 2.25.1