public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
@ 2024-12-18 17:06 fdmanana
  2024-12-18 17:06 ` [PATCH 01/20] btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty() fdmanana
                   ` (21 more replies)
  0 siblings, 22 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
is not necessary because we already have a path setup for writing, due
to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
or anything that calls btrfs_search_slot() that way (and it's obvious
from the context). These make the code more verbose, add some overhead
and increase the module's text size unnecessarily. This patchset removes
such unnecessary calls. Often people keep adding them because they copy
the approach done from such places.

The changes are made on a per file basis to make it easier to review or
bisect.

Module size before this patchset:

  $ size fs/btrfs/btrfs.ko 
     text	   data	    bss	    dec	    hex	filename
  1781992	 161029	  16920	1959941	 1de805	fs/btrfs/btrfs.ko

After this patchet:

  $ size fs/btrfs/btrfs.ko 
     text	   data	    bss	    dec	    hex	filename
  1780646	 161021	  16920	1958587	 1de2bb	fs/btrfs/btrfs.ko

Filipe Manana (20):
  btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: free-space-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: extent-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: block-group: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: delayed-inode: remove unnecessary call to btrfs_mark_buffer_dirty()
  btrfs: dev-replace: remove unnecessary call to btrfs_mark_buffer_dirty()
  btrfs: dir-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: file: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: file-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: free-space-cache: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: inode: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: inode-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: ioctl: remove unnecessary call to btrfs_mark_buffer_dirty()
  btrfs: qgroup: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: raid-stripe-tree: remove unnecessary call to btrfs_mark_buffer_dirty()
  btrfs: relocation: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: root-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: uuid-tree: remove unnecessary call to btrfs_mark_buffer_dirty()
  btrfs: volumes: remove unnecessary calls to btrfs_mark_buffer_dirty()
  btrfs: xattr: remove unnecessary call to btrfs_mark_buffer_dirty()

 fs/btrfs/block-group.c      |  2 --
 fs/btrfs/delayed-inode.c    |  1 -
 fs/btrfs/dev-replace.c      |  3 ---
 fs/btrfs/dir-item.c         |  2 --
 fs/btrfs/extent-tree.c      | 10 ----------
 fs/btrfs/file-item.c        |  3 ---
 fs/btrfs/file.c             | 11 -----------
 fs/btrfs/free-space-cache.c |  3 ---
 fs/btrfs/free-space-tree.c  |  5 -----
 fs/btrfs/inode-item.c       |  5 -----
 fs/btrfs/inode.c            |  5 -----
 fs/btrfs/ioctl.c            |  1 -
 fs/btrfs/qgroup.c           | 18 ------------------
 fs/btrfs/raid-stripe-tree.c |  1 -
 fs/btrfs/relocation.c       |  7 -------
 fs/btrfs/root-tree.c        |  2 --
 fs/btrfs/tree-log.c         |  4 ----
 fs/btrfs/uuid-tree.c        |  2 --
 fs/btrfs/volumes.c          | 12 +-----------
 fs/btrfs/xattr.c            |  1 -
 20 files changed, 1 insertion(+), 97 deletions(-)

-- 
2.45.2


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 01/20] btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 02/20] btrfs: free-space-tree: " fdmanana
                   ` (20 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/tree-log.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index c8d6587688b3..955d1677e865 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -590,7 +590,6 @@ static int overwrite_item(struct btrfs_trans_handle *trans,
 		}
 	}
 no_copy:
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	btrfs_release_path(path);
 	return 0;
 }
@@ -3588,7 +3587,6 @@ static noinline int insert_dir_log_key(struct btrfs_trans_handle *trans,
 		last_offset = max(last_offset, curr_end);
 	}
 	btrfs_set_dir_log_end(path->nodes[0], item, last_offset);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	btrfs_release_path(path);
 	return 0;
 }
@@ -4566,7 +4564,6 @@ static noinline int copy_items(struct btrfs_trans_handle *trans,
 		dst_index++;
 	}
 
-	btrfs_mark_buffer_dirty(trans, dst_path->nodes[0]);
 	btrfs_release_path(dst_path);
 out:
 	kfree(ins_data);
@@ -4776,7 +4773,6 @@ static int log_one_extent(struct btrfs_trans_handle *trans,
 	write_extent_buffer(leaf, &fi,
 			    btrfs_item_ptr_offset(leaf, path->slots[0]),
 			    sizeof(fi));
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 	btrfs_release_path(path);
 
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 02/20] btrfs: free-space-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
  2024-12-18 17:06 ` [PATCH 01/20] btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty() fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 03/20] btrfs: extent-tree: " fdmanana
                   ` (19 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
ike btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/free-space-tree.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 7ba50e133921..710205799409 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -89,7 +89,6 @@ static int add_new_free_space_info(struct btrfs_trans_handle *trans,
 			      struct btrfs_free_space_info);
 	btrfs_set_free_space_extent_count(leaf, info, 0);
 	btrfs_set_free_space_flags(leaf, info, 0);
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 	ret = 0;
 out:
@@ -287,7 +286,6 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 	flags |= BTRFS_FREE_SPACE_USING_BITMAPS;
 	btrfs_set_free_space_flags(leaf, info, flags);
 	expected_extent_count = btrfs_free_space_extent_count(leaf, info);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	if (extent_count != expected_extent_count) {
@@ -324,7 +322,6 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 		ptr = btrfs_item_ptr_offset(leaf, path->slots[0]);
 		write_extent_buffer(leaf, bitmap_cursor, ptr,
 				    data_size);
-		btrfs_mark_buffer_dirty(trans, leaf);
 		btrfs_release_path(path);
 
 		i += extent_size;
@@ -430,7 +427,6 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 	flags &= ~BTRFS_FREE_SPACE_USING_BITMAPS;
 	btrfs_set_free_space_flags(leaf, info, flags);
 	expected_extent_count = btrfs_free_space_extent_count(leaf, info);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	nrbits = block_group->length >> block_group->fs_info->sectorsize_bits;
@@ -495,7 +491,6 @@ static int update_free_space_extent_count(struct btrfs_trans_handle *trans,
 
 	extent_count += new_extents;
 	btrfs_set_free_space_extent_count(path->nodes[0], info, extent_count);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	btrfs_release_path(path);
 
 	if (!(flags & BTRFS_FREE_SPACE_USING_BITMAPS) &&
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 03/20] btrfs: extent-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
  2024-12-18 17:06 ` [PATCH 01/20] btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty() fdmanana
  2024-12-18 17:06 ` [PATCH 02/20] btrfs: free-space-tree: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 04/20] btrfs: block-group: " fdmanana
                   ` (18 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/extent-tree.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index e849fc34d8d9..14889b62ef59 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -570,7 +570,6 @@ static noinline int insert_extent_data_ref(struct btrfs_trans_handle *trans,
 			btrfs_set_extent_data_ref_count(leaf, ref, num_refs);
 		}
 	}
