[PATCH 0/3] Remove duplicate code in btrfs_prune_dentries/find_next_inode

[PATCH 0/3] Remove duplicate code in btrfs_prune_dentries/find_next_inode

[Nikolay Borisov]

Both functions share similar logic to find a particular inode. So this series
first factors out the common code in btrfs_find_inode and subsequently uses it
to remove most of the internals of the two client functions. This greatly
streamlines the codeflow in the affected functions.

The changes survived a full xfstest run.

Nikolay Borisov (3):
  btrfs: introduce btrfs_find_inode
  btrfs: use btrfs_find_inode in btrfs_prune_dentries
  btrfs: use btrfs_find_inode in find_next_inode

 fs/btrfs/ctree.h      |  1 +
 fs/btrfs/inode.c      | 75 ++++++++++++++++++++++++++++++-------------
 fs/btrfs/relocation.c | 54 +++++++++++--------------------
 3 files changed, 73 insertions(+), 57 deletions(-)

--
2.25.1

[PATCH 1/3] btrfs: introduce btrfs_find_inode

[Nikolay Borisov]

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
+ *
+ * @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)
+{
+	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

[PATCH 2/3] btrfs: use btrfs_find_inode in btrfs_prune_dentries

[Nikolay Borisov]

Now that we have a standalone function which encapsulates the logic of
searching the root's ino rb tree use that. It results in massive
simplification of the code.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 47 +++++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index c11169ba28b2..06724925a3d3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4635,44 +4635,27 @@ struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
 static void btrfs_prune_dentries(struct btrfs_root *root)
 {
 	struct btrfs_fs_info *fs_info = root->fs_info;
-	struct rb_node *node;
-	struct rb_node *prev;
-	struct btrfs_inode *entry;
-	struct inode *inode;
+	struct rb_node *node = NULL;
 	u64 objectid = 0;
 
 	if (!BTRFS_FS_ERROR(fs_info))
 		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
 
 	spin_lock(&root->inode_lock);
-again:
-	node = root->inode_tree.rb_node;
-	prev = NULL;
-	while (node) {
-		prev = node;
-		entry = rb_entry(node, struct btrfs_inode, rb_node);
+	do {
+		struct btrfs_inode *entry;
+		struct inode *inode;
 
-		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)) {
-				node = prev;
+		if (!node) {
+			node = btrfs_find_inode(root, objectid);
+			if (!node)
 				break;
-			}
-			prev = rb_next(prev);
 		}
-	}
-	while (node) {
+
 		entry = rb_entry(node, struct btrfs_inode, rb_node);
 		objectid = btrfs_ino(entry) + 1;
 		inode = igrab(&entry->vfs_inode);
+
 		if (inode) {
 			spin_unlock(&root->inode_lock);
 			if (atomic_read(&inode->i_count) > 1)
@@ -4684,14 +4667,18 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
 			iput(inode);
 			cond_resched();
 			spin_lock(&root->inode_lock);
-			goto again;
+			node = NULL;
+			continue;
 		}
 
-		if (cond_resched_lock(&root->inode_lock))
-			goto again;
+		if (cond_resched_lock(&root->inode_lock)) {
+			node = NULL;
+			continue;
+		}
 
 		node = rb_next(node);
-	}
+	} while(node);
+
 	spin_unlock(&root->inode_lock);
 }
 
-- 
2.25.1

[PATCH 3/3] btrfs: use btrfs_find_inode in find_next_inode

[Nikolay Borisov]

Start using btrfs_find_inode in find_next_inode, now that the common
logic has been encapsulated in the former function. This simplifies the
body of find_next_inode.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/relocation.c | 54 +++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index a6dc827e75af..fdb99e2ce949 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -951,52 +951,36 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
  */
 static struct inode *find_next_inode(struct btrfs_root *root, u64 objectid)
 {
-	struct rb_node *node;
-	struct rb_node *prev;
-	struct btrfs_inode *entry;
-	struct inode *inode;
+	struct rb_node *node = NULL;
+	struct inode *inode = NULL;
 
 	spin_lock(&root->inode_lock);
-again:
-	node = root->inode_tree.rb_node;
-	prev = NULL;
-	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)) {
-				node = prev;
+	do {
+		struct btrfs_inode *entry;
+
+		if (!node) {
+			node = btrfs_find_inode(root, objectid);
+			if (!node)
 				break;
-			}
-			prev = rb_next(prev);
 		}
-	}
-	while (node) {
+
 		entry = rb_entry(node, struct btrfs_inode, rb_node);
+		objectid = btrfs_ino(entry) + 1;
 		inode = igrab(&entry->vfs_inode);
-		if (inode) {
-			spin_unlock(&root->inode_lock);
-			return inode;
-		}
+		if (inode)
+			break;
 
-		objectid = btrfs_ino(entry) + 1;
-		if (cond_resched_lock(&root->inode_lock))
-			goto again;
+		if (cond_resched_lock(&root->inode_lock)) {
+			node = NULL;
+			continue;
+		}
 
 		node = rb_next(node);
-	}
+	} while(node);
+
 	spin_unlock(&root->inode_lock);
