[PATCH 0/7] Structure and callback cleanups

[PATCH 0/7] Structure and callback cleanups

[David Sterba]

A handful of removed structure members that are not used,
async_submit_bio is down by 16 bytes and async_cow by 8.

Some of the extent_io_ops callbacks are unnecessarily called indirectly.
The rest of extent_io_ops is going to be transformed in following
series.

David Sterba (7):
  btrfs: remove unused member async_submit_bio::fs_info
  btrfs: remove unused member async_submit_bio::bio_flags
  btrfs: remove redundant member async_cow::root
  btrfs: unify end_io callbacks of async_submit_bio
  btrfs: drop extent_io_ops::tree_fs_info callback
  btrfs: drop extent_io_ops::merge_bio_hook callback
  btrfs: drop extent_io_ops::set_range_writeback callback

 fs/btrfs/compression.c | 10 +++-------
 fs/btrfs/ctree.h       |  2 +-
 fs/btrfs/disk-io.c     | 42 +++---------------------------------------
 fs/btrfs/disk-io.h     |  5 +++--
 fs/btrfs/extent_io.c   | 28 +++++++---------------------
 fs/btrfs/extent_io.h   |  8 --------
 fs/btrfs/inode.c       | 32 ++++++++------------------------
 7 files changed, 25 insertions(+), 102 deletions(-)

-- 
2.18.0

[PATCH 1/7] btrfs: remove unused member async_submit_bio::fs_info

[David Sterba]