-	btrfs_mark_buffer_dirty(trans, leaf);
 	ret = 0;
 fail:
 	btrfs_release_path(path);
@@ -618,7 +617,6 @@ static noinline int remove_extent_data_ref(struct btrfs_trans_handle *trans,
 			btrfs_set_extent_data_ref_count(leaf, ref1, num_refs);
 		else if (key.type == BTRFS_SHARED_DATA_REF_KEY)
 			btrfs_set_shared_data_ref_count(leaf, ref2, num_refs);
-		btrfs_mark_buffer_dirty(trans, leaf);
 	}
 	return ret;
 }
@@ -1050,7 +1048,6 @@ void setup_inline_extent_backref(struct btrfs_trans_handle *trans,
 	} else {
 		btrfs_set_extent_inline_ref_offset(leaf, iref, root_objectid);
 	}
-	btrfs_mark_buffer_dirty(trans, leaf);
 }
 
 static int lookup_extent_backref(struct btrfs_trans_handle *trans,
@@ -1195,7 +1192,6 @@ static noinline_for_stack int update_inline_extent_backref(
 		item_size -= size;
 		btrfs_truncate_item(trans, path, item_size, 1);
 	}
-	btrfs_mark_buffer_dirty(trans, leaf);
 	return 0;
 }
 
@@ -1527,7 +1523,6 @@ static int __btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
 	if (extent_op)
 		__run_delayed_extent_op(extent_op, leaf, item);
 
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	/* now insert the actual backref */
@@ -1711,8 +1706,6 @@ static int run_delayed_extent_op(struct btrfs_trans_handle *trans,
 
 	ei = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_extent_item);
 	__run_delayed_extent_op(extent_op, leaf, ei);
