[PATCH 0/2] btrfs: use true/false and simplify boolean parameters in btrfs_{inc,dec}_ref

[PATCH 0/2] btrfs: use true/false and simplify boolean parameters in btrfs_{inc,dec}_ref

[Sun YangKai]

This tiny series removes the last 0/1 integer literals passed to
btrfs_inc_ref() / btrfs_dec_ref() and replaces the open-coded
if/else blocks with concise boolean expressions.

Patch 1 switches the callsites to the self-documenting true/false
constants, eliminating the implicit bool <-> int mixing.

Patch 2 folds the remaining if/else ladders into a single line
using the condition directly, which shrinks the code and makes the
intent obvious.

No functional change.

Sun YangKai (2):
  btrfs: use true/false for boolean parameters in btrfs_{inc,dec}_ref
  btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

 fs/btrfs/ctree.c       | 33 +++++++++++----------------------
 fs/btrfs/extent-tree.c | 21 +++++++--------------
 2 files changed, 18 insertions(+), 36 deletions(-)

-- 
2.51.2

[PATCH 1/2] btrfs: use true/false for boolean parameters in btrfs_{inc,dec}_ref

[Sun YangKai]

Replace integer literals 0/1 with true/false when calling
btrfs_inc_ref() and btrfs_dec_ref() to make the code self-documenting
and avoid mixing bool/integer types.

No functional change intended.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ctree.c       | 20 ++++++++++----------
 fs/btrfs/extent-tree.c |  8 ++++----
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3c4aea71bbbf..1b15cef86cbc 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -292,11 +292,11 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
 	}
 
 	if (new_root_objectid == BTRFS_TREE_RELOC_OBJECTID) {
-		ret = btrfs_inc_ref(trans, root, cow, 1);
+		ret = btrfs_inc_ref(trans, root, cow, true);
 		if (unlikely(ret))
 			btrfs_abort_transaction(trans, ret);
 	} else {
-		ret = btrfs_inc_ref(trans, root, cow, 0);
+		ret = btrfs_inc_ref(trans, root, cow, false);
 		if (unlikely(ret))
 			btrfs_abort_transaction(trans, ret);
 	}
@@ -420,15 +420,15 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		if ((owner == btrfs_root_id(root) ||
 		     btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID) &&
 		    !(flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)) {
-			ret = btrfs_inc_ref(trans, root, buf, 1);
+			ret = btrfs_inc_ref(trans, root, buf, true);
 			if (ret)
 				return ret;
 
 			if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID) {
-				ret = btrfs_dec_ref(trans, root, buf, 0);
+				ret = btrfs_dec_ref(trans, root, buf, false);
 				if (ret)
 					return ret;
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, true);
 				if (ret)
 					return ret;
 			}
@@ -439,21 +439,21 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 		} else {
 
 			if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, true);
 			else
-				ret = btrfs_inc_ref(trans, root, cow, 0);
+				ret = btrfs_inc_ref(trans, root, cow, false);
 			if (ret)
 				return ret;
 		}
 	} else {
 		if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
 			if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, 1);
+				ret = btrfs_inc_ref(trans, root, cow, true);
 			else
-				ret = btrfs_inc_ref(trans, root, cow, 0);
+				ret = btrfs_inc_ref(trans, root, cow, false);
 			if (ret)
 				return ret;