-	return NULL;
+	return inode;
 }
 
 /*
-- 
2.25.1

Re: [PATCH 0/3] Remove duplicate code in btrfs_prune_dentries/find_next_inode

[Sweet Tea Dorminy]

checkpatch has a minor nit, while(node) needs to be while (node), but 
otherwise, for the series:

Reviewed-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>

On 2022-07-21 09:50, Nikolay Borisov wrote:
> Both functions share similar logic to find a particular inode. So this 
> series
> first factors out the common code in btrfs_find_inode and subsequently 
> uses it
> to remove most of the internals of the two client functions. This 
> greatly
> streamlines the codeflow in the affected functions.
> 
> The changes survived a full xfstest run.
> 
> Nikolay Borisov (3):
>   btrfs: introduce btrfs_find_inode
>   btrfs: use btrfs_find_inode in btrfs_prune_dentries
>   btrfs: use btrfs_find_inode in find_next_inode
> 
>  fs/btrfs/ctree.h      |  1 +
>  fs/btrfs/inode.c      | 75 ++++++++++++++++++++++++++++++-------------
>  fs/btrfs/relocation.c | 54 +++++++++++--------------------
>  3 files changed, 73 insertions(+), 57 deletions(-)
> 
> --
> 2.25.1

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

[David Sterba]

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

Re: [PATCH 2/3] btrfs: use btrfs_find_inode in btrfs_prune_dentries

[David Sterba]

On Thu, Jul 21, 2022 at 04:50:05PM +0300, Nikolay Borisov wrote:
> Now that we have a standalone function which encapsulates the logic of
> searching the root's ino rb tree use that. It results in massive
> simplification of the code.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 47 +++++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index c11169ba28b2..06724925a3d3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4635,44 +4635,27 @@ struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
>  static void btrfs_prune_dentries(struct btrfs_root *root)
>  {
>  	struct btrfs_fs_info *fs_info = root->fs_info;
> -	struct rb_node *node;
> -	struct rb_node *prev;
> -	struct btrfs_inode *entry;
> -	struct inode *inode;
> +	struct rb_node *node = NULL;
>  	u64 objectid = 0;
>  
>  	if (!BTRFS_FS_ERROR(fs_info))
>  		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
>  
>  	spin_lock(&root->inode_lock);
> -again:
> -	node = root->inode_tree.rb_node;
> -	prev = NULL;
> -	while (node) {
> -		prev = node;
> -		entry = rb_entry(node, struct btrfs_inode, rb_node);
> +	do {
> +		struct btrfs_inode *entry;
> +		struct inode *inode;
>  
> -		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)) {
> -				node = prev;
> +		if (!node) {
> +			node = btrfs_find_inode(root, objectid);
> +			if (!node)
>  				break;
> -			}
> -			prev = rb_next(prev);
>  		}
> -	}
> -	while (node) {
> +
>  		entry = rb_entry(node, struct btrfs_inode, rb_node);
>  		objectid = btrfs_ino(entry) + 1;
>  		inode = igrab(&entry->vfs_inode);
> +
>  		if (inode) {
>  			spin_unlock(&root->inode_lock);
>  			if (atomic_read(&inode->i_count) > 1)
> @@ -4684,14 +4667,18 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
>  			iput(inode);
>  			cond_resched();
>  			spin_lock(&root->inode_lock);
> -			goto again;
> +			node = NULL;

This sets node to NULL and continues, which sends it down to while
(node), which makes it effectively a break and it's not equivalent to
the original behaviour that jumps back to again: or "do {" , or I'm
missing something.

> +			continue;
>  		}
>  
> -		if (cond_resched_lock(&root->inode_lock))
> -			goto again;
> +		if (cond_resched_lock(&root->inode_lock)) {
> +			node = NULL;
> +			continue;
> +		}
>  
>  		node = rb_next(node);
> -	}
> +	} while(node);
> +
>  	spin_unlock(&root->inode_lock);
>  }
>  
> -- 
> 2.25.1

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

[Filipe Manana]

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

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

[David Sterba]

On Thu, Aug 04, 2022 at 04:52:21PM +0100, Filipe Manana wrote:
> 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:
> > 
> > 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).

The function parameters are supposed to be set by callers and what's in
the prototype is contract. A const pointer says "callee will not change
this, promise", but for integer types it does not make sense because it
does not establish any guarantees to caller.

Local variables completely live inside a function and adding the const
there can in some cases optimize the code so that compiler does not need
to read the memory repeatedly. We've been adding it to the known
constant values like sectorsize, or when there's a feature bit or other
status information that's clearly unchanged during the function.

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

I'd rather make this an exception than a rule, to avoid mistakes and for
clarity that a long function takes certain input, but not for short
functions.

Re: [PATCH 2/3] btrfs: use btrfs_find_inode in btrfs_prune_dentries

[Nikolay Borisov]

On 4.08.22 г. 18:41 ч., David Sterba wrote:
> On Thu, Jul 21, 2022 at 04:50:05PM +0300, Nikolay Borisov wrote:
>> Now that we have a standalone function which encapsulates the logic of
>> searching the root's ino rb tree use that. It results in massive
>> simplification of the code.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>   fs/btrfs/inode.c | 47 +++++++++++++++++------------------------------
>>   1 file changed, 17 insertions(+), 30 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index c11169ba28b2..06724925a3d3 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4635,44 +4635,27 @@ struct rb_node *btrfs_find_inode(struct btrfs_root *root, const u64 objectid)
>>   static void btrfs_prune_dentries(struct btrfs_root *root)
>>   {
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>> -	struct rb_node *node;
>> -	struct rb_node *prev;
>> -	struct btrfs_inode *entry;
>> -	struct inode *inode;
>> +	struct rb_node *node = NULL;
>>   	u64 objectid = 0;
>>   
>>   	if (!BTRFS_FS_ERROR(fs_info))
>>   		WARN_ON(btrfs_root_refs(&root->root_item) != 0);
>>   
>>   	spin_lock(&root->inode_lock);
>> -again:
>> -	node = root->inode_tree.rb_node;
>> -	prev = NULL;
>> -	while (node) {
>> -		prev = node;
>> -		entry = rb_entry(node, struct btrfs_inode, rb_node);
>> +	do {
>> +		struct btrfs_inode *entry;
>> +		struct inode *inode;
>>   
>> -		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)) {
>> -				node = prev;
>> +		if (!node) {
>> +			node = btrfs_find_inode(root, objectid);
>> +			if (!node)
>>   				break;
>> -			}
>> -			prev = rb_next(prev);
>>   		}
>> -	}
>> -	while (node) {
>> +
>>   		entry = rb_entry(node, struct btrfs_inode, rb_node);
>>   		objectid = btrfs_ino(entry) + 1;
>>   		inode = igrab(&entry->vfs_inode);
>> +
>>   		if (inode) {
>>   			spin_unlock(&root->inode_lock);
>>   			if (atomic_read(&inode->i_count) > 1)
>> @@ -4684,14 +4667,18 @@ static void btrfs_prune_dentries(struct btrfs_root *root)
>>   			iput(inode);
>>   			cond_resched();
>>   			spin_lock(&root->inode_lock);
>> -			goto again;
>> +			node = NULL;
> 
> This sets node to NULL and continues, which sends it down to while
> (node), which makes it effectively a break and it's not equivalent to
> the original behaviour that jumps back to again: or "do {" , or I'm
> missing something.

You are right, this is buggy, I'll rework it.

> 
>> +			continue;
>>   		}
>>   
>> -		if (cond_resched_lock(&root->inode_lock))
>> -			goto again;
>> +		if (cond_resched_lock(&root->inode_lock)) {
>> +			node = NULL;
>> +			continue;
>> +		}
>>   
>>   		node = rb_next(node);
>> -	}
>> +	} while(node);
>> +
>>   	spin_unlock(&root->inode_lock);
>>   }
>>   
>> -- 
>> 2.25.1

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

[Filipe Manana]

On Thu, Aug 04, 2022 at 06:08:06PM +0200, David Sterba wrote:
> On Thu, Aug 04, 2022 at 04:52:21PM +0100, Filipe Manana wrote:
> > 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:
> > > 
> > > 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).
> 
> The function parameters are supposed to be set by callers and what's in
> the prototype is contract. A const pointer says "callee will not change
> this, promise", but for integer types it does not make sense because it
> does not establish any guarantees to caller.

Sure, but my point was not about giving guarantees to the caller.
It was all about readability for someone reading and changing code.

> 
> Local variables completely live inside a function and adding the const
> there can in some cases optimize the code so that compiler does not need
> to read the memory repeatedly. We've been adding it to the known
> constant values like sectorsize, or when there's a feature bit or other
> status information that's clearly unchanged during the function.
> 
> > 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.
> 
> I'd rather make this an exception than a rule, to avoid mistakes and for
> clarity that a long function takes certain input, but not for short
> functions.

Not sure if I understood that correctly.
You mean it's ok to add the const qualifier only if the function is long?