-
-	btrfs_mark_buffer_dirty(trans, leaf);
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -3268,7 +3261,6 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			}
 		} else {
 			btrfs_set_extent_refs(leaf, ei, refs);
-			btrfs_mark_buffer_dirty(trans, leaf);
 		}
 		if (found_extent) {
 			ret = remove_extent_backref(trans, extent_root, path,
@@ -4836,7 +4828,6 @@ static int alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 		btrfs_set_extent_data_ref_count(leaf, ref, ref_mod);
 	}
 
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	btrfs_free_path(path);
 
 	return alloc_reserved_extent(trans, ins->objectid, ins->offset);
@@ -4911,7 +4902,6 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 		btrfs_set_extent_inline_ref_offset(leaf, iref, node->ref_root);
 	}
 
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_free_path(path);
 
 	return alloc_reserved_extent(trans, node->bytenr, fs_info->nodesize);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 04/20] btrfs: block-group: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (2 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 03/20] btrfs: extent-tree: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 05/20] btrfs: delayed-inode: remove unnecessary call " fdmanana
                   ` (17 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 5be029734cfa..8f91aa431074 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -2670,7 +2670,6 @@ static int insert_dev_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_dev_extent_chunk_offset(leaf, extent, chunk_offset);
 
 	btrfs_set_dev_extent_length(leaf, extent, num_bytes);
-	btrfs_mark_buffer_dirty(trans, leaf);
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -3120,7 +3119,6 @@ static int update_block_group_item(struct btrfs_trans_handle *trans,
 						   cache->global_root_id);
 	btrfs_set_stack_block_group_flags(&bgi, cache->flags);
 	write_extent_buffer(leaf, &bgi, bi, sizeof(bgi));
-	btrfs_mark_buffer_dirty(trans, leaf);
 fail:
 	btrfs_release_path(path);
 	/*
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 05/20] btrfs: delayed-inode: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (3 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 04/20] btrfs: block-group: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 06/20] btrfs: dev-replace: " fdmanana
                   ` (16 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at __btrfs_update_delayed_inode() is
not necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1.

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/delayed-inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 508bdbae29a0..02c1efd53904 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1038,7 +1038,6 @@ static int __btrfs_update_delayed_inode(struct btrfs_trans_handle *trans,
 				    struct btrfs_inode_item);
 	write_extent_buffer(leaf, &node->inode_item, (unsigned long)inode_item,
 			    sizeof(struct btrfs_inode_item));
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 	if (!test_bit(BTRFS_DELAYED_NODE_DEL_IREF, &node->flags))
 		goto out;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 06/20] btrfs: dev-replace: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (4 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 05/20] btrfs: delayed-inode: remove unnecessary call " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 07/20] btrfs: dir-item: remove unnecessary calls " fdmanana
                   ` (15 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at btrfs_run_dev_replace() is not
necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1.

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/dev-replace.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index ac8e97ed13f7..f86fbea0b3de 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -440,9 +440,6 @@ int btrfs_run_dev_replace(struct btrfs_trans_handle *trans)
 		dev_replace->cursor_right);
 	dev_replace->item_needs_writeback = 0;
 	up_write(&dev_replace->rwsem);
-
-	btrfs_mark_buffer_dirty(trans, eb);
-
 out:
 	btrfs_free_path(path);
 
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 07/20] btrfs: dir-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (5 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 06/20] btrfs: dev-replace: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 08/20] btrfs: file: " fdmanana
                   ` (14 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/dir-item.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/dir-item.c b/fs/btrfs/dir-item.c
index 1ea5d8fcfbf7..ccf91de29f80 100644
--- a/fs/btrfs/dir-item.c
+++ b/fs/btrfs/dir-item.c
@@ -92,7 +92,6 @@ int btrfs_insert_xattr_item(struct btrfs_trans_handle *trans,
 
 	write_extent_buffer(leaf, name, name_ptr, name_len);
 	write_extent_buffer(leaf, data, data_ptr, data_len);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 
 	return ret;
 }
@@ -152,7 +151,6 @@ int btrfs_insert_dir_item(struct btrfs_trans_handle *trans,
 	name_ptr = (unsigned long)(dir_item + 1);
 
 	write_extent_buffer(leaf, name->name, name_ptr, name->len);
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 second_insert:
 	/* FIXME, use some real flag for selecting the extra index */
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 08/20] btrfs: file: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (6 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 07/20] btrfs: dir-item: remove unnecessary calls " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 09/20] btrfs: file-item: " fdmanana
                   ` (13 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 4775a17c4ee1..36f51c311bb1 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -314,7 +314,6 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 			btrfs_set_file_extent_offset(leaf, fi, extent_offset);
 			btrfs_set_file_extent_num_bytes(leaf, fi,
 							extent_end - args->start);
-			btrfs_mark_buffer_dirty(trans, leaf);
 
 			if (update_refs && disk_bytenr > 0) {
 				struct btrfs_ref ref = {
@@ -360,7 +359,6 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 			btrfs_set_file_extent_offset(leaf, fi, extent_offset);
 			btrfs_set_file_extent_num_bytes(leaf, fi,
 							extent_end - args->end);
-			btrfs_mark_buffer_dirty(trans, leaf);
 			if (update_refs && disk_bytenr > 0)
 				args->bytes_found += args->end - key.offset;
 			break;
@@ -384,7 +382,6 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 
 			btrfs_set_file_extent_num_bytes(leaf, fi,
 							args->start - key.offset);
-			btrfs_mark_buffer_dirty(trans, leaf);
 			if (update_refs && disk_bytenr > 0)
 				args->bytes_found += extent_end - args->start;
 			if (args->end == extent_end)
@@ -639,7 +636,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 							 trans->transid);
 			btrfs_set_file_extent_num_bytes(leaf, fi,
 							end - other_start);
-			btrfs_mark_buffer_dirty(trans, leaf);
 			goto out;
 		}
 	}
@@ -668,7 +664,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 							other_end - start);
 			btrfs_set_file_extent_offset(leaf, fi,
 						     start - orig_offset);
-			btrfs_mark_buffer_dirty(trans, leaf);
 			goto out;
 		}
 	}
@@ -702,7 +697,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_offset(leaf, fi, split - orig_offset);
 		btrfs_set_file_extent_num_bytes(leaf, fi,
 						extent_end - split);
-		btrfs_mark_buffer_dirty(trans, leaf);
 
 		ref.action = BTRFS_ADD_DELAYED_REF;
 		ref.bytenr = bytenr;
@@ -781,7 +775,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_type(leaf, fi,
 					   BTRFS_FILE_EXTENT_REG);
 		btrfs_set_file_extent_generation(leaf, fi, trans->transid);
-		btrfs_mark_buffer_dirty(trans, leaf);
 	} else {
 		fi = btrfs_item_ptr(leaf, del_slot - 1,
 			   struct btrfs_file_extent_item);
@@ -790,7 +783,6 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_generation(leaf, fi, trans->transid);
 		btrfs_set_file_extent_num_bytes(leaf, fi,
 						extent_end - key.offset);
-		btrfs_mark_buffer_dirty(trans, leaf);
 
 		ret = btrfs_del_items(trans, root, path, del_slot, del_nr);
 		if (ret < 0) {
@@ -2016,7 +2008,6 @@ static int fill_holes(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_ram_bytes(leaf, fi, num_bytes);
 		btrfs_set_file_extent_offset(leaf, fi, 0);
 		btrfs_set_file_extent_generation(leaf, fi, trans->transid);
-		btrfs_mark_buffer_dirty(trans, leaf);
 		goto out;
 	}
 
@@ -2033,7 +2024,6 @@ static int fill_holes(struct btrfs_trans_handle *trans,
 		btrfs_set_file_extent_ram_bytes(leaf, fi, num_bytes);
 		btrfs_set_file_extent_offset(leaf, fi, 0);
 		btrfs_set_file_extent_generation(leaf, fi, trans->transid);
-		btrfs_mark_buffer_dirty(trans, leaf);
 		goto out;
 	}
 	btrfs_release_path(path);
@@ -2181,7 +2171,6 @@ static int btrfs_insert_replace_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_file_extent_num_bytes(leaf, extent, replace_len);
 	if (extent_info->is_new_extent)
 		btrfs_set_file_extent_generation(leaf, extent, trans->transid);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	ret = btrfs_inode_set_file_extent_range(inode, extent_info->file_offset,
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 09/20] btrfs: file-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (7 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 08/20] btrfs: file: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 10/20] btrfs: free-space-cache: " fdmanana
                   ` (12 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/file-item.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 886749b39672..d04a3b47b1fb 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -190,8 +190,6 @@ int btrfs_insert_hole_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_file_extent_compression(leaf, item, 0);
 	btrfs_set_file_extent_encryption(leaf, item, 0);
 	btrfs_set_file_extent_other_encoding(leaf, item, 0);
-
-	btrfs_mark_buffer_dirty(trans, leaf);
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -1259,7 +1257,6 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 	ins_size /= csum_size;
 	total_bytes += ins_size * fs_info->sectorsize;
 
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	if (total_bytes < sums->len) {
 		btrfs_release_path(path);
 		cond_resched();
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 10/20] btrfs: free-space-cache: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (8 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 09/20] btrfs: file-item: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 11/20] btrfs: inode: " fdmanana
                   ` (11 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/free-space-cache.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index 17707c898eae..3048cb38dc80 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -198,7 +198,6 @@ static int __create_free_space_inode(struct btrfs_root *root,
 	btrfs_set_inode_nlink(leaf, inode_item, 1);
 	btrfs_set_inode_transid(leaf, inode_item, trans->transid);
 	btrfs_set_inode_block_group(leaf, inode_item, offset);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
@@ -216,7 +215,6 @@ static int __create_free_space_inode(struct btrfs_root *root,
 				struct btrfs_free_space_header);
 	memzero_extent_buffer(leaf, (unsigned long)header, sizeof(*header));
 	btrfs_set_free_space_key(leaf, header, &disk_key);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	return 0;
@@ -1189,7 +1187,6 @@ update_cache_item(struct btrfs_trans_handle *trans,
 	btrfs_set_free_space_entries(leaf, header, entries);
 	btrfs_set_free_space_bitmaps(leaf, header, bitmaps);
 	btrfs_set_free_space_generation(leaf, header, trans->transid);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	return 0;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 11/20] btrfs: inode: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (9 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 10/20] btrfs: free-space-cache: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 12/20] btrfs: inode-item: " fdmanana
                   ` (10 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8a173a24ac05..052ed957f65a 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -564,7 +564,6 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
 		kunmap_local(kaddr);
 		folio_put(folio);
 	}
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	/*
@@ -2918,7 +2917,6 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
 			btrfs_item_ptr_offset(leaf, path->slots[0]),
 			sizeof(struct btrfs_file_extent_item));
 
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_release_path(path);
 
 	/*
@@ -4082,7 +4080,6 @@ static noinline int btrfs_update_inode_item(struct btrfs_trans_handle *trans,
 				    struct btrfs_inode_item);
 
 	fill_inode_item(trans, leaf, inode_item, &inode->vfs_inode);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_set_inode_last_trans(trans, inode);
 	ret = 0;
 failed:
@@ -6377,7 +6374,6 @@ int btrfs_create_new_inode(struct btrfs_trans_handle *trans,
 		}
 	}
 
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	/*
 	 * We don't need the path anymore, plus inheriting properties, adding
 	 * ACLs, security xattrs, orphan item or adding the link, will result in
@@ -8649,7 +8645,6 @@ static int btrfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
 
 	ptr = btrfs_file_extent_inline_start(ei);
 	write_extent_buffer(leaf, symname, ptr, name_len);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_free_path(path);
 
 	d_instantiate_new(dentry, inode);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 12/20] btrfs: inode-item: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (10 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 11/20] btrfs: inode: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 13/20] btrfs: ioctl: remove unnecessary call " fdmanana
                   ` (9 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode-item.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 29572dfaf878..448aa1a682d6 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -298,8 +298,6 @@ static int btrfs_insert_inode_extref(struct btrfs_trans_handle *trans,
 
 	ptr = (unsigned long)&extref->name;
 	write_extent_buffer(path->nodes[0], name->name, ptr, name->len);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
-
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -363,8 +361,6 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans,
 		ptr = (unsigned long)(ref + 1);
 	}
 	write_extent_buffer(path->nodes[0], name->name, ptr, name->len);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
-
 out:
 	btrfs_free_path(path);
 
@@ -590,7 +586,6 @@ int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
 				num_dec = (orig_num_bytes - extent_num_bytes);
 				if (extent_start != 0)
 					control->sub_bytes += num_dec;
-				btrfs_mark_buffer_dirty(trans, leaf);
 			} else {
 				extent_num_bytes =
 					btrfs_file_extent_disk_num_bytes(leaf, fi);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 13/20] btrfs: ioctl: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (11 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 12/20] btrfs: inode-item: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 14/20] btrfs: qgroup: remove unnecessary calls " fdmanana
                   ` (8 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at btrfs_ioctl_default_subvol() is
not necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1.

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7872de140489..0946dcb978e4 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2916,7 +2916,6 @@ static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
 
 	btrfs_cpu_key_to_disk(&disk_key, &new_root->root_key);
 	btrfs_set_dir_item_key(path->nodes[0], di, &disk_key);
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 	btrfs_release_path(path);
 
 	btrfs_set_fs_incompat(fs_info, DEFAULT_SUBVOL);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 14/20] btrfs: qgroup: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (12 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 13/20] btrfs: ioctl: remove unnecessary call " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 15/20] btrfs: raid-stripe-tree: remove unnecessary call " fdmanana
                   ` (7 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/qgroup.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 993b5e803699..b90fabe302e6 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -673,9 +673,6 @@ static int add_qgroup_relation_item(struct btrfs_trans_handle *trans, u64 src,
 	key.offset = dst;
 
 	ret = btrfs_insert_empty_item(trans, quota_root, path, &key, 0);
-
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
-
 	btrfs_free_path(path);
 	return ret;
 }
@@ -752,8 +749,6 @@ static int add_qgroup_item(struct btrfs_trans_handle *trans,
 	btrfs_set_qgroup_info_excl(leaf, qgroup_info, 0);
 	btrfs_set_qgroup_info_excl_cmpr(leaf, qgroup_info, 0);
 
-	btrfs_mark_buffer_dirty(trans, leaf);
-
 	btrfs_release_path(path);
 
 	key.type = BTRFS_QGROUP_LIMIT_KEY;
@@ -771,8 +766,6 @@ static int add_qgroup_item(struct btrfs_trans_handle *trans,
 	btrfs_set_qgroup_limit_rsv_rfer(leaf, qgroup_limit, 0);
 	btrfs_set_qgroup_limit_rsv_excl(leaf, qgroup_limit, 0);
 
-	btrfs_mark_buffer_dirty(trans, leaf);
-
 	ret = 0;
 out:
 	btrfs_free_path(path);
@@ -859,9 +852,6 @@ static int update_qgroup_limit_item(struct btrfs_trans_handle *trans,
 	btrfs_set_qgroup_limit_max_excl(l, qgroup_limit, qgroup->max_excl);
 	btrfs_set_qgroup_limit_rsv_rfer(l, qgroup_limit, qgroup->rsv_rfer);
 	btrfs_set_qgroup_limit_rsv_excl(l, qgroup_limit, qgroup->rsv_excl);
-
-	btrfs_mark_buffer_dirty(trans, l);
-
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -905,9 +895,6 @@ static int update_qgroup_info_item(struct btrfs_trans_handle *trans,
 	btrfs_set_qgroup_info_rfer_cmpr(l, qgroup_info, qgroup->rfer_cmpr);
 	btrfs_set_qgroup_info_excl(l, qgroup_info, qgroup->excl);
 	btrfs_set_qgroup_info_excl_cmpr(l, qgroup_info, qgroup->excl_cmpr);
-
-	btrfs_mark_buffer_dirty(trans, l);
-
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -947,9 +934,6 @@ static int update_qgroup_status_item(struct btrfs_trans_handle *trans)
 	btrfs_set_qgroup_status_generation(l, ptr, trans->transid);
 	btrfs_set_qgroup_status_rescan(l, ptr,
 				fs_info->qgroup_rescan_progress.objectid);
-
-	btrfs_mark_buffer_dirty(trans, l);
-
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -1130,8 +1114,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 				      BTRFS_QGROUP_STATUS_FLAGS_MASK);
 	btrfs_set_qgroup_status_rescan(leaf, ptr, 0);
 
-	btrfs_mark_buffer_dirty(trans, leaf);
-
 	key.objectid = 0;
 	key.type = BTRFS_ROOT_REF_KEY;
 	key.offset = 0;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 15/20] btrfs: raid-stripe-tree: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (13 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 14/20] btrfs: qgroup: remove unnecessary calls " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 16/20] btrfs: relocation: remove unnecessary calls " fdmanana
                   ` (6 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at update_raid_extent_item() is not
necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1.

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/raid-stripe-tree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/raid-stripe-tree.c b/fs/btrfs/raid-stripe-tree.c
index 45b823a0913a..0bf3c032d9dc 100644
--- a/fs/btrfs/raid-stripe-tree.c
+++ b/fs/btrfs/raid-stripe-tree.c
@@ -169,7 +169,6 @@ static int update_raid_extent_item(struct btrfs_trans_handle *trans,
 
 	write_extent_buffer(leaf, stripe_extent, btrfs_item_ptr_offset(leaf, slot),
 			    item_size);
-	btrfs_mark_buffer_dirty(trans, leaf);
 	btrfs_free_path(path);
 
 	return ret;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 16/20] btrfs: relocation: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (14 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 15/20] btrfs: raid-stripe-tree: remove unnecessary call " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 17/20] btrfs: root-tree: " fdmanana
                   ` (5 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/relocation.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index cdd9a7b15a11..d4100e58172f 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -856,7 +856,6 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 	u32 i;
 	int ret = 0;
 	int first = 1;
-	int dirty = 0;
 
 	if (rc->stage != UPDATE_DATA_PTRS)
 		return 0;
@@ -936,7 +935,6 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 		}
 
 		btrfs_set_file_extent_disk_bytenr(leaf, fi, new_bytenr);
-		dirty = 1;
 
 		key.offset -= btrfs_file_extent_offset(leaf, fi);
 		ref.action = BTRFS_ADD_DELAYED_REF;
@@ -967,8 +965,6 @@ int replace_file_extents(struct btrfs_trans_handle *trans,
 			break;
 		}
 	}
-	if (dirty)
-		btrfs_mark_buffer_dirty(trans, leaf);
 	if (inode)
 		btrfs_add_delayed_iput(inode);
 	return ret;
@@ -1161,13 +1157,11 @@ int replace_path(struct btrfs_trans_handle *trans, struct reloc_control *rc,
 		 */
 		btrfs_set_node_blockptr(parent, slot, new_bytenr);
 		btrfs_set_node_ptr_generation(parent, slot, new_ptr_gen);
-		btrfs_mark_buffer_dirty(trans, parent);
 
 		btrfs_set_node_blockptr(path->nodes[level],
 					path->slots[level], old_bytenr);
 		btrfs_set_node_ptr_generation(path->nodes[level],
 					      path->slots[level], old_ptr_gen);
-		btrfs_mark_buffer_dirty(trans, path->nodes[level]);
 
 		ref.action = BTRFS_ADD_DELAYED_REF;
 		ref.bytenr = old_bytenr;
@@ -3728,7 +3722,6 @@ static int __insert_orphan_inode(struct btrfs_trans_handle *trans,
 	btrfs_set_inode_mode(leaf, item, S_IFREG | 0600);
 	btrfs_set_inode_flags(leaf, item, BTRFS_INODE_NOCOMPRESS |
 					  BTRFS_INODE_PREALLOC);
-	btrfs_mark_buffer_dirty(trans, leaf);
 out:
 	btrfs_free_path(path);
 	return ret;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 17/20] btrfs: root-tree: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (15 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 16/20] btrfs: relocation: remove unnecessary calls " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 18/20] btrfs: uuid-tree: remove unnecessary call " fdmanana
                   ` (4 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/root-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 33962671a96c..e22e6b06927a 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -197,7 +197,6 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 	btrfs_set_root_generation_v2(item, btrfs_root_generation(item));
 
 	write_extent_buffer(l, item, ptr, sizeof(*item));
-	btrfs_mark_buffer_dirty(trans, path->nodes[0]);
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -447,7 +446,6 @@ int btrfs_add_root_ref(struct btrfs_trans_handle *trans, u64 root_id,
 	btrfs_set_root_ref_name_len(leaf, ref, name->len);
 	ptr = (unsigned long)(ref + 1);
 	write_extent_buffer(leaf, name->name, ptr, name->len);
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 	if (key.type == BTRFS_ROOT_BACKREF_KEY) {
 		btrfs_release_path(path);
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 18/20] btrfs: uuid-tree: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (16 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 17/20] btrfs: root-tree: " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 19/20] btrfs: volumes: remove unnecessary calls " fdmanana
                   ` (3 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at btrfs_uuid_tree_add() is not
necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1 (through btrfs_insert_empty_item()).

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/uuid-tree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/uuid-tree.c b/fs/btrfs/uuid-tree.c
index aca2861f2187..17b5e81123a1 100644
--- a/fs/btrfs/uuid-tree.c
+++ b/fs/btrfs/uuid-tree.c
@@ -140,8 +140,6 @@ int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, const u8 *uuid, u8 typ
 	ret = 0;
 	subid_le = cpu_to_le64(subid_cpu);
 	write_extent_buffer(eb, &subid_le, offset, sizeof(subid_le));
-	btrfs_mark_buffer_dirty(trans, eb);
-
 out:
 	btrfs_free_path(path);
 	return ret;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 19/20] btrfs: volumes: remove unnecessary calls to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (17 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 18/20] btrfs: uuid-tree: remove unnecessary call " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-18 17:06 ` [PATCH 20/20] btrfs: xattr: remove unnecessary call " fdmanana
                   ` (2 subsequent siblings)
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

We have several places explicitly calling btrfs_mark_buffer_dirty() but
that is not necessarily since the target leaf came from a path that was
obtained for a btree search function that modifies the btree, something
like btrfs_insert_empty_item() or anything else that ends up calling
btrfs_search_slot() with a value of 1 for its 'cow' argument.

These just make the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove them.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/volumes.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d32913c51d69..7975d0d1236c 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2046,7 +2046,6 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 	ptr = btrfs_device_fsid(dev_item);
 	write_extent_buffer(leaf, trans->fs_info->fs_devices->metadata_uuid,
 			    ptr, BTRFS_FSID_SIZE);
-	btrfs_mark_buffer_dirty(trans, leaf);
 
 	ret = 0;
 out:
@@ -2742,11 +2741,9 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans)
 		device = btrfs_find_device(fs_info->fs_devices, &args);
 		BUG_ON(!device); /* Logic error */
 
-		if (device->fs_devices->seeding) {
+		if (device->fs_devices->seeding)
 			btrfs_set_device_generation(leaf, dev_item,
 						    device->generation);
-			btrfs_mark_buffer_dirty(trans, leaf);
-		}
 
 		path->slots[0]++;
 		goto next_slot;
@@ -3039,8 +3036,6 @@ static noinline int btrfs_update_device(struct btrfs_trans_handle *trans,
 				     btrfs_device_get_disk_total_bytes(device));
 	btrfs_set_device_bytes_used(leaf, dev_item,
 				    btrfs_device_get_bytes_used(device));
-	btrfs_mark_buffer_dirty(trans, leaf);
-
 out:
 	btrfs_free_path(path);
 	return ret;
@@ -3749,10 +3744,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
 	btrfs_set_balance_meta(leaf, item, &disk_bargs);
 	btrfs_cpu_balance_args_to_disk(&disk_bargs, &bctl->sys);
 	btrfs_set_balance_sys(leaf, item, &disk_bargs);
-
 	btrfs_set_balance_flags(leaf, item, bctl->flags);
-
-	btrfs_mark_buffer_dirty(trans, leaf);
 out:
 	btrfs_free_path(path);
 	err = btrfs_commit_transaction(trans);
@@ -7746,8 +7738,6 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 	for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++)
 		btrfs_set_dev_stats_value(eb, ptr, i,
 					  btrfs_dev_stat_read(device, i));
-	btrfs_mark_buffer_dirty(trans, eb);
-
 out:
 	btrfs_free_path(path);
 	return ret;
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 20/20] btrfs: xattr: remove unnecessary call to btrfs_mark_buffer_dirty()
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (18 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 19/20] btrfs: volumes: remove unnecessary calls " fdmanana
@ 2024-12-18 17:06 ` fdmanana
  2024-12-19  7:43 ` [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls Johannes Thumshirn
  2024-12-23 20:08 ` David Sterba
  21 siblings, 0 replies; 26+ messages in thread
From: fdmanana @ 2024-12-18 17:06 UTC (permalink / raw)
  To: linux-btrfs

From: Filipe Manana <fdmanana@suse.com>

The call to btrfs_mark_buffer_dirty() at btrfs_setxattr() is not
necessary as we have a path setup for writing with btrfs_search_slot()
having a 'cow' argument set to 1.

This just makes the code more verbose, confusing and add a little extra
overhead and well as increase the module's text size, so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/xattr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index bc18710d1dcf..3e0edbcf73e1 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -204,7 +204,6 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
 		btrfs_set_dir_data_len(leaf, di, size);
 		data_ptr = ((unsigned long)(di + 1)) + name_len;
 		write_extent_buffer(leaf, value, data_ptr, size);
-		btrfs_mark_buffer_dirty(trans, leaf);
 	} else {
 		/*
 		 * Insert, and we had space for the xattr, so path->slots[0] is
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (19 preceding siblings ...)
  2024-12-18 17:06 ` [PATCH 20/20] btrfs: xattr: remove unnecessary call " fdmanana
@ 2024-12-19  7:43 ` Johannes Thumshirn
  2024-12-23 20:08 ` David Sterba
  21 siblings, 0 replies; 26+ messages in thread
From: Johannes Thumshirn @ 2024-12-19  7:43 UTC (permalink / raw)
  To: fdmanana@kernel.org, linux-btrfs@vger.kernel.org

On 18.12.24 18:07, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
> is not necessary because we already have a path setup for writing, due
> to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
> or anything that calls btrfs_search_slot() that way (and it's obvious
> from the context). These make the code more verbose, add some overhead
> and increase the module's text size unnecessarily. This patchset removes
> such unnecessary calls. Often people keep adding them because they copy
> the approach done from such places.
> 
> The changes are made on a per file basis to make it easier to review or
> bisect.
> 
> Module size before this patchset:
> 
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1781992	 161029	  16920	1959941	 1de805	fs/btrfs/btrfs.ko
> 
> After this patchet:
> 
>    $ size fs/btrfs/btrfs.ko
>       text	   data	    bss	    dec	    hex	filename
>    1780646	 161021	  16920	1958587	 1de2bb	fs/btrfs/btrfs.ko

For the series,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
  2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
                   ` (20 preceding siblings ...)
  2024-12-19  7:43 ` [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls Johannes Thumshirn
@ 2024-12-23 20:08 ` David Sterba
  2025-01-06 10:54   ` Filipe Manana
  21 siblings, 1 reply; 26+ messages in thread
From: David Sterba @ 2024-12-23 20:08 UTC (permalink / raw)
  To: fdmanana; +Cc: linux-btrfs

On Wed, Dec 18, 2024 at 05:06:27PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
> is not necessary because we already have a path setup for writing, due
> to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
> or anything that calls btrfs_search_slot() that way (and it's obvious
> from the context). These make the code more verbose, add some overhead
> and increase the module's text size unnecessarily. This patchset removes
> such unnecessary calls. Often people keep adding them because they copy
> the approach done from such places.

I've read the explict calls to btrfs_mark_buffer_dirty() as source-level
marker of the section pairing btrfs_search_slot(). There are also some
assertions and eb state checks that were done in set_extent_buffer_dirty(),
now we're losing them.

While the code gets reduced and the calls may be redundant for some
reasons I think we should keep something at least to do the assertions,
or eventually optimize btrfs_mark_buffer_dirty() to be faster if the
path is set up by the search slot with cow=1.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
  2024-12-23 20:08 ` David Sterba
@ 2025-01-06 10:54   ` Filipe Manana
  2025-01-06 14:19     ` David Sterba
  0 siblings, 1 reply; 26+ messages in thread
From: Filipe Manana @ 2025-01-06 10:54 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On Mon, Dec 23, 2024 at 8:09 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Dec 18, 2024 at 05:06:27PM +0000, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
> > is not necessary because we already have a path setup for writing, due
> > to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
> > or anything that calls btrfs_search_slot() that way (and it's obvious
> > from the context). These make the code more verbose, add some overhead
> > and increase the module's text size unnecessarily. This patchset removes
> > such unnecessary calls. Often people keep adding them because they copy
> > the approach done from such places.
>
> I've read the explict calls to btrfs_mark_buffer_dirty() as source-level
> marker of the section pairing btrfs_search_slot(). There are also some
> assertions and eb state checks that were done in set_extent_buffer_dirty(),
> now we're losing them.

No, we aren't losing the assertions.
We've done them inside ctree.c, where we have already called
btrfs_mark_buffer_dirty() (which calls set_extent_buffer_dirty()).

>
> While the code gets reduced and the calls may be redundant for some
> reasons I think we should keep something at least to do the assertions,
> or eventually optimize btrfs_mark_buffer_dirty() to be faster if the
> path is set up by the search slot with cow=1.

This isn't just about optimization.
It's about simplifying usage of btree modification calls to users
outside ctree.c and having cleaner APIs.

In some places outside ctree.c we (unnecessarily) call
btrfs_mark_buffer_dirty(), while for others we don't.
Probably in the very early days it was needed for some reason, but
that's not the case anymore and we call btrfs_mark_buffer_dirty()
every time we COW or allocate a new block as part of a node/leaf split
in ctree.c.

Keeping these redundant calls is like saying we don't trust what
ctree.c does. Marking a new extent buffer dirty is a clear
responsibility of ctree.c.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
  2025-01-06 10:54   ` Filipe Manana
@ 2025-01-06 14:19     ` David Sterba
  2025-01-06 15:43       ` Filipe Manana
  0 siblings, 1 reply; 26+ messages in thread
From: David Sterba @ 2025-01-06 14:19 UTC (permalink / raw)
  To: Filipe Manana; +Cc: dsterba, linux-btrfs

On Mon, Jan 06, 2025 at 10:54:05AM +0000, Filipe Manana wrote:
> On Mon, Dec 23, 2024 at 8:09 PM David Sterba <dsterba@suse.cz> wrote:
> >
> > On Wed, Dec 18, 2024 at 05:06:27PM +0000, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
> > > is not necessary because we already have a path setup for writing, due
> > > to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
> > > or anything that calls btrfs_search_slot() that way (and it's obvious
> > > from the context). These make the code more verbose, add some overhead
> > > and increase the module's text size unnecessarily. This patchset removes
> > > such unnecessary calls. Often people keep adding them because they copy
> > > the approach done from such places.
> >
> > I've read the explict calls to btrfs_mark_buffer_dirty() as source-level
> > marker of the section pairing btrfs_search_slot(). There are also some
> > assertions and eb state checks that were done in set_extent_buffer_dirty(),
> > now we're losing them.
> 
> No, we aren't losing the assertions.
> We've done them inside ctree.c, where we have already called
> btrfs_mark_buffer_dirty() (which calls set_extent_buffer_dirty()).

What I mean that we would be doing the assertions again, after some
lines of code since the last call to btrfs_mark_buffer_dirty() from
ctree.c.

> > While the code gets reduced and the calls may be redundant for some
> > reasons I think we should keep something at least to do the assertions,
> > or eventually optimize btrfs_mark_buffer_dirty() to be faster if the
> > path is set up by the search slot with cow=1.
> 
> This isn't just about optimization.
> It's about simplifying usage of btree modification calls to users
> outside ctree.c and having cleaner APIs.
> 
> In some places outside ctree.c we (unnecessarily) call
> btrfs_mark_buffer_dirty(), while for others we don't.
> Probably in the very early days it was needed for some reason, but
> that's not the case anymore and we call btrfs_mark_buffer_dirty()
> every time we COW or allocate a new block as part of a node/leaf split
> in ctree.c.
> 
> Keeping these redundant calls is like saying we don't trust what
> ctree.c does. Marking a new extent buffer dirty is a clear
> responsibility of ctree.c.

I would not interpret is as distrust in the ctree.c code but rather as
an additional verification like we do elsewhere. Schematically

caller - start a new change in eb, call the API
ctree.c - create new eb
ctree.c - call btrfs_mark_buffer_dirty()
caller - do more changes, anything until the path is released etc
caller - (removed in the series) call btrfs_mark_buffer_dirty(), now
         performing transid assertion checks,
	 in set_extent_buffer_dirty():
	 - check_buffer_tree_ref()
	 - a few WARN_ONs checking state bits
	 - folio dirty bit checks

From functional POV removing btrfs_mark_buffer_dirty() is ok, but we may
still want to add a simple helper that verifies similar things as
before. With assertions it happens that the checks are redundant.
But feel free to add the series to for-next. The additional verification
would need a separate pass, examine the code locations and pick the
right checks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls
  2025-01-06 14:19     ` David Sterba
@ 2025-01-06 15:43       ` Filipe Manana
  0 siblings, 0 replies; 26+ messages in thread
From: Filipe Manana @ 2025-01-06 15:43 UTC (permalink / raw)
  To: dsterba; +Cc: linux-btrfs

On Mon, Jan 6, 2025 at 2:19 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Mon, Jan 06, 2025 at 10:54:05AM +0000, Filipe Manana wrote:
> > On Mon, Dec 23, 2024 at 8:09 PM David Sterba <dsterba@suse.cz> wrote:
> > >
> > > On Wed, Dec 18, 2024 at 05:06:27PM +0000, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > There's quite a lot of places calling btrfs_mark_buffer_dirty() when it
> > > > is not necessary because we already have a path setup for writing, due
> > > > to calls to btrfs_search_slot() with a 'cow' argument having a value of 1
> > > > or anything that calls btrfs_search_slot() that way (and it's obvious
> > > > from the context). These make the code more verbose, add some overhead
> > > > and increase the module's text size unnecessarily. This patchset removes
> > > > such unnecessary calls. Often people keep adding them because they copy
> > > > the approach done from such places.
> > >
> > > I've read the explict calls to btrfs_mark_buffer_dirty() as source-level
> > > marker of the section pairing btrfs_search_slot(). There are also some
> > > assertions and eb state checks that were done in set_extent_buffer_dirty(),
> > > now we're losing them.
> >
> > No, we aren't losing the assertions.
> > We've done them inside ctree.c, where we have already called
> > btrfs_mark_buffer_dirty() (which calls set_extent_buffer_dirty()).
>
> What I mean that we would be doing the assertions again, after some
> lines of code since the last call to btrfs_mark_buffer_dirty() from
> ctree.c.
>
> > > While the code gets reduced and the calls may be redundant for some
> > > reasons I think we should keep something at least to do the assertions,
> > > or eventually optimize btrfs_mark_buffer_dirty() to be faster if the
> > > path is set up by the search slot with cow=1.
> >
> > This isn't just about optimization.
> > It's about simplifying usage of btree modification calls to users
> > outside ctree.c and having cleaner APIs.
> >
> > In some places outside ctree.c we (unnecessarily) call
> > btrfs_mark_buffer_dirty(), while for others we don't.
> > Probably in the very early days it was needed for some reason, but
> > that's not the case anymore and we call btrfs_mark_buffer_dirty()
> > every time we COW or allocate a new block as part of a node/leaf split
> > in ctree.c.
> >
> > Keeping these redundant calls is like saying we don't trust what
> > ctree.c does. Marking a new extent buffer dirty is a clear
> > responsibility of ctree.c.
>
> I would not interpret is as distrust in the ctree.c code but rather as
> an additional verification like we do elsewhere. Schematically
>
> caller - start a new change in eb, call the API
> ctree.c - create new eb
> ctree.c - call btrfs_mark_buffer_dirty()
> caller - do more changes, anything until the path is released etc
> caller - (removed in the series) call btrfs_mark_buffer_dirty(), now
>          performing transid assertion checks,
>          in set_extent_buffer_dirty():
>          - check_buffer_tree_ref()
>          - a few WARN_ONs checking state bits
>          - folio dirty bit checks
>
> From functional POV removing btrfs_mark_buffer_dirty() is ok, but we may
> still want to add a simple helper that verifies similar things as
> before. With assertions it happens that the checks are redundant.

Well yes, but this is still odd.

The ctree.c call returned a path with a write locked leaf - so none of
the checks should ever fail after the btrfs_mark_buffer_dirty() call
made by ctree.c
If we don't trust that, following that reasoning, it's the same as
making every caller also assert that the leaf is write locked (and we
don't do that).

> But feel free to add the series to for-next. The additional verification
> would need a separate pass, examine the code locations and pick the
> right checks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2025-01-06 15:44 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-18 17:06 [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls fdmanana
2024-12-18 17:06 ` [PATCH 01/20] btrfs: tree-log: remove unnecessary calls to btrfs_mark_buffer_dirty() fdmanana
2024-12-18 17:06 ` [PATCH 02/20] btrfs: free-space-tree: " fdmanana
2024-12-18 17:06 ` [PATCH 03/20] btrfs: extent-tree: " fdmanana
2024-12-18 17:06 ` [PATCH 04/20] btrfs: block-group: " fdmanana
2024-12-18 17:06 ` [PATCH 05/20] btrfs: delayed-inode: remove unnecessary call " fdmanana
2024-12-18 17:06 ` [PATCH 06/20] btrfs: dev-replace: " fdmanana
2024-12-18 17:06 ` [PATCH 07/20] btrfs: dir-item: remove unnecessary calls " fdmanana
2024-12-18 17:06 ` [PATCH 08/20] btrfs: file: " fdmanana
2024-12-18 17:06 ` [PATCH 09/20] btrfs: file-item: " fdmanana
2024-12-18 17:06 ` [PATCH 10/20] btrfs: free-space-cache: " fdmanana
2024-12-18 17:06 ` [PATCH 11/20] btrfs: inode: " fdmanana
2024-12-18 17:06 ` [PATCH 12/20] btrfs: inode-item: " fdmanana
2024-12-18 17:06 ` [PATCH 13/20] btrfs: ioctl: remove unnecessary call " fdmanana
2024-12-18 17:06 ` [PATCH 14/20] btrfs: qgroup: remove unnecessary calls " fdmanana
2024-12-18 17:06 ` [PATCH 15/20] btrfs: raid-stripe-tree: remove unnecessary call " fdmanana
2024-12-18 17:06 ` [PATCH 16/20] btrfs: relocation: remove unnecessary calls " fdmanana
2024-12-18 17:06 ` [PATCH 17/20] btrfs: root-tree: " fdmanana
2024-12-18 17:06 ` [PATCH 18/20] btrfs: uuid-tree: remove unnecessary call " fdmanana
2024-12-18 17:06 ` [PATCH 19/20] btrfs: volumes: remove unnecessary calls " fdmanana
2024-12-18 17:06 ` [PATCH 20/20] btrfs: xattr: remove unnecessary call " fdmanana
2024-12-19  7:43 ` [PATCH 00/20] btrfs: remove plenty of redundant btrfs_mark_buffer_dirty() calls Johannes Thumshirn
2024-12-23 20:08 ` David Sterba
2025-01-06 10:54   ` Filipe Manana
2025-01-06 14:19     ` David Sterba
2025-01-06 15:43       ` Filipe Manana

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox