Skip to contentSkip to conversation

Misc cleanups

PATCH In review David Sterba 5 participants 30 messages Mar 8, 2018 – Mar 16, 2018 Btrfs file system

lore
Share & tools
lore

David Sterba

author maintainer
#0 lore raw

cover Misc cleanups

Cleanups that have piled over the time. The "very important" unused argument removals, some renaming and simplifications.

The effects on stack consumption are not huge, but still measurable:

ctree.c:tree_mod_log_insert_key +16 (48 -> 64)

ctree.c:tree_mod_log_insert_move               -8 (104 -> 96)
ctree.c:insert_ptr.isra.24                     -8 (80 -> 72)
ctree.c:del_ptr.isra.25                        -8 (96 -> 88)
ctree.c:btrfs_old_root_level                   -8 (32 -> 24)
ctree.c:btrfs_set_item_key_safe                -8 (88 -> 80)

extent_io.c:submit_extent_page -40 (112 -> 72)

David Sterba (22):
btrfs: assume that bio_ret is always valid in submit_extent_page
btrfs: assume that prev_em_start is always valid in __do_readpage
btrfs: remove redundant variable in __do_readpage
btrfs: cleanup merging conditions in submit_extent_page
btrfs: document more parameters of submit_extent_page
btrfs: drop fs_info parameter from tree_mod_log_set_node_key
btrfs: drop fs_info parameter from tree_mod_log_insert_move
btrfs: drop fs_info parameter from tree_mod_log_insert_key
btrfs: drop fs_info parameter from tree_mod_log_free_eb
btrfs: drop fs_info parameter from tree_mod_log_free_eb
btrfs: drop unused fs_info parameter from tree_mod_log_eb_move
btrfs: embed tree_mod_move structure to tree_mod_elem
btrfs: drop fs_info parameter from __tree_mod_log_oldest_root
btrfs: remove trivial locking wrappers of tree mod log
btrfs: kill trivial wrapper tree_mod_log_eb_move
btrfs: kill tree_mod_log_set_node_key helper
btrfs: kill tree_mod_log_set_root_pointer helper
btrfs: move allocation after simple tests in tree_mod_log_insert_key
btrfs: separate types for submit_bio_start and submit_bio_done
btrfs: remove unused parameters from extent_submit_bio_start_t
btrfs: remove unused parameters from extent_submit_bio_done_t
btrfs: rename submit callbacks and drop double underscores

 fs/btrfs/ctree.c     | 234 ++++++++++++++++++++-------------------------------
 fs/btrfs/disk-io.c   |  24 +++---
 fs/btrfs/disk-io.h   |   4 +-
 fs/btrfs/extent_io.c |  61 ++++++++------
 fs/btrfs/extent_io.h |   7 ++
 fs/btrfs/inode.c     |  30 +++----
 6 files changed, 158 insertions(+), 202 deletions(-)
Signature · 1 line
2.16.2
author maintainer
#1 lore raw

PATCH 1/22 btrfs: assume that bio_ret is always valid in submit_extent_page

All callers pass a valid pointer so we can drop the redundant checks. The call to submit_one_bio never happend and can be removed.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
fs/btrfs/extent_io.c +5 -5
index dfeb74a0be77..d00d5a59ff21 100644
@@ -2758,6 +2758,7 @@ static int merge_bio(struct extent_io_tree *tree, struct page *page,
 
 /*
  * @opf:	bio REQ_OP_* and REQ_* flags as one value
+ * @bio_ret:	must be valid pointer, newly allocated bio will be stored there
  */
 static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 			      struct writeback_control *wbc,

@@ -2778,7 +2779,9 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 	size_t page_size = min_t(size_t, size, PAGE_SIZE);
 	sector_t sector = offset >> 9;
 
-	if (bio_ret && *bio_ret) {
+	ASSERT(bio_ret);
+
+	if (*bio_ret) {
 		bio = *bio_ret;
 		if (old_compressed)
 			contig = bio->bi_iter.bi_sector == sector;

@@ -2813,10 +2816,7 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 		wbc_account_io(wbc, page, page_size);
 	}
 
-	if (bio_ret)
-		*bio_ret = bio;
-	else
-		ret = submit_one_bio(bio, mirror_num, bio_flags);
+	*bio_ret = bio;
 
 	return ret;
 }
Signature · 1 line
2.16.2
author maintainer
#2 lore raw

PATCH 2/22 btrfs: assume that prev_em_start is always valid in __do_readpage

In reply to David Sterba

All callers pass a valid pointer, we can remove the redundant checks.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
fs/btrfs/extent_io.c +6 -3
index d00d5a59ff21..36514baa661e 100644
@@ -2875,6 +2875,8 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
  * handlers)
  * XXX JDM: This needs looking at to ensure proper page locking
  * return 0 on success, otherwise return error
+ *
+ * @prev_em_start:	return value of previous em start value; must be valid
  */
 static int __do_readpage(struct extent_io_tree *tree,
 			 struct page *page,

@@ -2903,6 +2905,8 @@ static int __do_readpage(struct extent_io_tree *tree,
 	size_t blocksize = inode->i_sb->s_blocksize;
 	unsigned long this_bio_flag = 0;
 
+	ASSERT(prev_em_start);
+
 	set_page_extent_mapped(page);
 
 	end = page_end;

@@ -3012,12 +3016,11 @@ static int __do_readpage(struct extent_io_tree *tree,
 		 * non-optimal behavior (submitting 2 bios for the same extent).
 		 */
 		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags) &&
-		    prev_em_start && *prev_em_start != (u64)-1 &&
+		    *prev_em_start != (u64)-1 &&
 		    *prev_em_start != em->orig_start)
 			force_bio_submit = true;
 
-		if (prev_em_start)
-			*prev_em_start = em->orig_start;
+		*prev_em_start = em->orig_start;
 
 		free_extent_map(em);
 		em = NULL;
Signature · 1 line
2.16.2
#3 lore raw

Re: [PATCH 02/22] btrfs: assume that prev_em_start is always valid in __do_readpage

Quoted 21 lines · David Sterba

All callers pass a valid pointer, we can remove the redundant checks.

Signed-off-by David Sterba <[email protected]>
---
  fs/btrfs/extent_io.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)
fs/btrfs/extent_io.c +2 -0
index d00d5a59ff21..36514baa661e 100644
@@ -2875,6 +2875,8 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
   * handlers)
   * XXX JDM: This needs looking at to ensure proper page locking
   * return 0 on success, otherwise return error