-			ret = btrfs_dec_ref(trans, root, buf, 1);
+			ret = btrfs_dec_ref(trans, root, buf, true);
 			if (ret)
 				return ret;
 		}
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b22b0aaa99e4..527310f3aeb3 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5469,12 +5469,12 @@ static noinline int walk_down_proc(struct btrfs_trans_handle *trans,
 	/* wc->stage == UPDATE_BACKREF */
 	if (!(wc->flags[level] & flag)) {
 		ASSERT(path->locks[level]);
-		ret = btrfs_inc_ref(trans, root, eb, 1);
+		ret = btrfs_inc_ref(trans, root, eb, true);
 		if (unlikely(ret)) {
 			btrfs_abort_transaction(trans, ret);
 			return ret;
 		}
-		ret = btrfs_dec_ref(trans, root, eb, 0);
+		ret = btrfs_dec_ref(trans, root, eb, false);
 		if (unlikely(ret)) {
 			btrfs_abort_transaction(trans, ret);
 			return ret;
@@ -5876,13 +5876,13 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 	if (wc->refs[level] == 1) {
 		if (level == 0) {
 			if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
-				ret = btrfs_dec_ref(trans, root, eb, 1);
+				ret = btrfs_dec_ref(trans, root, eb, true);
 				if (ret) {
 					btrfs_abort_transaction(trans, ret);
 					return ret;
 				}
 			} else {
-				ret = btrfs_dec_ref(trans, root, eb, 0);
+				ret = btrfs_dec_ref(trans, root, eb, false);
 				if (unlikely(ret)) {
 					btrfs_abort_transaction(trans, ret);
 					return ret;
-- 
2.51.2

Re: [PATCH 1/2] btrfs: use true/false for boolean parameters in btrfs_{inc,dec}_ref

[Johannes Thumshirn]

Looks good,

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[Sun YangKai]

Replace open-coded if/else blocks with the boolean expression
directly, making the code shorter and easier to read.

No functional change.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/ctree.c       | 25 +++++++------------------
 fs/btrfs/extent-tree.c | 17 +++++------------
 2 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 1b15cef86cbc..e056dedec221 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -291,15 +291,9 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
 		return ret;
 	}
 
-	if (new_root_objectid == BTRFS_TREE_RELOC_OBJECTID) {
-		ret = btrfs_inc_ref(trans, root, cow, true);
-		if (unlikely(ret))
-			btrfs_abort_transaction(trans, ret);
-	} else {
-		ret = btrfs_inc_ref(trans, root, cow, false);
-		if (unlikely(ret))
-			btrfs_abort_transaction(trans, ret);
-	}
+	ret = btrfs_inc_ref(trans, root, cow, new_root_objectid == BTRFS_TREE_RELOC_OBJECTID);
+	if (unlikely(ret))
+		btrfs_abort_transaction(trans, ret);
 	if (ret) {
 		btrfs_tree_unlock(cow);
 		free_extent_buffer(cow);
@@ -437,20 +431,15 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 			if (ret)
 				return ret;
 		} else {
-
-			if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, true);
-			else
-				ret = btrfs_inc_ref(trans, root, cow, false);
+			ret = btrfs_inc_ref(trans, root, cow,
+					    btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID);
 			if (ret)
 				return ret;
 		}
 	} else {
 		if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
-			if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID)
-				ret = btrfs_inc_ref(trans, root, cow, true);
-			else
-				ret = btrfs_inc_ref(trans, root, cow, false);
+			ret = btrfs_inc_ref(trans, root, cow,
+					    btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID);
 			if (ret)
 				return ret;
 			ret = btrfs_dec_ref(trans, root, buf, true);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 527310f3aeb3..8e6f362de649 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5875,18 +5875,11 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans,
 
 	if (wc->refs[level] == 1) {
 		if (level == 0) {
-			if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
-				ret = btrfs_dec_ref(trans, root, eb, true);
-				if (ret) {
-					btrfs_abort_transaction(trans, ret);
-					return ret;
-				}
-			} else {
-				ret = btrfs_dec_ref(trans, root, eb, false);
-				if (unlikely(ret)) {
-					btrfs_abort_transaction(trans, ret);
-					return ret;
-				}
+			ret = btrfs_dec_ref(trans, root, eb,
+					    !!(wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF));
+			if (unlikely(ret)) {
+				btrfs_abort_transaction(trans, ret);
+				return ret;
 			}
 			if (btrfs_is_fstree(btrfs_root_id(root))) {
 				ret = btrfs_qgroup_trace_leaf_items(trans, eb);
-- 
2.51.2

Re: [PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[David Sterba]

On Thu, Nov 20, 2025 at 10:19:14PM +0800, Sun YangKai wrote:
> Replace open-coded if/else blocks with the boolean expression
> directly, making the code shorter and easier to read.
> 
> No functional change.
> 
> Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> ---
>  fs/btrfs/ctree.c       | 25 +++++++------------------
>  fs/btrfs/extent-tree.c | 17 +++++------------
>  2 files changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 1b15cef86cbc..e056dedec221 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -291,15 +291,9 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
>  		return ret;
>  	}
>  
> -	if (new_root_objectid == BTRFS_TREE_RELOC_OBJECTID) {
> -		ret = btrfs_inc_ref(trans, root, cow, true);
> -		if (unlikely(ret))
> -			btrfs_abort_transaction(trans, ret);
> -	} else {
> -		ret = btrfs_inc_ref(trans, root, cow, false);
> -		if (unlikely(ret))
> -			btrfs_abort_transaction(trans, ret);
> -	}
> +	ret = btrfs_inc_ref(trans, root, cow, new_root_objectid == BTRFS_TREE_RELOC_OBJECTID);
> +	if (unlikely(ret))
> +		btrfs_abort_transaction(trans, ret);

I find this worse than the original because the key piece of code is the
condition and putting it to the argument obscures it. I think we have
the condition in paramter in some places but it's where it does not
matter that much.

Re: [PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[Sun Yangkai]

> I find this worse than the original because the key piece of code is the
> condition and putting it to the argument obscures it. I think we have
> the condition in paramter in some places but it's where it does not
> matter that much.

Yes, I totally understand. The condition in function parameter is annoying. We can

1. Leave the `if (cond) foo(true); else foo(false);` untouched. This seems a
little stupid and we have to repeat the same thing in different branches.
But it is more clear and the if-branch condition fits better with the common
style. If you prefer it, just feel free to drop the second patch.

2. Do something like this patch. But the foo(cond) style is rare and alien for
people not get used to it.

3. We can optionally add a local variable like Boris mentioned.

> Nice improvement on patch 2. It's much better already, but I am also
> sort of curious how you feel about giving the bool a name to make it
> more self-documenting.
> 
> e.g.,
> bool full_backref = btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID;
> btrfs_inc_ref(trans, root, cow, full_backref);

It looks better than 2, but still seems uncommon. And
  - The local variable may be declared far away from where we need to use it.
  - The same condition might be used elsewhere in the scope, so we also want to
replace those usages with the new local variable and the name full_backref is no
longer proper.

So I didn't do this in the patch.

Thanks,
Sun YangKai

Re: [PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[Johannes Thumshirn]

On 11/21/25 4:17 AM, Sun Yangkai wrote:
> 3. We can optionally add a local variable like Boris mentioned.

Yeah I'd opt for that as well.

Re: [PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[David Sterba]

On Fri, Nov 21, 2025 at 06:58:53AM +0000, Johannes Thumshirn wrote:
> On 11/21/25 4:17 AM, Sun Yangkai wrote:
> > 3. We can optionally add a local variable like Boris mentioned.
> 
> Yeah I'd opt for that as well.

Ok, this looks reasonable. We use the 'const bool = (condition)' pattern
elsewhere too.

Re: [PATCH 2/2] btrfs: simplify boolean argument for btrfs_{inc,dec}_ref

[Daniel Vacek]

On Fri, 21 Nov 2025 at 04:17, Sun Yangkai <sunk67188@gmail.com> wrote:
> > I find this worse than the original because the key piece of code is the
> > condition and putting it to the argument obscures it. I think we have
> > the condition in paramter in some places but it's where it does not
> > matter that much.
>
> Yes, I totally understand. The condition in function parameter is annoying. We can
>
> 1. Leave the `if (cond) foo(true); else foo(false);` untouched. This seems a
> little stupid and we have to repeat the same thing in different branches.
> But it is more clear and the if-branch condition fits better with the common
> style. If you prefer it, just feel free to drop the second patch.
>
> 2. Do something like this patch. But the foo(cond) style is rare and alien for
> people not get used to it.
>
> 3. We can optionally add a local variable like Boris mentioned.

And perhaps add a comment on top of that.

> > Nice improvement on patch 2. It's much better already, but I am also
> > sort of curious how you feel about giving the bool a name to make it
> > more self-documenting.
> >
> > e.g.,
> > bool full_backref = btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID;
> > btrfs_inc_ref(trans, root, cow, full_backref);
>
> It looks better than 2, but still seems uncommon. And
>   - The local variable may be declared far away from where we need to use it.
>   - The same condition might be used elsewhere in the scope, so we also want to
> replace those usages with the new local variable and the name full_backref is no
> longer proper.
>
> So I didn't do this in the patch.
>
> Thanks,
> Sun YangKai
>
>

Re: [PATCH 0/2] btrfs: use true/false and simplify boolean parameters in btrfs_{inc,dec}_ref

[Boris Burkov]

On Thu, Nov 20, 2025 at 10:19:12PM +0800, Sun YangKai wrote:
> This tiny series removes the last 0/1 integer literals passed to
> btrfs_inc_ref() / btrfs_dec_ref() and replaces the open-coded
> if/else blocks with concise boolean expressions.
> 
> Patch 1 switches the callsites to the self-documenting true/false
> constants, eliminating the implicit bool <-> int mixing.
> 
> Patch 2 folds the remaining if/else ladders into a single line
> using the condition directly, which shrinks the code and makes the
> intent obvious.

Nice improvement on patch 2. It's much better already, but I am also
sort of curious how you feel about giving the bool a name to make it
more self-documenting.

e.g.,
bool full_backref = btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID;
btrfs_inc_ref(trans, root, cow, full_backref);

Just a thought, definitely not a blocker. Either way you decide, please
feel free to add

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> No functional change.
> 
> Sun YangKai (2):
>   btrfs: use true/false for boolean parameters in btrfs_{inc,dec}_ref
>   btrfs: simplify boolean argument for btrfs_{inc,dec}_ref
> 
>  fs/btrfs/ctree.c       | 33 +++++++++++----------------------
>  fs/btrfs/extent-tree.c | 21 +++++++--------------
>  2 files changed, 18 insertions(+), 36 deletions(-)
> 
> -- 
> 2.51.2
>