Introduced by c6100a4b4e3d1 ("Btrfs: replace tree->mapping with
tree->private_data") to be used in run_one_async_done where it got
unused after 736cd52e0c720103 ("Btrfs: remove nr_async_submits and
async_submit_draining").

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e3858b2fe014..edc2daef423b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -106,7 +106,6 @@ void __cold btrfs_end_io_wq_exit(void)
  */
 struct async_submit_bio {
 	void *private_data;
-	struct btrfs_fs_info *fs_info;
 	struct bio *bio;
 	extent_submit_bio_start_t *submit_bio_start;
 	extent_submit_bio_done_t *submit_bio_done;
@@ -801,7 +800,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 		return BLK_STS_RESOURCE;
 
 	async->private_data = private_data;
-	async->fs_info = fs_info;
 	async->bio = bio;
 	async->mirror_num = mirror_num;
 	async->submit_bio_start = submit_bio_start;
-- 
2.18.0

[PATCH 2/7] btrfs: remove unused member async_submit_bio::bio_flags

[David Sterba]

After splitting the start and end hooks in a758781d4b76c3 ("btrfs:
separate types for submit_bio_start and submit_bio_done"), some of
the function arguments were dropped but not removed from the structure.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/disk-io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index edc2daef423b..2e7932c753b2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -110,7 +110,6 @@ struct async_submit_bio {
 	extent_submit_bio_start_t *submit_bio_start;
 	extent_submit_bio_done_t *submit_bio_done;
 	int mirror_num;
-	unsigned long bio_flags;
 	/*
 	 * bio_offset is optional, can be used if the pages in the bio
 	 * can't tell us where in the file the bio should go
@@ -808,7 +807,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 	btrfs_init_work(&async->work, btrfs_worker_helper, run_one_async_start,
 			run_one_async_done, run_one_async_free);
 
-	async->bio_flags = bio_flags;
 	async->bio_offset = bio_offset;
 
 	async->status = 0;
-- 
2.18.0

[PATCH 3/7] btrfs: remove redundant member async_cow::root

[David Sterba]

The root is only used to get fs_info out of it, but the same can be
retrieved from the inode that's in async_cow.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 564ec00c765b..36b323b27dbf 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -358,7 +358,6 @@ struct async_extent {
 
 struct async_cow {
 	struct inode *inode;
-	struct btrfs_root *root;
 	struct page *locked_page;
 	u64 start;
 	u64 end;
@@ -1144,13 +1143,11 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 {
 	struct btrfs_fs_info *fs_info;
 	struct async_cow *async_cow;
-	struct btrfs_root *root;
 	unsigned long nr_pages;
 
 	async_cow = container_of(work, struct async_cow, work);
 
-	root = async_cow->root;
-	fs_info = root->fs_info;
+	fs_info = BTRFS_I(async_cow->inode)->root->fs_info;
 	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
 		PAGE_SHIFT;
 
@@ -1179,7 +1176,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct async_cow *async_cow;
-	struct btrfs_root *root = BTRFS_I(inode)->root;
 	unsigned long nr_pages;
 	u64 cur_end;
 
@@ -1189,7 +1185,6 @@ static int cow_file_range_async(struct inode *inode, struct page *locked_page,
 		async_cow = kmalloc(sizeof(*async_cow), GFP_NOFS);
 		BUG_ON(!async_cow); /* -ENOMEM */
 		async_cow->inode = igrab(inode);
-		async_cow->root = root;
 		async_cow->locked_page = locked_page;
 		async_cow->start = start;
 		async_cow->write_flags = write_flags;
-- 
2.18.0

Re: [PATCH 3/7] btrfs: remove redundant member async_cow::root

[David Sterba]

On Thu, Jul 19, 2018 at 01:05:14PM +0200, David Sterba wrote:
> The root is only used to get fs_info out of it, but the same can be
> retrieved from the inode that's in async_cow.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/inode.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 564ec00c765b..36b323b27dbf 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -358,7 +358,6 @@ struct async_extent {
>  
>  struct async_cow {
>  	struct inode *inode;
> -	struct btrfs_root *root;
>  	struct page *locked_page;
>  	u64 start;
>  	u64 end;
> @@ -1144,13 +1143,11 @@ static noinline void async_cow_submit(struct btrfs_work *work)
>  {
>  	struct btrfs_fs_info *fs_info;
>  	struct async_cow *async_cow;
> -	struct btrfs_root *root;
>  	unsigned long nr_pages;
>  
>  	async_cow = container_of(work, struct async_cow, work);
>  
> -	root = async_cow->root;
> -	fs_info = root->fs_info;
> +	fs_info = BTRFS_I(async_cow->inode)->root->fs_info;

This crashes if the inode is NULL, which can happen in async_cow_start
if compress_file_range returns 0 in num_added. The inode is moved to
delayed iputs and async_cow->inode zeroed.

Patch removed for now. We can reach the inode via the
async_cow::locked_page, but it's 5 pointers to chase and I want to have a
second thought on that if the 8 bytes in the structure is not cheaper.

--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1143,11 +1143,13 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 {
        struct btrfs_fs_info *fs_info;
        struct async_cow *async_cow;
+       struct inode *inode;
        unsigned long nr_pages;
 
        async_cow = container_of(work, struct async_cow, work);
 
-       fs_info = BTRFS_I(async_cow->inode)->root->fs_info;
+       inode = async_cow->locked_page->mapping->host;
+       fs_info = BTRFS_I(inode)->root->fs_info;
        nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
                PAGE_SHIFT;
 
---

[PATCH 4/7] btrfs: unify end_io callbacks of async_submit_bio

[David Sterba]

The end_io callbacks passed to btrfs_wq_submit_bio
(btrfs_submit_bio_done and btree_submit_bio_done) are effectively the
same code, there's no point to do the indirection. Export
btrfs_submit_bio_done and call it directly.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2e7932c753b2..c2d1c66edc49 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -108,7 +108,6 @@ struct async_submit_bio {
 	void *private_data;
 	struct bio *bio;
 	extent_submit_bio_start_t *submit_bio_start;
-	extent_submit_bio_done_t *submit_bio_done;
 	int mirror_num;
 	/*
 	 * bio_offset is optional, can be used if the pages in the bio
@@ -775,7 +774,7 @@ static void run_one_async_done(struct btrfs_work *work)
 		return;
 	}
 
-	async->submit_bio_done(async->private_data, async->bio, async->mirror_num);
+	btrfs_submit_bio_done(async->private_data, async->bio, async->mirror_num);
 }
 
 static void run_one_async_free(struct btrfs_work *work)
@@ -789,8 +788,7 @@ 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_start_t *submit_bio_start,
-				 extent_submit_bio_done_t *submit_bio_done)
+				 extent_submit_bio_start_t *submit_bio_start)
 {
 	struct async_submit_bio *async;
 
@@ -802,7 +800,6 @@ blk_status_t btrfs_wq_submit_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 	async->bio = bio;
 	async->mirror_num = mirror_num;
 	async->submit_bio_start = submit_bio_start;
-	async->submit_bio_done = submit_bio_done;
 
 	btrfs_init_work(&async->work, btrfs_worker_helper, run_one_async_start,
 			run_one_async_done, run_one_async_free);
@@ -845,24 +842,6 @@ 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,
-					    int mirror_num)
-{
-	struct inode *inode = private_data;
-	blk_status_t ret;
-
-	/*
-	 * when we're called for a write, we're already in the async
-	 * submission context.  Just jump into btrfs_map_bio
-	 */
-	ret = btrfs_map_bio(btrfs_sb(inode->i_sb), bio, mirror_num, 1);
-	if (ret) {
-		bio->bi_status = ret;
-		bio_endio(bio);
-	}
-	return ret;
-}
-
 static int check_async_write(struct btrfs_inode *bi)
 {
 	if (atomic_read(&bi->sync_writers))
@@ -905,8 +884,7 @@ 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);
 	}
 
 	if (ret)
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 1a3d277b027b..4cccba22640f 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -120,8 +120,9 @@ 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_start_t *submit_bio_start,
-			extent_submit_bio_done_t *submit_bio_done);
+			extent_submit_bio_start_t *submit_bio_start);
+blk_status_t btrfs_submit_bio_done(void *private_data, struct bio *bio,
+			  int mirror_num);
 int btrfs_write_tree_block(struct extent_buffer *buf);
 void btrfs_wait_tree_block_writeback(struct extent_buffer *buf);
 int btrfs_init_log_root_tree(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 48f1ee9ad379..129fc4ab30ac 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -92,9 +92,6 @@ typedef	blk_status_t (extent_submit_bio_hook_t)(void *private_data, struct bio *
 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);
-
 struct extent_io_ops {
 	/*
 	 * The following callbacks must be allways defined, the function
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 36b323b27dbf..0899ab54cd06 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1952,7 +1952,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,
+blk_status_t btrfs_submit_bio_done(void *private_data, struct bio *bio,
 			  int mirror_num)
 {
 	struct inode *inode = private_data;
@@ -2025,8 +2025,7 @@ 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);
 		goto out;
 	} else if (!skip_sum) {
 		ret = btrfs_csum_one_bio(inode, bio, 0, 0);
@@ -8290,8 +8289,7 @@ static inline blk_status_t btrfs_submit_dio_bio(struct bio *bio,
 	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);
 		goto err;
 	} else if (write) {
 		/*
-- 
2.18.0

[PATCH 5/7] btrfs: drop extent_io_ops::tree_fs_info callback

[David Sterba]

All implementations of the callback are trivial and do the same and
there's only user. Merge everything together.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c2d1c66edc49..d6c04933abbc 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4523,12 +4523,6 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 	return 0;
 }
 
-static struct btrfs_fs_info *btree_fs_info(void *private_data)
-{
-	struct inode *inode = private_data;
-	return btrfs_sb(inode->i_sb);
-}
-
 static const struct extent_io_ops btree_extent_io_ops = {
 	/* mandatory callbacks */
 	.submit_bio_hook = btree_submit_bio_hook,
@@ -4537,7 +4531,6 @@ static const struct extent_io_ops btree_extent_io_ops = {
 	.merge_bio_hook = btrfs_merge_bio_hook,
 	.readpage_io_failed_hook = btree_io_failed_hook,
 	.set_range_writeback = btrfs_set_range_writeback,
-	.tree_fs_info = btree_fs_info,
 
 	/* optional callbacks */
 };
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index fea015be4ce1..f7a138278f16 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -140,14 +140,6 @@ static int add_extent_changeset(struct extent_state *state, unsigned bits,
 
 static void flush_write_bio(struct extent_page_data *epd);
 
-static inline struct btrfs_fs_info *
-tree_fs_info(struct extent_io_tree *tree)
-{
-	if (tree->ops)
-		return tree->ops->tree_fs_info(tree->private_data);
-	return NULL;
-}
-
 int __init extent_io_init(void)
 {
 	extent_state_cache = kmem_cache_create("btrfs_extent_state",
@@ -564,8 +556,10 @@ alloc_extent_state_atomic(struct extent_state *prealloc)
 
 static void extent_io_tree_panic(struct extent_io_tree *tree, int err)
 {
-	btrfs_panic(tree_fs_info(tree), err,
-		    "Locking error: Extent tree was modified by another thread while locked.");
+	struct inode *inode = tree->private_data;
+
+	btrfs_panic(btrfs_sb(inode->i_sb), err,
+	"locking error: extent tree was modified by another thread while locked");
 }
 
 /*
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 129fc4ab30ac..6edbe01fd09d 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -105,7 +105,6 @@ struct extent_io_ops {
 			      size_t size, struct bio *bio,
 			      unsigned long bio_flags);
 	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
-	struct btrfs_fs_info *(*tree_fs_info)(void *private_data);
 	void (*set_range_writeback)(void *private_data, u64 start, u64 end);
 
 	/*
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0899ab54cd06..1614551cf953 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10468,12 +10468,6 @@ static int btrfs_readpage_io_failed_hook(struct page *page, int failed_mirror)
 	return -EAGAIN;
 }
 
-static struct btrfs_fs_info *iotree_fs_info(void *private_data)
-{
-	struct inode *inode = private_data;
-	return btrfs_sb(inode->i_sb);
-}
-
 static void btrfs_check_extent_io_range(void *private_data, const char *caller,
 					u64 start, u64 end)
 {
@@ -10548,7 +10542,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {
 	.readpage_end_io_hook = btrfs_readpage_end_io_hook,
 	.merge_bio_hook = btrfs_merge_bio_hook,
 	.readpage_io_failed_hook = btrfs_readpage_io_failed_hook,
-	.tree_fs_info = iotree_fs_info,
 	.set_range_writeback = btrfs_set_range_writeback,
 
 	/* optional callbacks */
-- 
2.18.0

[PATCH 6/7] btrfs: drop extent_io_ops::merge_bio_hook callback

[David Sterba]

The data and metadata callback implementation both use the same
function. We can remove the call indirection completely.

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

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 70dace47258b..9bfa66592aa7 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
 	struct bio *bio = NULL;
 	struct compressed_bio *cb;
 	unsigned long bytes_left;
-	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
 	int pg_index = 0;
 	struct page *page;
 	u64 first_byte = disk_start;
@@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
 		page = compressed_pages[pg_index];
 		page->mapping = inode->i_mapping;
 		if (bio->bi_iter.bi_size)
-			submit = io_tree->ops->merge_bio_hook(page, 0,
-							   PAGE_SIZE,
-							   bio, 0);
+			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0);
 
 		page->mapping = NULL;
 		if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <
@@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		page->index = em_start >> PAGE_SHIFT;
 
 		if (comp_bio->bi_iter.bi_size)
-			submit = tree->ops->merge_bio_hook(page, 0,
-							PAGE_SIZE,
-							comp_bio, 0);
+			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE,
+					comp_bio, 0);
 
 		page->mapping = NULL;
 		if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index d6c04933abbc..31ab764cf4e3 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {
 	/* mandatory callbacks */
 	.submit_bio_hook = btree_submit_bio_hook,
 	.readpage_end_io_hook = btree_readpage_end_io_hook,
-	/* note we're sharing with inode.c for the merge bio hook */
-	.merge_bio_hook = btrfs_merge_bio_hook,
 	.readpage_io_failed_hook = btree_io_failed_hook,
 	.set_range_writeback = btrfs_set_range_writeback,
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f7a138278f16..55a845621f85 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
 		else
 			contig = bio_end_sector(bio) == sector;
 
-		if (tree->ops && tree->ops->merge_bio_hook(page, offset,
-					page_size, bio, bio_flags))
+		if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size,
+						      bio, bio_flags))
 			can_merge = false;
 
 		if (prev_bio_flags != bio_flags || !contig || !can_merge ||
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 6edbe01fd09d..ec7c4a2982d0 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -101,9 +101,6 @@ struct extent_io_ops {
 	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
 				    struct page *page, u64 start, u64 end,
 				    int mirror);
-	int (*merge_bio_hook)(struct page *page, unsigned long offset,
-			      size_t size, struct bio *bio,
-			      unsigned long bio_flags);
 	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
 	void (*set_range_writeback)(void *private_data, u64 start, u64 end);
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1614551cf953..845621a71468 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1893,8 +1893,8 @@ static void btrfs_clear_bit_hook(void *private_data,
 }
 
 /*
- * extent_io.c merge_bio_hook, this must check the chunk tree to make sure
- * we don't create bios that span stripes or chunks
+ * Merge bio hook, this must check the chunk tree to make sure we don't create
+ * bios that span stripes or chunks
  *
  * return 1 if page cannot be merged to bio
  * return 0 if page can be merged to bio
@@ -10540,7 +10540,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {
 	/* mandatory callbacks */
 	.submit_bio_hook = btrfs_submit_bio_hook,
 	.readpage_end_io_hook = btrfs_readpage_end_io_hook,
-	.merge_bio_hook = btrfs_merge_bio_hook,
 	.readpage_io_failed_hook = btrfs_readpage_io_failed_hook,
 	.set_range_writeback = btrfs_set_range_writeback,
 
-- 
2.18.0

Re: [PATCH 6/7] btrfs: drop extent_io_ops::merge_bio_hook callback

[Nikolay Borisov]

On 19.07.2018 14:05, David Sterba wrote:
> The data and metadata callback implementation both use the same
> function. We can remove the call indirection completely.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/compression.c | 10 +++-------
>  fs/btrfs/disk-io.c     |  2 --
>  fs/btrfs/extent_io.c   |  4 ++--
>  fs/btrfs/extent_io.h   |  3 ---
>  fs/btrfs/inode.c       |  5 ++---
>  5 files changed, 7 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 70dace47258b..9bfa66592aa7 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  	struct bio *bio = NULL;
>  	struct compressed_bio *cb;
>  	unsigned long bytes_left;
> -	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
>  	int pg_index = 0;
>  	struct page *page;
>  	u64 first_byte = disk_start;
> @@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  		page = compressed_pages[pg_index];
>  		page->mapping = inode->i_mapping;
>  		if (bio->bi_iter.bi_size)
> -			submit = io_tree->ops->merge_bio_hook(page, 0,
> -							   PAGE_SIZE,
> -							   bio, 0);
> +			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0);
>  
>  		page->mapping = NULL;
>  		if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <
> @@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  		page->index = em_start >> PAGE_SHIFT;
>  
>  		if (comp_bio->bi_iter.bi_size)
> -			submit = tree->ops->merge_bio_hook(page, 0,
> -							PAGE_SIZE,
> -							comp_bio, 0);
> +			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE,
> +					comp_bio, 0);
>  
>  		page->mapping = NULL;
>  		if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index d6c04933abbc..31ab764cf4e3 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {
>  	/* mandatory callbacks */
>  	.submit_bio_hook = btree_submit_bio_hook,
>  	.readpage_end_io_hook = btree_readpage_end_io_hook,
> -	/* note we're sharing with inode.c for the merge bio hook */
> -	.merge_bio_hook = btrfs_merge_bio_hook,
>  	.readpage_io_failed_hook = btree_io_failed_hook,
>  	.set_range_writeback = btrfs_set_range_writeback,
>  
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f7a138278f16..55a845621f85 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
>  		else
>  			contig = bio_end_sector(bio) == sector;
>  
> -		if (tree->ops && tree->ops->merge_bio_hook(page, offset,
> -					page_size, bio, bio_flags))
> +		if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size,
> +						      bio, bio_flags))

While this is correct I dislike having the if (tree->ops ) check. So
under what conditions do we NOT have the tree->ops set? Looking at the
code it seems we are setting it everytime we create an inode in
btrfs_create. We also set it for symlinks (btrfs_symlink) and a tmp file
(btrfs_tmpfile). Strangely enough in btrfs_read_locked_inode we only set
it for regular files (S_IFREG).

submit_extent_page seems to be called for both metadata but as we no
longer distinguish between eb merge_hook and data merge_hook perhaps the
tree->ops could be killed as well ?

>  			can_merge = false;
>  
>  		if (prev_bio_flags != bio_flags || !contig || !can_merge ||
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 6edbe01fd09d..ec7c4a2982d0 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -101,9 +101,6 @@ struct extent_io_ops {
>  	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  				    struct page *page, u64 start, u64 end,
>  				    int mirror);
> -	int (*merge_bio_hook)(struct page *page, unsigned long offset,
> -			      size_t size, struct bio *bio,
> -			      unsigned long bio_flags);
>  	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
>  	void (*set_range_writeback)(void *private_data, u64 start, u64 end);
>  
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 1614551cf953..845621a71468 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1893,8 +1893,8 @@ static void btrfs_clear_bit_hook(void *private_data,
>  }
>  
>  /*
> - * extent_io.c merge_bio_hook, this must check the chunk tree to make sure
> - * we don't create bios that span stripes or chunks
> + * Merge bio hook, this must check the chunk tree to make sure we don't create
> + * bios that span stripes or chunks
>   *
>   * return 1 if page cannot be merged to bio
>   * return 0 if page can be merged to bio
> @@ -10540,7 +10540,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {
>  	/* mandatory callbacks */
>  	.submit_bio_hook = btrfs_submit_bio_hook,
>  	.readpage_end_io_hook = btrfs_readpage_end_io_hook,
> -	.merge_bio_hook = btrfs_merge_bio_hook,
>  	.readpage_io_failed_hook = btrfs_readpage_io_failed_hook,
>  	.set_range_writeback = btrfs_set_range_writeback,
>  
> 

Re: [PATCH 6/7] btrfs: drop extent_io_ops::merge_bio_hook callback

[David Sterba]

On Thu, Jul 19, 2018 at 03:02:58PM +0300, Nikolay Borisov wrote:
> On 19.07.2018 14:05, David Sterba wrote:
> > The data and metadata callback implementation both use the same
> > function. We can remove the call indirection completely.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/compression.c | 10 +++-------
> >  fs/btrfs/disk-io.c     |  2 --
> >  fs/btrfs/extent_io.c   |  4 ++--
> >  fs/btrfs/extent_io.h   |  3 ---
> >  fs/btrfs/inode.c       |  5 ++---
> >  5 files changed, 7 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> > index 70dace47258b..9bfa66592aa7 100644
> > --- a/fs/btrfs/compression.c
> > +++ b/fs/btrfs/compression.c
> > @@ -299,7 +299,6 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
> >  	struct bio *bio = NULL;
> >  	struct compressed_bio *cb;
> >  	unsigned long bytes_left;
> > -	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> >  	int pg_index = 0;
> >  	struct page *page;
> >  	u64 first_byte = disk_start;
> > @@ -338,9 +337,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
> >  		page = compressed_pages[pg_index];
> >  		page->mapping = inode->i_mapping;
> >  		if (bio->bi_iter.bi_size)
> > -			submit = io_tree->ops->merge_bio_hook(page, 0,
> > -							   PAGE_SIZE,
> > -							   bio, 0);
> > +			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE, bio, 0);
> >  
> >  		page->mapping = NULL;
> >  		if (submit || bio_add_page(bio, page, PAGE_SIZE, 0) <
> > @@ -622,9 +619,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> >  		page->index = em_start >> PAGE_SHIFT;
> >  
> >  		if (comp_bio->bi_iter.bi_size)
> > -			submit = tree->ops->merge_bio_hook(page, 0,
> > -							PAGE_SIZE,
> > -							comp_bio, 0);
> > +			submit = btrfs_merge_bio_hook(page, 0, PAGE_SIZE,
> > +					comp_bio, 0);
> >  
> >  		page->mapping = NULL;
> >  		if (submit || bio_add_page(comp_bio, page, PAGE_SIZE, 0) <
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index d6c04933abbc..31ab764cf4e3 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4527,8 +4527,6 @@ static const struct extent_io_ops btree_extent_io_ops = {
> >  	/* mandatory callbacks */
> >  	.submit_bio_hook = btree_submit_bio_hook,
> >  	.readpage_end_io_hook = btree_readpage_end_io_hook,
> > -	/* note we're sharing with inode.c for the merge bio hook */
> > -	.merge_bio_hook = btrfs_merge_bio_hook,
> >  	.readpage_io_failed_hook = btree_io_failed_hook,
> >  	.set_range_writeback = btrfs_set_range_writeback,
> >  
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index f7a138278f16..55a845621f85 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -2784,8 +2784,8 @@ static int submit_extent_page(unsigned int opf, struct extent_io_tree *tree,
> >  		else
> >  			contig = bio_end_sector(bio) == sector;
> >  
> > -		if (tree->ops && tree->ops->merge_bio_hook(page, offset,
> > -					page_size, bio, bio_flags))
> > +		if (tree->ops && btrfs_merge_bio_hook(page, offset, page_size,
> > +						      bio, bio_flags))
> 
> While this is correct I dislike having the if (tree->ops ) check. So
> under what conditions do we NOT have the tree->ops set? Looking at the
> code it seems we are setting it everytime we create an inode in
> btrfs_create. We also set it for symlinks (btrfs_symlink) and a tmp file
> (btrfs_tmpfile). Strangely enough in btrfs_read_locked_inode we only set
> it for regular files (S_IFREG).

That's correct. I did not notice setting extent_io ops in the symlink
and tmpfile functions and found only the weirdness in
btrfs_read_locked_inode, but it looks like we can drop the tree->ops
completely.

> submit_extent_page seems to be called for both metadata but as we no
> longer distinguish between eb merge_hook and data merge_hook perhaps the
> tree->ops could be killed as well ?

I have WIP patches that replace the callbacks that touch the callsites
doing the tree->ops checks, so I'll keep it mind that there are more
cleanups to be done.

[PATCH 7/7] btrfs: drop extent_io_ops::set_range_writeback callback

[David Sterba]

The data and metadata callback implementation both use the same
function. We can remove the call indirection and intermediate helper
completely.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index ee1e152cb94b..9c638931b75e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3172,7 +3172,7 @@ int btrfs_create_subvol_root(struct btrfs_trans_handle *trans,
 int btrfs_merge_bio_hook(struct page *page, unsigned long offset,
 			 size_t size, struct bio *bio,
 			 unsigned long bio_flags);
-void btrfs_set_range_writeback(void *private_data, u64 start, u64 end);
+void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end);
 vm_fault_t btrfs_page_mkwrite(struct vm_fault *vmf);
 int btrfs_readpage(struct file *file, struct page *page);
 void btrfs_evict_inode(struct inode *inode);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 31ab764cf4e3..4e02c2ac1a96 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4528,7 +4528,6 @@ static const struct extent_io_ops btree_extent_io_ops = {
 	.submit_bio_hook = btree_submit_bio_hook,
 	.readpage_end_io_hook = btree_readpage_end_io_hook,
 	.readpage_io_failed_hook = btree_io_failed_hook,
-	.set_range_writeback = btrfs_set_range_writeback,
 
 	/* optional callbacks */
 };
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 55a845621f85..a427e44c6613 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -1380,14 +1380,6 @@ void extent_range_redirty_for_io(struct inode *inode, u64 start, u64 end)
 	}
 }
 
-/*
- * helper function to set both pages and extents in the tree writeback
- */
-static void set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)
-{
-	tree->ops->set_range_writeback(tree->private_data, start, end);
-}
-
 /* find the first state struct with 'bits' set after 'start', and
  * return it.  tree->lock must be held.  NULL will returned if
  * nothing was found after 'start'
@@ -3416,7 +3408,7 @@ static noinline_for_stack int __extent_writepage_io(struct inode *inode,
 			continue;
 		}
 
-		set_range_writeback(tree, cur, cur + iosize - 1);
+		btrfs_set_range_writeback(tree, cur, cur + iosize - 1);
 		if (!PageWriteback(page)) {
 			btrfs_err(BTRFS_I(inode)->root->fs_info,
 				   "page %lu not writeback, cur %llu end %llu",
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index ec7c4a2982d0..57c2c31f3170 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -102,7 +102,6 @@ struct extent_io_ops {
 				    struct page *page, u64 start, u64 end,
 				    int mirror);
 	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
-	void (*set_range_writeback)(void *private_data, u64 start, u64 end);
 
 	/*
 	 * Optional hooks, called if the pointer is not NULL
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 845621a71468..886c205b77f6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10482,9 +10482,9 @@ static void btrfs_check_extent_io_range(void *private_data, const char *caller,
 	}
 }
 
-void btrfs_set_range_writeback(void *private_data, u64 start, u64 end)
+void btrfs_set_range_writeback(struct extent_io_tree *tree, u64 start, u64 end)
 {
-	struct inode *inode = private_data;
+	struct inode *inode = tree->private_data;
 	unsigned long index = start >> PAGE_SHIFT;
 	unsigned long end_index = end >> PAGE_SHIFT;
 	struct page *page;
@@ -10541,7 +10541,6 @@ static const struct extent_io_ops btrfs_extent_io_ops = {
 	.submit_bio_hook = btrfs_submit_bio_hook,
 	.readpage_end_io_hook = btrfs_readpage_end_io_hook,
 	.readpage_io_failed_hook = btrfs_readpage_io_failed_hook,
-	.set_range_writeback = btrfs_set_range_writeback,
 
 	/* optional callbacks */
 	.fill_delalloc = run_delalloc_range,
-- 
2.18.0

Re: [PATCH 0/7] Structure and callback cleanups

[Nikolay Borisov]

On 19.07.2018 14:05, David Sterba wrote:
> A handful of removed structure members that are not used,
> async_submit_bio is down by 16 bytes and async_cow by 8.
> 
> Some of the extent_io_ops callbacks are unnecessarily called indirectly.
> The rest of extent_io_ops is going to be transformed in following
> series.
> 
> David Sterba (7):
>   btrfs: remove unused member async_submit_bio::fs_info
>   btrfs: remove unused member async_submit_bio::bio_flags
>   btrfs: remove redundant member async_cow::root
>   btrfs: unify end_io callbacks of async_submit_bio
>   btrfs: drop extent_io_ops::tree_fs_info callback
>   btrfs: drop extent_io_ops::merge_bio_hook callback
>   btrfs: drop extent_io_ops::set_range_writeback callback
> 
>  fs/btrfs/compression.c | 10 +++-------
>  fs/btrfs/ctree.h       |  2 +-
>  fs/btrfs/disk-io.c     | 42 +++---------------------------------------
>  fs/btrfs/disk-io.h     |  5 +++--
>  fs/btrfs/extent_io.c   | 28 +++++++---------------------
>  fs/btrfs/extent_io.h   |  8 --------
>  fs/btrfs/inode.c       | 32 ++++++++------------------------
>  7 files changed, 25 insertions(+), 102 deletions(-)
> 

For the whole series (apart from my comments on 6/7):

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

I'm guessing this will alleviate some of the performance hit stemming
from  spectre/meltdown mitigations.