[PATCH 00/22] Misc cleanups

[PATCH 00/22] Misc cleanups

[David Sterba]

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(-)

-- 
2.16.2

[PATCH 01/22] btrfs: assume that bio_ret is always valid in submit_extent_page

[David Sterba]

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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index dfeb74a0be77..d00d5a59ff21 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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;
 }
-- 
2.16.2

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

[David Sterba]

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

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index d00d5a59ff21..36514baa661e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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;
-- 
2.16.2

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

[Anand Jain]

On 03/08/2018 10:33 PM, David Sterba wrote:
> All callers pass a valid pointer, we can remove the redundant checks.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/extent_io.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index d00d5a59ff21..36514baa661e 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -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);
> +


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



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

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

[David Sterba]

On Tue, Mar 13, 2018 at 11:03:31PM +0800, Anand Jain wrote:
> On 03/08/2018 10:33 PM, David Sterba wrote:
> > All callers pass a valid pointer, we can remove the redundant checks.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/extent_io.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index d00d5a59ff21..36514baa661e 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -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);
> > +
> 
> 
> mount is failing with this patch as in this stack below the 
> prev_em_start is NULL.

Right, I've removed the patch.

[PATCH 03/22] btrfs: remove redundant variable in __do_readpage

[David Sterba]

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

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 36514baa661e..ffcbdb2390a1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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);
-- 
2.16.2

[PATCH 04/22] btrfs: cleanup merging conditions in submit_extent_page

[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 <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ffcbdb2390a1..2fbc58b0b908 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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) {
-- 
2.16.2

[PATCH 05/22] btrfs: document more parameters of submit_extent_page

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2fbc58b0b908..f1d199c8883e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -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,
-- 
2.16.2

[PATCH 06/22] btrfs: drop fs_info parameter from tree_mod_log_set_node_key

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b88a79e69ddf..11e985ca0bc2 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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)
-- 
2.16.2

[PATCH 07/22] btrfs: drop fs_info parameter from tree_mod_log_insert_move

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 11e985ca0bc2..3b707af76579 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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);
 }
 
-- 
2.16.2

[PATCH 08/22] btrfs: drop fs_info parameter from tree_mod_log_insert_key

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3b707af76579..60e27bef0e27 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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);
 	}
 
-- 
2.16.2

[PATCH 09/22] btrfs: drop fs_info parameter from tree_mod_log_free_eb

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 60e27bef0e27..6d2f8562bb36 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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;
-- 
2.16.2

[PATCH 10/22] btrfs: drop fs_info parameter from tree_mod_log_free_eb

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 6d2f8562bb36..632beadf5725 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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);
 }
 
-- 
2.16.2

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

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 632beadf5725..5585433eac40 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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),
-- 
2.16.2

[PATCH 12/22] btrfs: embed tree_mod_move structure to tree_mod_elem

[David Sterba]

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

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5585433eac40..e53af58697db 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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;
-- 
2.16.2

[PATCH 13/22] btrfs: drop fs_info parameter from __tree_mod_log_oldest_root

[David Sterba]

It's provided by the extent_buffer.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index e53af58697db..716c140798c3 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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 {
-- 
2.16.2

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

[David Sterba]

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

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 58 +++++++++++++++++++-------------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 716c140798c3..7d39ef6ce20a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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);
 }
 
-- 
2.16.2

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

[Nikolay Borisov]

On  8.03.2018 16:33, David Sterba wrote:
> The wrappers are trivial and do bring any extra value on top of the

nit: s/do/don't/ ?

> plain locking primitives.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Just 2 minor nits, otherwise LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/ctree.c | 58 +++++++++++++++++++-------------------------------------
>  1 file changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 716c140798c3..7d39ef6ce20a 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -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.
nit: s/;;/::/
>   */
>  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);
>  }
>  
> 

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

[David Sterba]

On Thu, Mar 08, 2018 at 05:37:33PM +0200, Nikolay Borisov wrote:
> 
> 
> On  8.03.2018 16:33, David Sterba wrote:
> > The wrappers are trivial and do bring any extra value on top of the
> 
> nit: s/do/don't/ ?

Right, fixed.

> > plain locking primitives.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> 
> Just 2 minor nits, otherwise LGTM:
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> > - * 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.

[PATCH 15/22] btrfs: kill trivial wrapper tree_mod_log_eb_move

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 7d39ef6ce20a..45c17433bb76 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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),
-- 
2.16.2

[PATCH 16/22] btrfs: kill tree_mod_log_set_node_key helper

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 45c17433bb76..ceb0836d74cd 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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)
-- 
2.16.2

[PATCH 17/22] btrfs: kill tree_mod_log_set_root_pointer helper

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index ceb0836d74cd..30950439731e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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 */
-- 
2.16.2

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

[Nikolay Borisov]

On  8.03.2018 16:33, David Sterba wrote:
> A useless wrapper around tree_mod_log_insert_root that hides missing
> error handling. Move it to the callers.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

For 15-17:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/ctree.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index ceb0836d74cd..30950439731e 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -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 */
> 

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

[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 <dsterba@suse.com>
---
 fs/btrfs/ctree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 30950439731e..da6e2c3ca2d0 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -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

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

[Filipe Manana]

On Thu, Mar 8, 2018 at 2:33 PM, David Sterba <dsterba@suse.com> wrote:
> 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).


>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 30950439731e..da6e2c3ca2d0 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

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

[David Sterba]

On Thu, Mar 08, 2018 at 03:26:11PM +0000, Filipe Manana wrote:
> On Thu, Mar 8, 2018 at 2:33 PM, David Sterba <dsterba@suse.com> wrote:
> > 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.

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

[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 <dsterba@suse.com>
---
 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(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 21f34ad0d411..6ffc97305d6a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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;
 
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 301151a50ac1..dcc94215de4f 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -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);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index a7a850abd600..202546435db6 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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
-- 
2.16.2

[PATCH 20/22] btrfs: remove unused parameters from extent_submit_bio_start_t

[David Sterba]

Remove parameters not used by any of the callbacks.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c   | 2 --
 fs/btrfs/extent_io.h | 3 +--
 fs/btrfs/inode.c     | 4 +---
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 6ffc97305d6a..e396ed366751 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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)
 {
 	/*
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 202546435db6..1d3792568e64 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index f53470112670..9b33544caaf8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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;
-- 
2.16.2

[PATCH 21/22] btrfs: remove unused parameters from extent_submit_bio_done_t

[David Sterba]

Remove parameters not used by any of the callbacks.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c   | 6 ++----
 fs/btrfs/extent_io.h | 3 +--
 fs/btrfs/inode.c     | 3 +--
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e396ed366751..f24522a6f36f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 1d3792568e64..d69c106077fc 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -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 {
 	/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 9b33544caaf8..5cfba8be73be 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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);
-- 
2.16.2

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

[David Sterba]

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c |  8 ++++----
 fs/btrfs/inode.c   | 23 +++++++++++------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f24522a6f36f..e112708e3e9b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 5cfba8be73be..16c6e2fe3b58 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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;
 
-- 
2.16.2