+ *
+ * @prev_em_start:	return value of previous em start value; must be valid
   */
  static int __do_readpage(struct extent_io_tree *tree,
  			 struct page *page,

fs/btrfs/extent_io.c static int __do_readpage(struct extent_io_tree *tree, · line 2909

Quoted 5 lines
fs/btrfs/extent_io.c +2 -0
@@ -2903,6 +2905,8 @@ static int __do_readpage(struct extent_io_tree *tree,
  	size_t blocksize = inode->i_sb->s_blocksize;
  	unsigned long this_bio_flag = 0;
 
+	ASSERT(prev_em_start);
+

mount is failing with this patch as in this stack below the prev_em_start is NULL.

--------
static int __extent_read_full_page(struct extent_io_tree *tree,
struct page *page, get_extent_t *get_extent, struct bio **bio, int mirror_num, unsigned long *bio_flags, unsigned int read_flags)
{
::
ret = __do_readpage(tree, page, get_extent, NULL, bio, mirror_num,
bio_flags, read_flags, NULL);

----------
kernel: assertion failed: prev_em_start, file: fs/btrfs/extent_io.c,
line: 2911
kernel: ------------[ cut here ]------------
kernel: kernel BUG at fs/btrfs/ctree.h:3462!

 kernel:  __do_readpage+0x7f/0x7e0 [btrfs]
 kernel:  ? btree_fs_info+0x20/0x20 [btrfs]
 kernel:  ? mark_held_locks+0x65/0x80
 kernel:  ? _raw_spin_unlock_irq+0x29/0x40
 kernel:  __extent_read_full_page+0xe7/0x100 [btrfs]
 kernel:  ? btree_fs_info+0x20/0x20 [btrfs]
 kernel:  read_extent_buffer_pages+0x1af/0x2b0 [btrfs]
 kernel:  btree_read_extent_buffer_pages+0x4f/0xe0 [btrfs]
 kernel:  read_tree_block+0x31/0x60 [btrfs]
 kernel:  ? __raw_spin_lock_init+0x2d/0x50
 kernel:  open_ctree+0x19a2/0x25f0 [btrfs]
 kernel:  btrfs_mount_root+0x465/0x720 [btrfs]
 kernel:  ? __lockdep_init_map+0xb6/0x1d0
 kernel:  ? mount_fs+0x89/0x130
 kernel:  ? __init_waitqueue_head+0x36/0x50
 kernel:  mount_fs+0x89/0x130
 kernel:  vfs_kern_mount+0x69/0x160

---------

Thanks, Anand

Quoted 19 lines
fs/btrfs/extent_io.c +2 -3
  	set_page_extent_mapped(page);
 
  	end = page_end;

@@ -3012,12 +3016,11 @@ static int __do_readpage(struct extent_io_tree *tree,
  		 * non-optimal behavior (submitting 2 bios for the same extent).
  		 */
  		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags) &&
-		    prev_em_start && *prev_em_start != (u64)-1 &&
+		    *prev_em_start != (u64)-1 &&
  		    *prev_em_start != em->orig_start)
  			force_bio_submit = true;
 
-		if (prev_em_start)
-			*prev_em_start = em->orig_start;
+		*prev_em_start = em->orig_start;
 
  		free_extent_map(em);
  		em = NULL;
#4 lore raw

Re: [PATCH 02/22] btrfs: assume that prev_em_start is always valid in __do_readpage

Quoted 25 lines · Anand Jain
Quoted 24 lines · David Sterba

All callers pass a valid pointer, we can remove the redundant checks.

Signed-off-by David Sterba <[email protected]>
---
  fs/btrfs/extent_io.c | 9 ++++++---
  1 file changed, 6 insertions(+), 3 deletions(-)
fs/btrfs/extent_io.c +2 -0
index d00d5a59ff21..36514baa661e 100644
@@ -2875,6 +2875,8 @@ __get_extent_map(struct inode *inode, struct page *page, size_t pg_offset,
   * handlers)
   * XXX JDM: This needs looking at to ensure proper page locking
   * return 0 on success, otherwise return error
+ *
+ * @prev_em_start:	return value of previous em start value; must be valid
   */
  static int __do_readpage(struct extent_io_tree *tree,
  			 struct page *page,

@@ -2903,6 +2905,8 @@ static int __do_readpage(struct extent_io_tree *tree,
  	size_t blocksize = inode->i_sb->s_blocksize;
  	unsigned long this_bio_flag = 0;
Quoted 6 lines

fs/btrfs/extent_io.c static int __do_readpage(struct extent_io_tree *tree, · line 2909

Quoted 2 lines
fs/btrfs/extent_io.c +2 -0
+	ASSERT(prev_em_start);
+

mount is failing with this patch as in this stack below the prev_em_start is NULL.

Right, I've removed the patch.

author maintainer
#5 lore raw

PATCH 3/22 btrfs: remove redundant variable in __do_readpage

In reply to David Sterba

The value of page_end is only stored to end, no other use.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
fs/btrfs/extent_io.c +1 -3
index 36514baa661e..ffcbdb2390a1 100644
@@ -2888,8 +2888,7 @@ static int __do_readpage(struct extent_io_tree *tree,
 {
 	struct inode *inode = page->mapping->host;
 	u64 start = page_offset(page);
-	u64 page_end = start + PAGE_SIZE - 1;
-	u64 end;
+	const u64 end = start + PAGE_SIZE - 1;
 	u64 cur = start;
 	u64 extent_offset;
 	u64 last_byte = i_size_read(inode);

@@ -2909,7 +2908,6 @@ static int __do_readpage(struct extent_io_tree *tree,
 
 	set_page_extent_mapped(page);
 
-	end = page_end;
 	if (!PageUptodate(page)) {
 		if (cleancache_get_page(page) == 0) {
 			BUG_ON(blocksize != PAGE_SIZE);
Signature · 1 line
2.16.2
author maintainer
#6 lore raw

PATCH 4/22 btrfs: cleanup merging conditions in submit_extent_page

In reply to David Sterba

The merge call was factored out to a separate helper but it's a trivial one and arguably we can opencode it and cache the value.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)
fs/btrfs/extent_io.c +9 -17
index ffcbdb2390a1..2fbc58b0b908 100644
@@ -2744,18 +2744,6 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 	return blk_status_to_errno(ret);
 }
 
-static int merge_bio(struct extent_io_tree *tree, struct page *page,
-		     unsigned long offset, size_t size, struct bio *bio,
-		     unsigned long bio_flags)
-{
-	int ret = 0;
-	if (tree->ops)
-		ret = tree->ops->merge_bio_hook(page, offset, size, bio,
-						bio_flags);
-	return ret;
-
-}
-
 /*
  * @opf:	bio REQ_OP_* and REQ_* flags as one value
  * @bio_ret:	must be valid pointer, newly allocated bio will be stored there

@@ -2774,23 +2762,27 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 {
 	int ret = 0;
 	struct bio *bio;
-	int contig = 0;
-	int old_compressed = prev_bio_flags & EXTENT_BIO_COMPRESSED;
 	size_t page_size = min_t(size_t, size, PAGE_SIZE);
 	sector_t sector = offset >> 9;
 
 	ASSERT(bio_ret);
 
 	if (*bio_ret) {
+		bool contig;
+		bool can_merge = true;
+
 		bio = *bio_ret;
-		if (old_compressed)
+		if (prev_bio_flags & EXTENT_BIO_COMPRESSED)
 			contig = bio->bi_iter.bi_sector == sector;
 		else
 			contig = bio_end_sector(bio) == sector;
 
-		if (prev_bio_flags != bio_flags || !contig ||
+		if (tree->ops && tree->ops->merge_bio_hook(page, offset,
+					page_size, bio, bio_flags))
+			can_merge = false;
+
+		if (prev_bio_flags != bio_flags || !contig || !can_merge ||
 		    force_bio_submit ||
-		    merge_bio(tree, page, pg_offset, page_size, bio, bio_flags) ||
 		    bio_add_page(bio, page, page_size, pg_offset) < page_size) {
 			ret = submit_one_bio(bio, mirror_num, prev_bio_flags);
 			if (ret < 0) {
Signature · 1 line
2.16.2
author maintainer
#7 lore raw

PATCH 5/22 btrfs: document more parameters of submit_extent_page

In reply to David Sterba
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
fs/btrfs/extent_io.c +12 -0
index 2fbc58b0b908..f1d199c8883e 100644
@@ -2746,7 +2746,19 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 
 /*
  * @opf:	bio REQ_OP_* and REQ_* flags as one value
+ * @tree:	tree so we can call our merge_bio hook
+ * @wbc:	optional writeback control for io accounting
+ * @page:	page to add to the bio
+ * @pg_offset:	offset of the new bio or to check whether we are adding
+ *              a contiguous page to the previous one
+ * @size:	portion of page that we want to write
+ * @offset:	starting offset in the page
+ * @bdev:	attach newly created bios to this bdev
  * @bio_ret:	must be valid pointer, newly allocated bio will be stored there
+ * @end_io_func:     end_io callback for new bio
+ * @mirror_num:	     desired mirror to read/write
+ * @prev_bio_flags:  flags of previous bio to see if we can merge the current one
+ * @bio_flags:	flags of the current bio to see if we can merge them
  */
 static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 			      struct writeback_control *wbc,
Signature · 1 line
2.16.2
author maintainer
#8 lore raw

PATCH 6/22 btrfs: drop fs_info parameter from tree_mod_log_set_node_key

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)
fs/btrfs/ctree.c +8 -11
index b88a79e69ddf..11e985ca0bc2 100644
@@ -877,13 +877,12 @@ tree_mod_log_eb_move(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 	BUG_ON(ret < 0);
 }
 
-static noinline void
-tree_mod_log_set_node_key(struct btrfs_fs_info *fs_info,
-			  struct extent_buffer *eb, int slot, int atomic)
+static noinline void tree_mod_log_set_node_key(struct extent_buffer *eb,
+		int slot, int atomic)
 {
 	int ret;
 
-	ret = tree_mod_log_insert_key(fs_info, eb, slot,
+	ret = tree_mod_log_insert_key(eb->fs_info, eb, slot,
 					MOD_LOG_KEY_REPLACE,
 					atomic ? GFP_ATOMIC : GFP_NOFS);
 	BUG_ON(ret < 0);

@@ -2007,8 +2006,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		} else {
 			struct btrfs_disk_key right_key;
 			btrfs_node_key(right, &right_key, 0);
-			tree_mod_log_set_node_key(fs_info, parent,
-						  pslot + 1, 0);
+			tree_mod_log_set_node_key(parent, pslot + 1, 0);
 			btrfs_set_node_key(parent, &right_key, pslot + 1);
 			btrfs_mark_buffer_dirty(parent);
 		}

@@ -2052,7 +2050,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		/* update the parent key to reflect our changes */
 		struct btrfs_disk_key mid_key;
 		btrfs_node_key(mid, &mid_key, 0);
-		tree_mod_log_set_node_key(fs_info, parent, pslot, 0);
+		tree_mod_log_set_node_key(parent, pslot, 0);
 		btrfs_set_node_key(parent, &mid_key, pslot);
 		btrfs_mark_buffer_dirty(parent);
 	}

@@ -2153,7 +2151,7 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 			struct btrfs_disk_key disk_key;
 			orig_slot += left_nr;
 			btrfs_node_key(mid, &disk_key, 0);
-			tree_mod_log_set_node_key(fs_info, parent, pslot, 0);
+			tree_mod_log_set_node_key(parent, pslot, 0);
 			btrfs_set_node_key(parent, &disk_key, pslot);
 			btrfs_mark_buffer_dirty(parent);
 			if (btrfs_header_nritems(left) > orig_slot) {

@@ -2207,8 +2205,7 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 			struct btrfs_disk_key disk_key;
 
 			btrfs_node_key(right, &disk_key, 0);
-			tree_mod_log_set_node_key(fs_info, parent,
-						  pslot + 1, 0);
+			tree_mod_log_set_node_key(parent, pslot + 1, 0);
 			btrfs_set_node_key(parent, &disk_key, pslot + 1);
 			btrfs_mark_buffer_dirty(parent);
 

@@ -3167,7 +3164,7 @@ static void fixup_low_keys(struct btrfs_fs_info *fs_info,
 		if (!path->nodes[i])
 			break;
 		t = path->nodes[i];
-		tree_mod_log_set_node_key(fs_info, t, tslot, 1);
+		tree_mod_log_set_node_key(t, tslot, 1);
 		btrfs_set_node_key(t, key, tslot);
 		btrfs_mark_buffer_dirty(path->nodes[i]);
 		if (tslot != 0)
Signature · 1 line
2.16.2
author maintainer
#9 lore raw

PATCH 7/22 btrfs: drop fs_info parameter from tree_mod_log_insert_move

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)
fs/btrfs/ctree.c +10 -13
index 11e985ca0bc2..3b707af76579 100644
@@ -564,10 +564,8 @@ tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
 	return ret;
 }
 
-static noinline int
-tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
-			 struct extent_buffer *eb, int dst_slot, int src_slot,
-			 int nr_items)
+static noinline int tree_mod_log_insert_move(struct extent_buffer *eb,
+		int dst_slot, int src_slot, int nr_items)
 {
 	struct tree_mod_elem *tm = NULL;
 	struct tree_mod_elem **tm_list = NULL;

@@ -575,7 +573,7 @@ tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
 	int i;
 	int locked = 0;
 
-	if (!tree_mod_need_log(fs_info, eb))
+	if (!tree_mod_need_log(eb->fs_info, eb))
 		return 0;
 
 	tm_list = kcalloc(nr_items, sizeof(struct tree_mod_elem *), GFP_NOFS);

@@ -603,7 +601,7 @@ tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	if (tree_mod_dont_log(fs_info, eb))
+	if (tree_mod_dont_log(eb->fs_info, eb))
 		goto free_tms;
 	locked = 1;
 

@@ -613,26 +611,26 @@ tree_mod_log_insert_move(struct btrfs_fs_info *fs_info,
 	 * buffer, i.e. dst_slot < src_slot.
 	 */
 	for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) {
-		ret = __tree_mod_log_insert(fs_info, tm_list[i]);
+		ret = __tree_mod_log_insert(eb->fs_info, tm_list[i]);
 		if (ret)
 			goto free_tms;
 	}
 
-	ret = __tree_mod_log_insert(fs_info, tm);
+	ret = __tree_mod_log_insert(eb->fs_info, tm);
 	if (ret)
 		goto free_tms;
-	tree_mod_log_write_unlock(fs_info);
+	tree_mod_log_write_unlock(eb->fs_info);
 	kfree(tm_list);
 
 	return 0;
 free_tms:
 	for (i = 0; i < nr_items; i++) {
 		if (tm_list[i] && !RB_EMPTY_NODE(&tm_list[i]->node))
-			rb_erase(&tm_list[i]->node, &fs_info->tree_mod_log);
+			rb_erase(&tm_list[i]->node, &eb->fs_info->tree_mod_log);
 		kfree(tm_list[i]);
 	}
 	if (locked)
-		tree_mod_log_write_unlock(fs_info);
+		tree_mod_log_write_unlock(eb->fs_info);
 	kfree(tm_list);
 	kfree(tm);
 

@@ -872,8 +870,7 @@ tree_mod_log_eb_move(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 		     int dst_offset, int src_offset, int nr_items)
 {
 	int ret;
-	ret = tree_mod_log_insert_move(fs_info, dst, dst_offset, src_offset,
-				       nr_items);
+	ret = tree_mod_log_insert_move(dst, dst_offset, src_offset, nr_items);
 	BUG_ON(ret < 0);
 }
Signature · 1 line
2.16.2
author maintainer
#10 lore raw

PATCH 8/22 btrfs: drop fs_info parameter from tree_mod_log_insert_key

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)
fs/btrfs/ctree.c +12 -15
index 3b707af76579..60e27bef0e27 100644
@@ -536,28 +536,26 @@ alloc_tree_mod_elem(struct extent_buffer *eb, int slot,
 	return tm;
 }
 
-static noinline int
-tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
-			struct extent_buffer *eb, int slot,
-			enum mod_log_op op, gfp_t flags)
+static noinline int tree_mod_log_insert_key(struct extent_buffer *eb, int slot,
+		enum mod_log_op op, gfp_t flags)
 {
 	struct tree_mod_elem *tm;
 	int ret;
 
-	if (!tree_mod_need_log(fs_info, eb))
+	if (!tree_mod_need_log(eb->fs_info, eb))
 		return 0;
 
 	tm = alloc_tree_mod_elem(eb, slot, op, flags);
 	if (!tm)
 		return -ENOMEM;
 
-	if (tree_mod_dont_log(fs_info, eb)) {
+	if (tree_mod_dont_log(eb->fs_info, eb)) {
 		kfree(tm);
 		return 0;
 	}
 
-	ret = __tree_mod_log_insert(fs_info, tm);
-	tree_mod_log_write_unlock(fs_info);
+	ret = __tree_mod_log_insert(eb->fs_info, tm);
+	tree_mod_log_write_unlock(eb->fs_info);
 	if (ret)
 		kfree(tm);
 

@@ -879,8 +877,7 @@ static noinline void tree_mod_log_set_node_key(struct extent_buffer *eb,
 {
 	int ret;
 
-	ret = tree_mod_log_insert_key(eb->fs_info, eb, slot,
-					MOD_LOG_KEY_REPLACE,
+	ret = tree_mod_log_insert_key(eb, slot, MOD_LOG_KEY_REPLACE,
 					atomic ? GFP_ATOMIC : GFP_NOFS);
 	BUG_ON(ret < 0);
 }

@@ -1178,7 +1175,7 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
 		add_root_to_dirty_list(root);
 	} else {
 		WARN_ON(trans->transid != btrfs_header_generation(parent));
-		tree_mod_log_insert_key(fs_info, parent, parent_slot,
+		tree_mod_log_insert_key(parent, parent_slot,
 					MOD_LOG_KEY_REPLACE, GFP_NOFS);
 		btrfs_set_node_blockptr(parent, parent_slot,
 					cow->start);

@@ -3441,8 +3438,8 @@ static void insert_ptr(struct btrfs_trans_handle *trans,
 			      (nritems - slot) * sizeof(struct btrfs_key_ptr));
 	}
 	if (level) {
-		ret = tree_mod_log_insert_key(fs_info, lower, slot,
-					      MOD_LOG_KEY_ADD, GFP_NOFS);
+		ret = tree_mod_log_insert_key(lower, slot, MOD_LOG_KEY_ADD,
+				GFP_NOFS);
 		BUG_ON(ret < 0);
 	}
 	btrfs_set_node_key(lower, key, slot);

@@ -4914,8 +4911,8 @@ static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 			      sizeof(struct btrfs_key_ptr) *
 			      (nritems - slot - 1));
 	} else if (level) {
-		ret = tree_mod_log_insert_key(fs_info, parent, slot,
-					      MOD_LOG_KEY_REMOVE, GFP_NOFS);
+		ret = tree_mod_log_insert_key(parent, slot, MOD_LOG_KEY_REMOVE,
+				GFP_NOFS);
 		BUG_ON(ret < 0);
 	}
Signature · 1 line
2.16.2
author maintainer
#11 lore raw

PATCH 9/22 btrfs: drop fs_info parameter from tree_mod_log_free_eb

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
fs/btrfs/ctree.c +6 -9
index 60e27bef0e27..6d2f8562bb36 100644
@@ -41,8 +41,6 @@ static int balance_node_right(struct btrfs_trans_handle *trans,
 			      struct extent_buffer *src_buf);
 static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 		    int level, int slot);
-static int tree_mod_log_free_eb(struct btrfs_fs_info *fs_info,
-				 struct extent_buffer *eb);
 
 struct btrfs_path *btrfs_alloc_path(void)
 {

@@ -882,8 +880,7 @@ static noinline void tree_mod_log_set_node_key(struct extent_buffer *eb,
 	BUG_ON(ret < 0);
 }
 
-static noinline int
-tree_mod_log_free_eb(struct btrfs_fs_info *fs_info, struct extent_buffer *eb)
+static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 {
 	struct tree_mod_elem **tm_list = NULL;
 	int nritems = 0;

@@ -893,7 +890,7 @@ tree_mod_log_free_eb(struct btrfs_fs_info *fs_info, struct extent_buffer *eb)
 	if (btrfs_header_level(eb) == 0)
 		return 0;
 
-	if (!tree_mod_need_log(fs_info, NULL))
+	if (!tree_mod_need_log(eb->fs_info, NULL))
 		return 0;
 
 	nritems = btrfs_header_nritems(eb);

@@ -910,11 +907,11 @@ tree_mod_log_free_eb(struct btrfs_fs_info *fs_info, struct extent_buffer *eb)
 		}
 	}
 
-	if (tree_mod_dont_log(fs_info, eb))
+	if (tree_mod_dont_log(eb->fs_info, eb))
 		goto free_tms;
 
-	ret = __tree_mod_log_free_eb(fs_info, tm_list, nritems);
-	tree_mod_log_write_unlock(fs_info);
+	ret = __tree_mod_log_free_eb(eb->fs_info, tm_list, nritems);
+	tree_mod_log_write_unlock(eb->fs_info);
 	if (ret)
 		goto free_tms;
 	kfree(tm_list);

@@ -1183,7 +1180,7 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
 					      trans->transid);
 		btrfs_mark_buffer_dirty(parent);
 		if (last_ref) {
-			ret = tree_mod_log_free_eb(fs_info, buf);
+			ret = tree_mod_log_free_eb(buf);
 			if (ret) {
 				btrfs_abort_transaction(trans, ret);
 				return ret;
Signature · 1 line
2.16.2
author maintainer
#12 lore raw

PATCH 10/22 btrfs: drop fs_info parameter from tree_mod_log_free_eb

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
fs/btrfs/ctree.c +4 -7
index 6d2f8562bb36..632beadf5725 100644
@@ -654,12 +654,10 @@ __tree_mod_log_free_eb(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-static noinline int
-tree_mod_log_insert_root(struct btrfs_fs_info *fs_info,
-			 struct extent_buffer *old_root,
-			 struct extent_buffer *new_root,
-			 int log_removal)
+static noinline int tree_mod_log_insert_root(struct extent_buffer *old_root,
+			 struct extent_buffer *new_root, int log_removal)
 {
+	struct btrfs_fs_info *fs_info = old_root->fs_info;
 	struct tree_mod_elem *tm = NULL;
 	struct tree_mod_elem **tm_list = NULL;
 	int nritems = 0;

@@ -932,8 +930,7 @@ tree_mod_log_set_root_pointer(struct btrfs_root *root,
 			      int log_removal)
 {
 	int ret;
-	ret = tree_mod_log_insert_root(root->fs_info, root->node,
-				       new_root_node, log_removal);
+	ret = tree_mod_log_insert_root(root->node, new_root_node, log_removal);
 	BUG_ON(ret < 0);
 }
Signature · 1 line
2.16.2
author maintainer
#13 lore raw

PATCH 11/22 btrfs: drop unused fs_info parameter from tree_mod_log_eb_move

In reply to David Sterba
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)
fs/btrfs/ctree.c +6 -7
index 632beadf5725..5585433eac40 100644
@@ -859,8 +859,7 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 	return ret;
 }
 
-static inline void
-tree_mod_log_eb_move(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
+static inline void tree_mod_log_eb_move(struct extent_buffer *dst,
 		     int dst_offset, int src_offset, int nr_items)
 {
 	int ret;

@@ -3305,7 +3304,7 @@ static int balance_node_right(struct btrfs_trans_handle *trans,
 	if (max_push < push_items)
 		push_items = max_push;
 
-	tree_mod_log_eb_move(fs_info, dst, push_items, 0, dst_nritems);
+	tree_mod_log_eb_move(dst, push_items, 0, dst_nritems);
 	memmove_extent_buffer(dst, btrfs_node_key_ptr_offset(push_items),
 				      btrfs_node_key_ptr_offset(0),
 				      (dst_nritems) *

@@ -3424,8 +3423,8 @@ static void insert_ptr(struct btrfs_trans_handle *trans,
 	BUG_ON(nritems == BTRFS_NODEPTRS_PER_BLOCK(fs_info));
 	if (slot != nritems) {
 		if (level)
-			tree_mod_log_eb_move(fs_info, lower, slot + 1,
-					     slot, nritems - slot);
+			tree_mod_log_eb_move(lower, slot + 1, slot,
+					nritems - slot);
 		memmove_extent_buffer(lower,
 			      btrfs_node_key_ptr_offset(slot + 1),
 			      btrfs_node_key_ptr_offset(slot),

@@ -4897,8 +4896,8 @@ static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 	nritems = btrfs_header_nritems(parent);
 	if (slot != nritems - 1) {
 		if (level)
-			tree_mod_log_eb_move(fs_info, parent, slot,
-					     slot + 1, nritems - slot - 1);
+			tree_mod_log_eb_move(parent, slot, slot + 1,
+					nritems - slot - 1);
 		memmove_extent_buffer(parent,
 			      btrfs_node_key_ptr_offset(slot),
 			      btrfs_node_key_ptr_offset(slot + 1),
Signature · 1 line
2.16.2
author maintainer
#14 lore raw

PATCH 12/22 btrfs: embed tree_mod_move structure to tree_mod_elem

In reply to David Sterba

The tree_mod_move is not used anywhere and can be embedded as anonymous structure.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
fs/btrfs/ctree.c +4 -6
index 5585433eac40..e53af58697db 100644
@@ -299,11 +299,6 @@ enum mod_log_op {
 	MOD_LOG_ROOT_REPLACE,
 };
 
-struct tree_mod_move {
-	int dst_slot;
-	int nr_items;
-};
-
 struct tree_mod_root {
 	u64 logical;
 	u8 level;

@@ -326,7 +321,10 @@ struct tree_mod_elem {
 	u64 blockptr;
 
 	/* this is used for op == MOD_LOG_MOVE_KEYS */
-	struct tree_mod_move move;
+	struct {
+		int dst_slot;
+		int nr_items;
+	} move;
 
 	/* this is used for op == MOD_LOG_ROOT_REPLACE */
 	struct tree_mod_root old_root;
Signature · 1 line
2.16.2
author maintainer
#15 lore raw

PATCH 13/22 btrfs: drop fs_info parameter from __tree_mod_log_oldest_root

In reply to David Sterba

It's provided by the extent_buffer.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
fs/btrfs/ctree.c +5 -6
index e53af58697db..716c140798c3 100644
@@ -1195,9 +1195,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
  * returns the logical address of the oldest predecessor of the given root.
  * entries older than time_seq are ignored.
  */
-static struct tree_mod_elem *
-__tree_mod_log_oldest_root(struct btrfs_fs_info *fs_info,
-			   struct extent_buffer *eb_root, u64 time_seq)
+static struct tree_mod_elem *__tree_mod_log_oldest_root(
+		struct extent_buffer *eb_root, u64 time_seq)
 {
 	struct tree_mod_elem *tm;
 	struct tree_mod_elem *found = NULL;

@@ -1214,7 +1213,7 @@ __tree_mod_log_oldest_root(struct btrfs_fs_info *fs_info,
 	 * first operation that's logged for this root.
 	 */
 	while (1) {
-		tm = tree_mod_log_search_oldest(fs_info, root_logical,
+		tm = tree_mod_log_search_oldest(eb_root->fs_info, root_logical,
 						time_seq);
 		if (!looped && !tm)
 			return NULL;

@@ -1404,7 +1403,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
 	u64 logical;
 
 	eb_root = btrfs_read_lock_root_node(root);
-	tm = __tree_mod_log_oldest_root(fs_info, eb_root, time_seq);
+	tm = __tree_mod_log_oldest_root(eb_root, time_seq);
 	if (!tm)
 		return eb_root;
 

@@ -1468,7 +1467,7 @@ int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq)
 	int level;
 	struct extent_buffer *eb_root = btrfs_root_node(root);
 
-	tm = __tree_mod_log_oldest_root(root->fs_info, eb_root, time_seq);
+	tm = __tree_mod_log_oldest_root(eb_root, time_seq);
 	if (tm && tm->op == MOD_LOG_ROOT_REPLACE) {
 		level = tm->old_root.level;
 	} else {
Signature · 1 line
2.16.2
author maintainer
#16 lore raw

PATCH 14/22 btrfs: remove trivial locking wrappers of tree mod log

In reply to David Sterba

The wrappers are trivial and do bring any extra value on top of the plain locking primitives.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 58 +++++++++++++++++++-------------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)
fs/btrfs/ctree.c +19 -39
index 716c140798c3..7d39ef6ce20a 100644
@@ -330,26 +330,6 @@ struct tree_mod_elem {
 	struct tree_mod_root old_root;
 };
 
-static inline void tree_mod_log_read_lock(struct btrfs_fs_info *fs_info)
-{
-	read_lock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_read_unlock(struct btrfs_fs_info *fs_info)
-{
-	read_unlock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_write_lock(struct btrfs_fs_info *fs_info)
-{
-	write_lock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_write_unlock(struct btrfs_fs_info *fs_info)
-{
-	write_unlock(&fs_info->tree_mod_log_lock);
-}
-
 /*
  * Pull a new tree mod seq number for our operation.
  */

@@ -369,14 +349,14 @@ static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
 u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info *fs_info,
 			   struct seq_list *elem)
 {
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	spin_lock(&fs_info->tree_mod_seq_lock);
 	if (!elem->seq) {
 		elem->seq = btrfs_inc_tree_mod_seq(fs_info);
 		list_add_tail(&elem->list, &fs_info->tree_mod_seq_list);
 	}
 	spin_unlock(&fs_info->tree_mod_seq_lock);
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 
 	return elem->seq;
 }

@@ -418,7 +398,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
 	 * anything that's lower than the lowest existing (read: blocked)
 	 * sequence number can be removed from the tree.
 	 */
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	tm_root = &fs_info->tree_mod_log;
 	for (node = rb_first(tm_root); node; node = next) {
 		next = rb_next(node);

@@ -428,7 +408,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
 		rb_erase(node, tm_root);
 		kfree(tm);
 	}
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 }
 
 /*

@@ -439,7 +419,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
  * for root replace operations, or the logical address of the affected
  * block for all other operations.
  *
- * Note: must be called with write lock (tree_mod_log_write_lock).
+ * Note: must be called with write lock for fs_info_;;tree_mod_log_lock.
  */
 static noinline int
 __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)

@@ -477,7 +457,7 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
  * Determines if logging can be omitted. Returns 1 if it can. Otherwise, it
  * returns zero with the tree_mod_log_lock acquired. The caller must hold
  * this until all tree mod log insertions are recorded in the rb tree and then
- * call tree_mod_log_write_unlock() to release.
+ * write unlock fs_info::tree_mod_log_lock.
  */
 static inline int tree_mod_dont_log(struct btrfs_fs_info *fs_info,
 				    struct extent_buffer *eb) {

@@ -487,9 +467,9 @@ static inline int tree_mod_dont_log(struct btrfs_fs_info *fs_info,
 	if (eb && btrfs_header_level(eb) == 0)
 		return 1;
 
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	if (list_empty(&(fs_info)->tree_mod_seq_list)) {
-		tree_mod_log_write_unlock(fs_info);
+		write_unlock(&fs_info->tree_mod_log_lock);
 		return 1;
 	}
 

@@ -551,7 +531,7 @@ static noinline int tree_mod_log_insert_key(struct extent_buffer *eb, int slot,
 	}
 
 	ret = __tree_mod_log_insert(eb->fs_info, tm);
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
 		kfree(tm);
 

@@ -613,7 +593,7 @@ static noinline int tree_mod_log_insert_move(struct extent_buffer *eb,
 	ret = __tree_mod_log_insert(eb->fs_info, tm);
 	if (ret)
 		goto free_tms;
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return 0;

@@ -624,7 +604,7 @@ static noinline int tree_mod_log_insert_move(struct extent_buffer *eb,
 		kfree(tm_list[i]);
 	}
 	if (locked)
-		tree_mod_log_write_unlock(eb->fs_info);
+		write_unlock(&eb->fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 	kfree(tm);
 

@@ -703,7 +683,7 @@ static noinline int tree_mod_log_insert_root(struct extent_buffer *old_root,
 	if (!ret)
 		ret = __tree_mod_log_insert(fs_info, tm);
 
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 	if (ret)
 		goto free_tms;
 	kfree(tm_list);

@@ -730,7 +710,7 @@ __tree_mod_log_search(struct btrfs_fs_info *fs_info, u64 start, u64 min_seq,
 	struct tree_mod_elem *cur = NULL;
 	struct tree_mod_elem *found = NULL;
 
-	tree_mod_log_read_lock(fs_info);
+	read_lock(&fs_info->tree_mod_log_lock);
 	tm_root = &fs_info->tree_mod_log;
 	node = tm_root->rb_node;
 	while (node) {

@@ -758,7 +738,7 @@ __tree_mod_log_search(struct btrfs_fs_info *fs_info, u64 start, u64 min_seq,
 			break;
 		}
 	}
-	tree_mod_log_read_unlock(fs_info);
+	read_unlock(&fs_info->tree_mod_log_lock);
 
 	return found;
 }

@@ -839,7 +819,7 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 			goto free_tms;
 	}
 
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return 0;

@@ -851,7 +831,7 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 		kfree(tm_list[i]);
 	}
 	if (locked)
-		tree_mod_log_write_unlock(fs_info);
+		write_unlock(&fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return ret;

@@ -906,7 +886,7 @@ static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 		goto free_tms;
 
 	ret = __tree_mod_log_free_eb(eb->fs_info, tm_list, nritems);
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
 		goto free_tms;
 	kfree(tm_list);

@@ -1262,7 +1242,7 @@ __tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
 	unsigned long p_size = sizeof(struct btrfs_key_ptr);
 
 	n = btrfs_header_nritems(eb);
-	tree_mod_log_read_lock(fs_info);
+	read_lock(&fs_info->tree_mod_log_lock);
 	while (tm && tm->seq >= time_seq) {
 		/*
 		 * all the operations are recorded with the operator used for

@@ -1317,7 +1297,7 @@ __tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
 		if (tm->logical != first_tm->logical)
 			break;
 	}
-	tree_mod_log_read_unlock(fs_info);
+	read_unlock(&fs_info->tree_mod_log_lock);
 	btrfs_set_header_nritems(eb, n);
 }
Signature · 1 line
2.16.2
Reviewed-by on v1 14/22
#17 lore raw

Re: [PATCH 14/22] btrfs: remove trivial locking wrappers of tree mod log

Quoted 1 line · David Sterba

The wrappers are trivial and do bring any extra value on top of the

nit: s/do/don't/ ?

Quoted 3 lines

plain locking primitives.

Signed-off-by David Sterba <[email protected]>

Just 2 minor nits, otherwise LGTM:

Reviewed-by Nikolay Borisov <[email protected]>
Quoted 71 lines
---
 fs/btrfs/ctree.c | 58 +++++++++++++++++++-------------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)
fs/btrfs/ctree.c +4 -24
index 716c140798c3..7d39ef6ce20a 100644
@@ -330,26 +330,6 @@ struct tree_mod_elem {
 	struct tree_mod_root old_root;
 };
 
-static inline void tree_mod_log_read_lock(struct btrfs_fs_info *fs_info)
-{
-	read_lock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_read_unlock(struct btrfs_fs_info *fs_info)
-{
-	read_unlock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_write_lock(struct btrfs_fs_info *fs_info)
-{
-	write_lock(&fs_info->tree_mod_log_lock);
-}
-
-static inline void tree_mod_log_write_unlock(struct btrfs_fs_info *fs_info)
-{
-	write_unlock(&fs_info->tree_mod_log_lock);
-}
-
 /*
  * Pull a new tree mod seq number for our operation.
  */

@@ -369,14 +349,14 @@ static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
 u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info *fs_info,
 			   struct seq_list *elem)
 {
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	spin_lock(&fs_info->tree_mod_seq_lock);
 	if (!elem->seq) {
 		elem->seq = btrfs_inc_tree_mod_seq(fs_info);
 		list_add_tail(&elem->list, &fs_info->tree_mod_seq_list);
 	}
 	spin_unlock(&fs_info->tree_mod_seq_lock);
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 
 	return elem->seq;
 }

@@ -418,7 +398,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
 	 * anything that's lower than the lowest existing (read: blocked)
 	 * sequence number can be removed from the tree.
 	 */
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	tm_root = &fs_info->tree_mod_log;
 	for (node = rb_first(tm_root); node; node = next) {
 		next = rb_next(node);

@@ -428,7 +408,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
 		rb_erase(node, tm_root);
 		kfree(tm);
 	}
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 }
 
 /*

fs/btrfs/ctree.c void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info, · line 422

Quoted 5 lines
fs/btrfs/ctree.c +1 -1
@@ -439,7 +419,7 @@ void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
  * for root replace operations, or the logical address of the affected
  * block for all other operations.
  *
- * Note: must be called with write lock (tree_mod_log_write_lock).
+ * Note: must be called with write lock for fs_info_;;tree_mod_log_lock.

nit: s/;;/::/

Quoted 124 lines
fs/btrfs/ctree.c +14 -14
  */
 static noinline int
 __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)

@@ -477,7 +457,7 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info, struct tree_mod_elem *tm)
  * Determines if logging can be omitted. Returns 1 if it can. Otherwise, it
  * returns zero with the tree_mod_log_lock acquired. The caller must hold
  * this until all tree mod log insertions are recorded in the rb tree and then
- * call tree_mod_log_write_unlock() to release.
+ * write unlock fs_info::tree_mod_log_lock.
  */
 static inline int tree_mod_dont_log(struct btrfs_fs_info *fs_info,
 				    struct extent_buffer *eb) {

@@ -487,9 +467,9 @@ static inline int tree_mod_dont_log(struct btrfs_fs_info *fs_info,
 	if (eb && btrfs_header_level(eb) == 0)
 		return 1;
 
-	tree_mod_log_write_lock(fs_info);
+	write_lock(&fs_info->tree_mod_log_lock);
 	if (list_empty(&(fs_info)->tree_mod_seq_list)) {
-		tree_mod_log_write_unlock(fs_info);
+		write_unlock(&fs_info->tree_mod_log_lock);
 		return 1;
 	}
 

@@ -551,7 +531,7 @@ static noinline int tree_mod_log_insert_key(struct extent_buffer *eb, int slot,
 	}
 
 	ret = __tree_mod_log_insert(eb->fs_info, tm);
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
 		kfree(tm);
 

@@ -613,7 +593,7 @@ static noinline int tree_mod_log_insert_move(struct extent_buffer *eb,
 	ret = __tree_mod_log_insert(eb->fs_info, tm);
 	if (ret)
 		goto free_tms;
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return 0;

@@ -624,7 +604,7 @@ static noinline int tree_mod_log_insert_move(struct extent_buffer *eb,
 		kfree(tm_list[i]);
 	}
 	if (locked)
-		tree_mod_log_write_unlock(eb->fs_info);
+		write_unlock(&eb->fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 	kfree(tm);
 

@@ -703,7 +683,7 @@ static noinline int tree_mod_log_insert_root(struct extent_buffer *old_root,
 	if (!ret)
 		ret = __tree_mod_log_insert(fs_info, tm);
 
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 	if (ret)
 		goto free_tms;
 	kfree(tm_list);

@@ -730,7 +710,7 @@ __tree_mod_log_search(struct btrfs_fs_info *fs_info, u64 start, u64 min_seq,
 	struct tree_mod_elem *cur = NULL;
 	struct tree_mod_elem *found = NULL;
 
-	tree_mod_log_read_lock(fs_info);
+	read_lock(&fs_info->tree_mod_log_lock);
 	tm_root = &fs_info->tree_mod_log;
 	node = tm_root->rb_node;
 	while (node) {

@@ -758,7 +738,7 @@ __tree_mod_log_search(struct btrfs_fs_info *fs_info, u64 start, u64 min_seq,
 			break;
 		}
 	}
-	tree_mod_log_read_unlock(fs_info);
+	read_unlock(&fs_info->tree_mod_log_lock);
 
 	return found;
 }

@@ -839,7 +819,7 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 			goto free_tms;
 	}
 
-	tree_mod_log_write_unlock(fs_info);
+	write_unlock(&fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return 0;

@@ -851,7 +831,7 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 		kfree(tm_list[i]);
 	}
 	if (locked)
-		tree_mod_log_write_unlock(fs_info);
+		write_unlock(&fs_info->tree_mod_log_lock);
 	kfree(tm_list);
 
 	return ret;

@@ -906,7 +886,7 @@ static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 		goto free_tms;
 
 	ret = __tree_mod_log_free_eb(eb->fs_info, tm_list, nritems);
-	tree_mod_log_write_unlock(eb->fs_info);
+	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
 		goto free_tms;
 	kfree(tm_list);

@@ -1262,7 +1242,7 @@ __tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
 	unsigned long p_size = sizeof(struct btrfs_key_ptr);
 
 	n = btrfs_header_nritems(eb);
-	tree_mod_log_read_lock(fs_info);
+	read_lock(&fs_info->tree_mod_log_lock);
 	while (tm && tm->seq >= time_seq) {
 		/*
 		 * all the operations are recorded with the operator used for

@@ -1317,7 +1297,7 @@ __tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct extent_buffer *eb,
 		if (tm->logical != first_tm->logical)
 			break;
 	}
-	tree_mod_log_read_unlock(fs_info);
+	read_unlock(&fs_info->tree_mod_log_lock);
 	btrfs_set_header_nritems(eb, n);
 }
#18 lore raw

Re: [PATCH 14/22] btrfs: remove trivial locking wrappers of tree mod log

Quoted 6 lines · Nikolay Borisov
Quoted 1 line · David Sterba

The wrappers are trivial and do bring any extra value on top of the

nit: s/do/don't/ ?

Right, fixed.

Quoted 11 lines
Quoted 3 lines

plain locking primitives.

Signed-off-by David Sterba <[email protected]>

Just 2 minor nits, otherwise LGTM:

Reviewed-by Nikolay Borisov <[email protected]>
Quoted 2 lines

- * Note: must be called with write lock (tree_mod_log_write_lock).
+ * Note: must be called with write lock for fs_info_;;tree_mod_log_lock.

nit: s/;;/::/

Fixed, thanks.

author maintainer
#19 lore raw

PATCH 15/22 btrfs: kill trivial wrapper tree_mod_log_eb_move

In reply to David Sterba

The wrapper is effectively an alias for tree_mod_log_insert_move but also hides the missing error handling. To make that more visible, lift the BUG_ON to the callers.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)
fs/btrfs/ctree.c +12 -15
index 7d39ef6ce20a..45c17433bb76 100644
@@ -837,14 +837,6 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 	return ret;
 }
 
-static inline void tree_mod_log_eb_move(struct extent_buffer *dst,
-		     int dst_offset, int src_offset, int nr_items)
-{
-	int ret;
-	ret = tree_mod_log_insert_move(dst, dst_offset, src_offset, nr_items);
-	BUG_ON(ret < 0);
-}
-
 static noinline void tree_mod_log_set_node_key(struct extent_buffer *eb,
 		int slot, int atomic)
 {

@@ -3225,8 +3217,8 @@ static int push_node_left(struct btrfs_trans_handle *trans,
 
 	if (push_items < src_nritems) {
 		/*
-		 * don't call tree_mod_log_eb_move here, key removal was already
-		 * fully logged by tree_mod_log_eb_copy above.
+		 * Don't call tree_mod_log_insert_move here, key removal was
+		 * already fully logged by tree_mod_log_eb_copy above.
 		 */
 		memmove_extent_buffer(src, btrfs_node_key_ptr_offset(0),
 				      btrfs_node_key_ptr_offset(push_items),

@@ -3281,7 +3273,8 @@ static int balance_node_right(struct btrfs_trans_handle *trans,
 	if (max_push < push_items)
 		push_items = max_push;
 
-	tree_mod_log_eb_move(dst, push_items, 0, dst_nritems);
+	ret = tree_mod_log_insert_move(dst, push_items, 0, dst_nritems);
+	BUG_ON(ret < 0);
 	memmove_extent_buffer(dst, btrfs_node_key_ptr_offset(push_items),
 				      btrfs_node_key_ptr_offset(0),
 				      (dst_nritems) *

@@ -3399,9 +3392,11 @@ static void insert_ptr(struct btrfs_trans_handle *trans,
 	BUG_ON(slot > nritems);
 	BUG_ON(nritems == BTRFS_NODEPTRS_PER_BLOCK(fs_info));
 	if (slot != nritems) {
-		if (level)
-			tree_mod_log_eb_move(lower, slot + 1, slot,
+		if (level) {
+			ret = tree_mod_log_insert_move(lower, slot + 1, slot,
 					nritems - slot);
+			BUG_ON(ret < 0);
+		}
 		memmove_extent_buffer(lower,
 			      btrfs_node_key_ptr_offset(slot + 1),
 			      btrfs_node_key_ptr_offset(slot),

@@ -4872,9 +4867,11 @@ static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
 
 	nritems = btrfs_header_nritems(parent);
 	if (slot != nritems - 1) {
-		if (level)
-			tree_mod_log_eb_move(parent, slot, slot + 1,
+		if (level) {
+			ret = tree_mod_log_insert_move(parent, slot, slot + 1,
 					nritems - slot - 1);
+			BUG_ON(ret < 0);
+		}
 		memmove_extent_buffer(parent,
 			      btrfs_node_key_ptr_offset(slot),
 			      btrfs_node_key_ptr_offset(slot + 1),
Signature · 1 line
2.16.2
author maintainer
#20 lore raw

PATCH 16/22 btrfs: kill tree_mod_log_set_node_key helper

In reply to David Sterba

A trivial wrapper that can be simply opencoded and makes the GFP allocation request more visible. The error handling is now moved to the callers.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
fs/btrfs/ctree.c +17 -15
index 45c17433bb76..ceb0836d74cd 100644
@@ -837,16 +837,6 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 	return ret;
 }
 
-static noinline void tree_mod_log_set_node_key(struct extent_buffer *eb,
-		int slot, int atomic)
-{
-	int ret;
-
-	ret = tree_mod_log_insert_key(eb, slot, MOD_LOG_KEY_REPLACE,
-					atomic ? GFP_ATOMIC : GFP_NOFS);
-	BUG_ON(ret < 0);
-}
-
 static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 {
 	struct tree_mod_elem **tm_list = NULL;

@@ -1962,7 +1952,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		} else {
 			struct btrfs_disk_key right_key;
 			btrfs_node_key(right, &right_key, 0);
-			tree_mod_log_set_node_key(parent, pslot + 1, 0);
+			ret = tree_mod_log_insert_key(parent, pslot + 1,
+					MOD_LOG_KEY_REPLACE, GFP_NOFS);
+			BUG_ON(ret < 0);
 			btrfs_set_node_key(parent, &right_key, pslot + 1);
 			btrfs_mark_buffer_dirty(parent);
 		}

@@ -2006,7 +1998,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		/* update the parent key to reflect our changes */
 		struct btrfs_disk_key mid_key;
 		btrfs_node_key(mid, &mid_key, 0);
-		tree_mod_log_set_node_key(parent, pslot, 0);
+		ret = tree_mod_log_insert_key(parent, pslot,
+				MOD_LOG_KEY_REPLACE, GFP_NOFS);
+		BUG_ON(ret < 0);
 		btrfs_set_node_key(parent, &mid_key, pslot);
 		btrfs_mark_buffer_dirty(parent);
 	}

@@ -2107,7 +2101,9 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 			struct btrfs_disk_key disk_key;
 			orig_slot += left_nr;
 			btrfs_node_key(mid, &disk_key, 0);
-			tree_mod_log_set_node_key(parent, pslot, 0);
+			ret = tree_mod_log_insert_key(parent, pslot,
+					MOD_LOG_KEY_REPLACE, GFP_NOFS);
+			BUG_ON(ret < 0);
 			btrfs_set_node_key(parent, &disk_key, pslot);
 			btrfs_mark_buffer_dirty(parent);
 			if (btrfs_header_nritems(left) > orig_slot) {

@@ -2161,7 +2157,9 @@ static noinline int push_nodes_for_insert(struct btrfs_trans_handle *trans,
 			struct btrfs_disk_key disk_key;
 
 			btrfs_node_key(right, &disk_key, 0);
-			tree_mod_log_set_node_key(parent, pslot + 1, 0);
+			ret = tree_mod_log_insert_key(parent, pslot + 1,
+					MOD_LOG_KEY_REPLACE, GFP_NOFS);
+			BUG_ON(ret < 0);
 			btrfs_set_node_key(parent, &disk_key, pslot + 1);
 			btrfs_mark_buffer_dirty(parent);
 

@@ -3114,13 +3112,17 @@ static void fixup_low_keys(struct btrfs_fs_info *fs_info,
 {
 	int i;
 	struct extent_buffer *t;
+	int ret;
 
 	for (i = level; i < BTRFS_MAX_LEVEL; i++) {
 		int tslot = path->slots[i];
+
 		if (!path->nodes[i])
 			break;
 		t = path->nodes[i];
-		tree_mod_log_set_node_key(t, tslot, 1);
+		ret = tree_mod_log_insert_key(t, tslot, MOD_LOG_KEY_REPLACE,
+				GFP_ATOMIC);
+		BUG_ON(ret < 0);
 		btrfs_set_node_key(t, key, tslot);
 		btrfs_mark_buffer_dirty(path->nodes[i]);
 		if (tslot != 0)
Signature · 1 line
2.16.2
author maintainer
#21 lore raw

PATCH 17/22 btrfs: kill tree_mod_log_set_root_pointer helper

In reply to David Sterba

A useless wrapper around tree_mod_log_insert_root that hides missing error handling. Move it to the callers.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
fs/btrfs/ctree.c +7 -13
index ceb0836d74cd..30950439731e 100644
@@ -883,16 +883,6 @@ static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 	return ret;
 }
 
-static noinline void
-tree_mod_log_set_root_pointer(struct btrfs_root *root,
-			      struct extent_buffer *new_root_node,
-			      int log_removal)
-{
-	int ret;
-	ret = tree_mod_log_insert_root(root->node, new_root_node, log_removal);
-	BUG_ON(ret < 0);
-}
-
 /*
  * check if the tree block can be shared by multiple trees
  */

@@ -1119,7 +1109,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
 			parent_start = buf->start;
 
 		extent_buffer_get(cow);
-		tree_mod_log_set_root_pointer(root, cow, 1);
+		ret = tree_mod_log_insert_root(root->node, cow, 1);
+		BUG_ON(ret < 0);
 		rcu_assign_pointer(root->node, cow);
 
 		btrfs_free_tree_block(trans, root, buf, parent_start,

@@ -1873,7 +1864,8 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 			goto enospc;
 		}
 
-		tree_mod_log_set_root_pointer(root, child, 1);
+		ret = tree_mod_log_insert_root(root->node, child, 1);
+		BUG_ON(ret < 0);
 		rcu_assign_pointer(root->node, child);
 
 		add_root_to_dirty_list(root);

@@ -3319,6 +3311,7 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 	struct extent_buffer *c;
 	struct extent_buffer *old;
 	struct btrfs_disk_key lower_key;
+	int ret;
 
 	BUG_ON(path->nodes[level]);
 	BUG_ON(path->nodes[level-1] != root->node);

@@ -3357,7 +3350,8 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 	btrfs_mark_buffer_dirty(c);
 
 	old = root->node;
-	tree_mod_log_set_root_pointer(root, c, 0);
+	ret = tree_mod_log_insert_root(root->node, c, 0);
+	BUG_ON(ret < 0);
 	rcu_assign_pointer(root->node, c);
 
 	/* the super has an extra ref to root->node */
Signature · 1 line
2.16.2
Reviewed-by on v1 17/22
#22 lore raw

Re: [PATCH 17/22] btrfs: kill tree_mod_log_set_root_pointer helper

Quoted 4 lines · David Sterba

A useless wrapper around tree_mod_log_insert_root that hides missing error handling. Move it to the callers.

Signed-off-by David Sterba <[email protected]>

For 15-17:

Reviewed-by Nikolay Borisov <[email protected]>
Quoted 64 lines
---
 fs/btrfs/ctree.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)
fs/btrfs/ctree.c +7 -13
index ceb0836d74cd..30950439731e 100644
@@ -883,16 +883,6 @@ static noinline int tree_mod_log_free_eb(struct extent_buffer *eb)
 	return ret;
 }
 
-static noinline void
-tree_mod_log_set_root_pointer(struct btrfs_root *root,
-			      struct extent_buffer *new_root_node,
-			      int log_removal)
-{
-	int ret;
-	ret = tree_mod_log_insert_root(root->node, new_root_node, log_removal);
-	BUG_ON(ret < 0);
-}
-
 /*
  * check if the tree block can be shared by multiple trees
  */

@@ -1119,7 +1109,8 @@ static noinline int __btrfs_cow_block(struct btrfs_trans_handle *trans,
 			parent_start = buf->start;
 
 		extent_buffer_get(cow);
-		tree_mod_log_set_root_pointer(root, cow, 1);
+		ret = tree_mod_log_insert_root(root->node, cow, 1);
+		BUG_ON(ret < 0);
 		rcu_assign_pointer(root->node, cow);
 
 		btrfs_free_tree_block(trans, root, buf, parent_start,

@@ -1873,7 +1864,8 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 			goto enospc;
 		}
 
-		tree_mod_log_set_root_pointer(root, child, 1);
+		ret = tree_mod_log_insert_root(root->node, child, 1);
+		BUG_ON(ret < 0);
 		rcu_assign_pointer(root->node, child);
 
 		add_root_to_dirty_list(root);

@@ -3319,6 +3311,7 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 	struct extent_buffer *c;
 	struct extent_buffer *old;
 	struct btrfs_disk_key lower_key;
+	int ret;
 
 	BUG_ON(path->nodes[level]);
 	BUG_ON(path->nodes[level-1] != root->node);

@@ -3357,7 +3350,8 @@ static noinline int insert_new_root(struct btrfs_trans_handle *trans,
 	btrfs_mark_buffer_dirty(c);
 
 	old = root->node;
-	tree_mod_log_set_root_pointer(root, c, 0);
+	ret = tree_mod_log_insert_root(root->node, c, 0);
+	BUG_ON(ret < 0);
 	rcu_assign_pointer(root->node, c);
 
 	/* the super has an extra ref to root->node */
author maintainer
#23 lore raw

PATCH 18/22 btrfs: move allocation after simple tests in tree_mod_log_insert_key

In reply to David Sterba

The allocation of tree_mod_elem can be delayed after tree_mod_dont_log. In case it causes the function to return, the allocation would be redundant. Otherwise it could cause unnecessary failure if there's not enough memory.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
fs/btrfs/ctree.c +3 -5
index 30950439731e..da6e2c3ca2d0 100644
@@ -521,15 +521,13 @@ static noinline int tree_mod_log_insert_key(struct extent_buffer *eb, int slot,
 	if (!tree_mod_need_log(eb->fs_info, eb))
 		return 0;
 
+	if (tree_mod_dont_log(eb->fs_info, eb))
+		return 0;
+
 	tm = alloc_tree_mod_elem(eb, slot, op, flags);
 	if (!tm)
 		return -ENOMEM;
 
-	if (tree_mod_dont_log(eb->fs_info, eb)) {
-		kfree(tm);
-		return 0;
-	}
-
 	ret = __tree_mod_log_insert(eb->fs_info, tm);
 	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
Signature · 1 line
2.16.2
#24 lore raw

Re: [PATCH 18/22] btrfs: move allocation after simple tests in tree_mod_log_insert_key

Quoted 4 lines · David Sterba

The allocation of tree_mod_elem can be delayed after tree_mod_dont_log. In case it causes the function to return, the allocation would be redundant. Otherwise it could cause unnecessary failure if there's not enough memory.

Nop, the allocation must be made before calling tree_mod_dont_log(), as that function acquires a write lock.
That's why there's a call to tree_mod_need_log() before the allocation and a call to tree_mod_dont_log() after (they do the same checks).

Quoted 36 lines
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/ctree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
fs/btrfs/ctree.c +3 -6
index 30950439731e..da6e2c3ca2d0 100644
@@ -521,15 +521,13 @@ static noinline int tree_mod_log_insert_key(struct extent_buffer *eb, int slot,
        if (!tree_mod_need_log(eb->fs_info, eb))
                return 0;
 
+       if (tree_mod_dont_log(eb->fs_info, eb))
+               return 0;
+
        tm = alloc_tree_mod_elem(eb, slot, op, flags);
        if (!tm)
                return -ENOMEM;
 
-       if (tree_mod_dont_log(eb->fs_info, eb)) {
-               kfree(tm);
-               return 0;
-       }
-
        ret = __tree_mod_log_insert(eb->fs_info, tm);
        write_unlock(&eb->fs_info->tree_mod_log_lock);
        if (ret)
--

2.16.2

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”
#25 lore raw

Re: [PATCH 18/22] btrfs: move allocation after simple tests in tree_mod_log_insert_key

Quoted 10 lines · Filipe Manana
Quoted 4 lines · David Sterba

The allocation of tree_mod_elem can be delayed after tree_mod_dont_log. In case it causes the function to return, the allocation would be redundant. Otherwise it could cause unnecessary failure if there's not enough memory.

Nop, the allocation must be made before calling tree_mod_dont_log(), as that function acquires a write lock.
That's why there's a call to tree_mod_need_log() before the allocation and a call to tree_mod_dont_log() after (they do the same checks).

Ah right, thanks, it's even documented, dunno why I missed it.

author maintainer
#26 lore raw

PATCH 19/22 btrfs: separate types for submit_bio_start and submit_bio_done

In reply to David Sterba

The callbacks make use of different parameters that are passed to the other type unnecessarily. This patch adds separate types for each and the unused parameters will be removed.

The type extent_submit_bio_hook_t keeps all parameters and can be used where the start/done types are not appropriate.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   | 8 ++++----
 fs/btrfs/disk-io.h   | 4 ++--
 fs/btrfs/extent_io.h | 9 +++++++++
 3 files changed, 15 insertions(+), 6 deletions(-)
fs/btrfs/disk-io.c +4 -4
index 21f34ad0d411..6ffc97305d6a 100644
@@ -124,8 +124,8 @@ struct async_submit_bio {
 	void *private_data;
 	struct btrfs_fs_info *fs_info;
 	struct bio *bio;
-	extent_submit_bio_hook_t *submit_bio_start;
-	extent_submit_bio_hook_t *submit_bio_done;
+	extent_submit_bio_start_t *submit_bio_start;
+	extent_submit_bio_done_t *submit_bio_done;
 	int mirror_num;
 	unsigned long bio_flags;
 	/*

@@ -759,8 +759,8 @@ static void run_one_async_free(struct btrfs_work *work)
 blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 				 int mirror_num, unsigned long bio_flags,
 				 u64 bio_offset, void *private_data,
-				 extent_submit_bio_hook_t *submit_bio_start,
-				 extent_submit_bio_hook_t *submit_bio_done)
+				 extent_submit_bio_start_t *submit_bio_start,
+				 extent_submit_bio_done_t *submit_bio_done)
 {
 	struct async_submit_bio *async;
fs/btrfs/disk-io.h +2 -2
index 301151a50ac1..dcc94215de4f 100644
@@ -131,8 +131,8 @@ blk_status_t btrfs_bio_wq_end_io(struct btrfs_fs_info *info, struct bio *bio,
 blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 			int mirror_num, unsigned long bio_flags,
 			u64 bio_offset, void *private_data,
-			extent_submit_bio_hook_t *submit_bio_start,
-			extent_submit_bio_hook_t *submit_bio_done);
+			extent_submit_bio_start_t *submit_bio_start,
+			extent_submit_bio_done_t *submit_bio_done);
 unsigned long btrfs_async_submit_limit(struct btrfs_fs_info *info);
 int btrfs_write_tree_block(struct extent_buffer *buf);
 void btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
fs/btrfs/extent_io.h +9 -0
index a7a850abd600..202546435db6 100644
@@ -95,6 +95,15 @@ struct io_failure_record;
 typedef	blk_status_t (extent_submit_bio_hook_t)(void *private_data, struct bio *bio,
 				       int mirror_num, unsigned long bio_flags,
 				       u64 bio_offset);
+
+typedef blk_status_t (extent_submit_bio_start_t)(void *private_data,
+		struct bio *bio, int mirror_num, unsigned long bio_flags,
+		u64 bio_offset);
+
+typedef blk_status_t (extent_submit_bio_done_t)(void *private_data,
+		struct bio *bio, int mirror_num, unsigned long bio_flags,
+		u64 bio_offset);
+
 struct extent_io_ops {
 	/*
 	 * The following callbacks must be allways defined, the function
Signature · 1 line
2.16.2
author maintainer
#27 lore raw

PATCH 20/22 btrfs: remove unused parameters from extent_submit_bio_start_t

In reply to David Sterba

Remove parameters not used by any of the callbacks.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   | 2 --
 fs/btrfs/extent_io.h | 3 +--
 fs/btrfs/inode.c     | 4 +---
 3 files changed, 2 insertions(+), 7 deletions(-)
fs/btrfs/disk-io.c +0 -2
index 6ffc97305d6a..e396ed366751 100644
@@ -725,7 +725,6 @@ static void run_one_async_start(struct btrfs_work *work)
 
 	async = container_of(work, struct  async_submit_bio, work);
 	ret = async->submit_bio_start(async->private_data, async->bio,
-				      async->mirror_num, async->bio_flags,
 				      async->bio_offset);
 	if (ret)
 		async->status = ret;

@@ -808,7 +807,6 @@ static blk_status_t btree_csum_one_bio(struct bio *bio)
 }
 
 static blk_status_t __btree_submit_bio_start(void *private_data, struct bio *bio,
-					     int mirror_num, unsigned long bio_flags,
 					     u64 bio_offset)
 {
 	/*
fs/btrfs/extent_io.h +1 -2
index 202546435db6..1d3792568e64 100644
@@ -97,8 +97,7 @@ typedef	blk_status_t (extent_submit_bio_hook_t)(void *private_data, struct bio *
 				       u64 bio_offset);
 
 typedef blk_status_t (extent_submit_bio_start_t)(void *private_data,
-		struct bio *bio, int mirror_num, unsigned long bio_flags,
-		u64 bio_offset);
+		struct bio *bio, u64 bio_offset);
 
 typedef blk_status_t (extent_submit_bio_done_t)(void *private_data,
 		struct bio *bio, int mirror_num, unsigned long bio_flags,
fs/btrfs/inode.c +1 -3
index f53470112670..9b33544caaf8 100644
@@ -1922,7 +1922,6 @@ int btrfs_merge_bio_hook(struct page *page, unsigned long offset,
  * are inserted into the btree
  */
 static blk_status_t __btrfs_submit_bio_start(void *private_data, struct bio *bio,
-				    int mirror_num, unsigned long bio_flags,
 				    u64 bio_offset)
 {
 	struct inode *inode = private_data;

@@ -8271,8 +8270,7 @@ static void btrfs_endio_direct_write(struct bio *bio)
 }
 
 static blk_status_t __btrfs_submit_bio_start_direct_io(void *private_data,
-				    struct bio *bio, int mirror_num,
-				    unsigned long bio_flags, u64 offset)
+				    struct bio *bio, u64 offset)
 {
 	struct inode *inode = private_data;
 	blk_status_t ret;
Signature · 1 line
2.16.2
author maintainer
#28 lore raw

PATCH 21/22 btrfs: remove unused parameters from extent_submit_bio_done_t

In reply to David Sterba

Remove parameters not used by any of the callbacks.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   | 6 ++----
 fs/btrfs/extent_io.h | 3 +--
 fs/btrfs/inode.c     | 3 +--
 3 files changed, 4 insertions(+), 8 deletions(-)
fs/btrfs/disk-io.c +2 -4
index e396ed366751..f24522a6f36f 100644
@@ -743,8 +743,7 @@ static void run_one_async_done(struct btrfs_work *work)
 		return;
 	}
 
-	async->submit_bio_done(async->private_data, async->bio, async->mirror_num,
-			       async->bio_flags, async->bio_offset);
+	async->submit_bio_done(async->private_data, async->bio, async->mirror_num);
 }
 
 static void run_one_async_free(struct btrfs_work *work)

@@ -817,8 +816,7 @@ static blk_status_t __btree_submit_bio_start(void *private_data, struct bio *bio
 }
 
 static blk_status_t __btree_submit_bio_done(void *private_data, struct bio *bio,
-					    int mirror_num, unsigned long bio_flags,
-					    u64 bio_offset)
+					    int mirror_num)
 {
 	struct inode *inode = private_data;
 	blk_status_t ret;
fs/btrfs/extent_io.h +1 -2
index 1d3792568e64..d69c106077fc 100644
@@ -100,8 +100,7 @@ typedef blk_status_t (extent_submit_bio_start_t)(void *private_data,
 		struct bio *bio, u64 bio_offset);
 
 typedef blk_status_t (extent_submit_bio_done_t)(void *private_data,
-		struct bio *bio, int mirror_num, unsigned long bio_flags,
-		u64 bio_offset);
+		struct bio *bio, int mirror_num);
 
 struct extent_io_ops {
 	/*
fs/btrfs/inode.c +1 -2
index 9b33544caaf8..5cfba8be73be 100644
@@ -1941,8 +1941,7 @@ static blk_status_t __btrfs_submit_bio_start(void *private_data, struct bio *bio
  * are inserted into the btree
  */
 static blk_status_t __btrfs_submit_bio_done(void *private_data, struct bio *bio,
-			  int mirror_num, unsigned long bio_flags,
-			  u64 bio_offset)
+			  int mirror_num)
 {
 	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
Signature · 1 line
2.16.2
author maintainer
#29 lore raw

PATCH 22/22 btrfs: rename submit callbacks and drop double underscores

In reply to David Sterba
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c |  8 ++++----
 fs/btrfs/inode.c   | 23 +++++++++++------------
 2 files changed, 15 insertions(+), 16 deletions(-)
fs/btrfs/disk-io.c +4 -4
index f24522a6f36f..e112708e3e9b 100644
@@ -805,7 +805,7 @@ static blk_status_t btree_csum_one_bio(struct bio *bio)
 	return errno_to_blk_status(ret);
 }
 
-static blk_status_t __btree_submit_bio_start(void *private_data, struct bio *bio,
+static blk_status_t btree_submit_bio_start(void *private_data, struct bio *bio,
 					     u64 bio_offset)
 {
 	/*

@@ -815,7 +815,7 @@ static blk_status_t __btree_submit_bio_start(void *private_data, struct bio *bio
 	return btree_csum_one_bio(bio);
 }
 
-static blk_status_t __btree_submit_bio_done(void *private_data, struct bio *bio,
+static blk_status_t btree_submit_bio_done(void *private_data, struct bio *bio,
 					    int mirror_num)
 {
 	struct inode *inode = private_data;

@@ -875,8 +875,8 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 		 */
 		ret = btrfs_wq_submit_bio(fs_info, bio, mirror_num, 0,
 					  bio_offset, private_data,
-					  __btree_submit_bio_start,
-					  __btree_submit_bio_done);
+					  btree_submit_bio_start,
+					  btree_submit_bio_done);
 	}
 
 	if (ret)
fs/btrfs/inode.c +11 -12
index 5cfba8be73be..16c6e2fe3b58 100644
@@ -1921,7 +1921,7 @@ int btrfs_merge_bio_hook(struct page *page, unsigned long offset,
  * At IO completion time the cums attached on the ordered extent record
  * are inserted into the btree
  */
-static blk_status_t __btrfs_submit_bio_start(void *private_data, struct bio *bio,
+static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,
 				    u64 bio_offset)
 {
 	struct inode *inode = private_data;

@@ -1940,7 +1940,7 @@ static blk_status_t __btrfs_submit_bio_start(void *private_data, struct bio *bio
  * At IO completion time the cums attached on the ordered extent record
  * are inserted into the btree
  */
-static blk_status_t __btrfs_submit_bio_done(void *private_data, struct bio *bio,
+static blk_status_t btrfs_submit_bio_done(void *private_data, struct bio *bio,
 			  int mirror_num)
 {
 	struct inode *inode = private_data;

@@ -2013,8 +2013,8 @@ static blk_status_t btrfs_submit_bio_hook(void *private_data, struct bio *bio,
 		/* we're doing a write, do the async checksumming */
 		ret = btrfs_wq_submit_bio(fs_info, bio, mirror_num, bio_flags,
 					  bio_offset, inode,
-					  __btrfs_submit_bio_start,
-					  __btrfs_submit_bio_done);
+					  btrfs_submit_bio_start,
+					  btrfs_submit_bio_done);
 		goto out;
 	} else if (!skip_sum) {
 		ret = btrfs_csum_one_bio(inode, bio, 0, 0);

@@ -8268,7 +8268,7 @@ static void btrfs_endio_direct_write(struct bio *bio)
 	bio_put(bio);
 }
 
-static blk_status_t __btrfs_submit_bio_start_direct_io(void *private_data,
+static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data,
 				    struct bio *bio, u64 offset)
 {
 	struct inode *inode = private_data;

@@ -8349,9 +8349,8 @@ static inline blk_status_t btrfs_lookup_and_bind_dio_csum(struct inode *inode,
 	return 0;
 }
 
-static inline blk_status_t
-__btrfs_submit_dio_bio(struct bio *bio, struct inode *inode, u64 file_offset,
-		       int async_submit)
+static inline blk_status_t btrfs_submit_dio_bio(struct bio *bio,
+		struct inode *inode, u64 file_offset, int async_submit)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_dio_private *dip = bio->bi_private;

@@ -8374,8 +8373,8 @@ __btrfs_submit_dio_bio(struct bio *bio, struct inode *inode, u64 file_offset,
 	if (write && async_submit) {
 		ret = btrfs_wq_submit_bio(fs_info, bio, 0, 0,
 					  file_offset, inode,
-					  __btrfs_submit_bio_start_direct_io,
-					  __btrfs_submit_bio_done);
+					  btrfs_submit_bio_start_direct_io,
+					  btrfs_submit_bio_done);
 		goto err;
 	} else if (write) {
 		/*

@@ -8461,7 +8460,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip)
 		 */
 		atomic_inc(&dip->pending_bios);
 
-		status = __btrfs_submit_dio_bio(bio, inode, file_offset,
+		status = btrfs_submit_dio_bio(bio, inode, file_offset,
 						async_submit);
 		if (status) {
 			bio_put(bio);

@@ -8481,7 +8480,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip)
 	} while (submit_len > 0);
 
 submit:
-	status = __btrfs_submit_dio_bio(bio, inode, file_offset, async_submit);
+	status = btrfs_submit_dio_bio(bio, inode, file_offset, async_submit);
 	if (!status)
 		return 0;
Signature · 1 line
2.16.2