[PATCH 0/6] Simplifications around submit_bio_hook

[PATCH 0/6] Simplifications around submit_bio_hook

[Nikolay Borisov]

I've been taking a look at the submit_bio_hook et al  and I saw some
opportunities for improvement. First I begin by simplifying the definition of 
submit_bio_hook callback and making one of its arguments explicit. 

Patch 3 removes a redundant extent_io_tree argument that was passed through 
several layers of functions and ultimately made it somewhat cumbersome to reason
what extent_io_tree is being used. 

Patch 4 and 5 hardcode a 0 being passed  for the bio_offset parameter to
btrfs_submit_bio_start/btree_submit_bio_start since it only matters for DIO. 

Patch 6 finally does away with the bio_offset parameter of submit_bio_hook. 

Here is the output of bloat-o-meter for posterity:

add/remove: 0/0 grow/shrink: 4/7 up/down: 48/-319 (-271)
Function                                     old     new   delta
read_extent_buffer_pages                     784     822     +38
btrfs_add_ordered_sum                        100     104      +4
btrfs_submit_bio_hook                        364     367      +3
btree_submit_bio_hook                        175     178      +3
end_bio_extent_readpage                     2119    2116      -3
btrfs_reloc_clone_csums                      272     262     -10
btrfs_csum_one_bio                          1358    1348     -10
btree_read_extent_buffer_pages               282     272     -10
readahead_tree_block                          73      55     -18
reada_tree_block_flagged                     194     165     -29
submit_one_bio                               313      74    -239
Total: Before=1072719, After=1072448, chg -0.03%

Nikolay Borisov (6):
  btrfs: Define submit_bio_hook's type directly
  btrfs: Change submit_bio_hook to taking an inode directly
  btrfs: Remove 'tree' argument from read_extent_buffer_pages
  btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio
  btrfs: Always pass 0 bio_offset for btree_submit_bio_start
  btrfs: Remove bio_offset argument from submit_bio_hook

 fs/btrfs/disk-io.c   | 22 +++++++---------------
 fs/btrfs/extent_io.c | 10 ++++------
 fs/btrfs/extent_io.h |  9 +++------
 fs/btrfs/inode.c     | 11 +++++------
 4 files changed, 19 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/6] btrfs: Define submit_bio_hook's type directly

[Nikolay Borisov]

There is no need to use a typedef to define the type of the function
and then use that to define the respective member in extent_io_ops.
Define struct's member directly. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index f7ca1516f70b..79bd20cf4226 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -95,9 +95,6 @@ struct btrfs_inode;
 struct btrfs_io_bio;
 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, u64 bio_offset);
@@ -107,7 +104,9 @@ struct extent_io_ops {
 	 * The following callbacks must be always defined, the function
 	 * pointer will be called unconditionally.
 	 */
-	extent_submit_bio_hook_t *submit_bio_hook;
+	blk_status_t (*submit_bio_hook)(void *private_data, struct bio *bio,
+					int mirror_num, unsigned long bio_flags,
+					u64 bio_offset);
 	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
 				    struct page *page, u64 start, u64 end,
 				    int mirror);
-- 
2.17.1

[PATCH 2/6] btrfs: Change submit_bio_hook to taking an inode directly

[Nikolay Borisov]

The only possible 'private_data' that is passed to this function is
actually an inode. Make that explicit by changing the signature of the
call back. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c   | 5 ++---
 fs/btrfs/extent_io.h | 2 +-
 fs/btrfs/inode.c     | 3 +--
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 31bdedfb7490..b05adaf2bb80 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -885,11 +885,10 @@ static int check_async_write(struct btrfs_inode *bi)
 	return 1;
 }
 
-static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
+static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
 					  int mirror_num, unsigned long bio_flags,
 					  u64 bio_offset)
 {
-	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int async = check_async_write(BTRFS_I(inode));
 	blk_status_t ret;
@@ -915,7 +914,7 @@ static blk_status_t btree_submit_bio_hook(void *private_data, struct bio *bio,
 		 * checksumming can happen in parallel across all CPUs
 		 */
 		ret = btrfs_wq_submit_bio(fs_info, bio, mirror_num, 0,
-					  bio_offset, private_data,
+					  bio_offset, inode,
 					  btree_submit_bio_start);
 	}
 
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 79bd20cf4226..abafb48947ef 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -104,7 +104,7 @@ struct extent_io_ops {
 	 * The following callbacks must be always defined, the function
 	 * pointer will be called unconditionally.
 	 */
-	blk_status_t (*submit_bio_hook)(void *private_data, struct bio *bio,
+	blk_status_t (*submit_bio_hook)(struct inode *inode, struct bio *bio,
 					int mirror_num, unsigned long bio_flags,
 					u64 bio_offset);
 	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4e2f9f66bf59..07cf7050c99f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1953,11 +1953,10 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,
  *
  *    c-3) otherwise:			async submit
  */
-static blk_status_t btrfs_submit_bio_hook(void *private_data, struct bio *bio,
+static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
 				 int mirror_num, unsigned long bio_flags,
 				 u64 bio_offset)
 {
-	struct inode *inode = private_data;
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 	enum btrfs_wq_endio_type metadata = BTRFS_WQ_ENDIO_DATA;
-- 
2.17.1

[PATCH 3/6] btrfs: Remove 'tree' argument from read_extent_buffer_pages

[Nikolay Borisov]

This function always uses the btree inode's io_tree. Stop taking the
tree as a function argument and instead access it internally from
read_extent_buffer_pages. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c   | 12 +++---------
 fs/btrfs/extent_io.c |  4 ++--
 fs/btrfs/extent_io.h |  3 +--
 3 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b05adaf2bb80..2f535e9d816e 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -465,8 +465,7 @@ static int btree_read_extent_buffer_pages(struct extent_buffer *eb,
 	io_tree = &BTRFS_I(fs_info->btree_inode)->io_tree;
 	while (1) {
 		clear_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
-		ret = read_extent_buffer_pages(io_tree, eb, WAIT_COMPLETE,
-					       mirror_num);
+		ret = read_extent_buffer_pages(eb, WAIT_COMPLETE, mirror_num);
 		if (!ret) {
 			if (verify_parent_transid(io_tree, eb,
 						   parent_transid, 0))
@@ -1034,15 +1033,13 @@ static const struct address_space_operations btree_aops = {
 void readahead_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr)
 {
 	struct extent_buffer *buf = NULL;
-	struct inode *btree_inode = fs_info->btree_inode;
 	int ret;
 
 	buf = btrfs_find_create_tree_block(fs_info, bytenr);
 	if (IS_ERR(buf))
 		return;
 
-	ret = read_extent_buffer_pages(&BTRFS_I(btree_inode)->io_tree, buf,
-			WAIT_NONE, 0);
+	ret = read_extent_buffer_pages(buf, WAIT_NONE, 0);
 	if (ret < 0)
 		free_extent_buffer_stale(buf);
 	else
@@ -1053,8 +1050,6 @@ int reada_tree_block_flagged(struct btrfs_fs_info *fs_info, u64 bytenr,
 			 int mirror_num, struct extent_buffer **eb)
 {
 	struct extent_buffer *buf = NULL;
-	struct inode *btree_inode = fs_info->btree_inode;
-	struct extent_io_tree *io_tree = &BTRFS_I(btree_inode)->io_tree;
 	int ret;
 
 	buf = btrfs_find_create_tree_block(fs_info, bytenr);
@@ -1063,8 +1058,7 @@ int reada_tree_block_flagged(struct btrfs_fs_info *fs_info, u64 bytenr,
 
 	set_bit(EXTENT_BUFFER_READAHEAD, &buf->bflags);
 
-	ret = read_extent_buffer_pages(io_tree, buf, WAIT_PAGE_LOCK,
-				       mirror_num);
+	ret = read_extent_buffer_pages(buf, WAIT_PAGE_LOCK, mirror_num);
 	if (ret) {
 		free_extent_buffer_stale(buf);
 		return ret;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 828708f6510c..048d6c6fe7b9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5330,8 +5330,7 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb)
 	}
 }
 
-int read_extent_buffer_pages(struct extent_io_tree *tree,
-			     struct extent_buffer *eb, int wait, int mirror_num)
+int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
 {
 	int i;
 	struct page *page;
@@ -5343,6 +5342,7 @@ int read_extent_buffer_pages(struct extent_io_tree *tree,
 	unsigned long num_reads = 0;
 	struct bio *bio = NULL;
 	unsigned long bio_flags = 0;
+	struct extent_io_tree *tree = &BTRFS_I(eb->fs_info->btree_inode)->io_tree;
 
 	if (test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))
 		return 0;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index abafb48947ef..37240e03c4e3 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -433,8 +433,7 @@ void free_extent_buffer_stale(struct extent_buffer *eb);
 #define WAIT_NONE	0
 #define WAIT_COMPLETE	1
 #define WAIT_PAGE_LOCK	2
-int read_extent_buffer_pages(struct extent_io_tree *tree,
-			     struct extent_buffer *eb, int wait,
+int read_extent_buffer_pages(struct extent_buffer *eb, int wait,
 			     int mirror_num);
 void wait_on_extent_buffer_writeback(struct extent_buffer *eb);
 
-- 
2.17.1

[PATCH 4/6] btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio

[Nikolay Borisov]

Buffered writeback always calls btrfs_csum_one_bio with the last 2
arguments being 0 irrespective of what the bio_offset has been passed
to btrfs_submit_bio_start. Make this apparent by explicitly passing 0
for bio_offset when calling btrfs_wq_submit_bio from
btrfs_submit_bio_hook. This will allow for further simplifications down
the line. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 07cf7050c99f..6e5f1afa7407 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1991,8 +1991,7 @@ static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
 			goto mapit;
 		/* 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);
+					  0, inode, btrfs_submit_bio_start);
 		goto out;
 	} else if (!skip_sum) {
 		ret = btrfs_csum_one_bio(inode, bio, 0, 0);
-- 
2.17.1

[PATCH 5/6] btrfs: Always pass 0 bio_offset for btree_submit_bio_start

[Nikolay Borisov]

The btree submit hook queues the async csum and forwards the bio_offset
parameter passed to btree_submit_bio_hook. This is redundant since
btree_submit_bio_start calls btree_csum_one_bio which doesn't use
the offset at all. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 2f535e9d816e..ee9e03b8aae3 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -913,8 +913,7 @@ static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
 		 * checksumming can happen in parallel across all CPUs
 		 */
 		ret = btrfs_wq_submit_bio(fs_info, bio, mirror_num, 0,
-					  bio_offset, inode,
-					  btree_submit_bio_start);
+					  0, inode, btree_submit_bio_start);
 	}
 
 	if (ret)
-- 
2.17.1

[PATCH 6/6] btrfs: Remove bio_offset argument from submit_bio_hook

[Nikolay Borisov]

None of the implementers of the submit_bio_hook use the bio_offset
parameter, simply remove it. No functional changes.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ee9e03b8aae3..0a7897adc58f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -885,8 +885,8 @@ static int check_async_write(struct btrfs_inode *bi)
 }
 
 static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
-					  int mirror_num, unsigned long bio_flags,
-					  u64 bio_offset)
+					  int mirror_num,
+					  unsigned long bio_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int async = check_async_write(BTRFS_I(inode));
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 048d6c6fe7b9..61191156ee2b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -152,16 +152,14 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 	struct bio_vec *bvec = bio_last_bvec_all(bio);
 	struct bio_vec bv;
 	struct extent_io_tree *tree = bio->bi_private;
-	u64 start;
 
 	mp_bvec_last_segment(bvec, &bv);
-	start = page_offset(bv.bv_page) + bv.bv_offset;
 
 	bio->bi_private = NULL;
 
 	if (tree->ops)
 		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
-					   mirror_num, bio_flags, start);
+						 mirror_num, bio_flags);
 	else
 		btrfsic_submit_bio(bio);
 
@@ -2546,7 +2544,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
 		read_mode, failrec->this_mirror, failrec->in_validation);
 
 	status = tree->ops->submit_bio_hook(tree->private_data, bio, failrec->this_mirror,
-					 failrec->bio_flags, 0);
+					 failrec->bio_flags);
 	if (status) {
 		free_io_failure(failure_tree, tree, failrec);
 		bio_put(bio);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 37240e03c4e3..aa18a16a6ed7 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -105,8 +105,7 @@ struct extent_io_ops {
 	 * pointer will be called unconditionally.
 	 */
 	blk_status_t (*submit_bio_hook)(struct inode *inode, struct bio *bio,
-					int mirror_num, unsigned long bio_flags,
-					u64 bio_offset);
+					int mirror_num, unsigned long bio_flags);
 	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
 				    struct page *page, u64 start, u64 end,
 				    int mirror);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6e5f1afa7407..476111d44021 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1954,8 +1954,9 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,
  *    c-3) otherwise:			async submit
  */
 static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
-				 int mirror_num, unsigned long bio_flags,
-				 u64 bio_offset)
+					  int mirror_num,
+					  unsigned long bio_flags)
+
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-- 
2.17.1

Re: [PATCH 6/6] btrfs: Remove bio_offset argument from submit_bio_hook

[Nikolay Borisov]

On 10.04.19 г. 17:24 ч., Nikolay Borisov wrote:
> None of the implementers of the submit_bio_hook use the bio_offset
> parameter, simply remove it. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/disk-io.c   | 4 ++--
>  fs/btrfs/extent_io.c | 6 ++----
>  fs/btrfs/extent_io.h | 3 +--
>  fs/btrfs/inode.c     | 5 +++--
>  4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index ee9e03b8aae3..0a7897adc58f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -885,8 +885,8 @@ static int check_async_write(struct btrfs_inode *bi)
>  }
>  
>  static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
> -					  int mirror_num, unsigned long bio_flags,
> -					  u64 bio_offset)
> +					  int mirror_num,
> +					  unsigned long bio_flags)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	int async = check_async_write(BTRFS_I(inode));
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 048d6c6fe7b9..61191156ee2b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -152,16 +152,14 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
>  	struct bio_vec *bvec = bio_last_bvec_all(bio);
>  	struct bio_vec bv;
>  	struct extent_io_tree *tree = bio->bi_private;
> -	u64 start;
>  
>  	mp_bvec_last_segment(bvec, &bv);
> -	start = page_offset(bv.bv_page) + bv.bv_offset;

This hunk could be extended even further by removing bv/bvec definitions
as well as the call to mp_bvec_last_segment. Shall I resend or are you
going to fold this change David?

>  
>  	bio->bi_private = NULL;
>  
>  	if (tree->ops)
>  		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
> -					   mirror_num, bio_flags, start);
> +						 mirror_num, bio_flags);
>  	else
>  		btrfsic_submit_bio(bio);
>  
> @@ -2546,7 +2544,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
>  		read_mode, failrec->this_mirror, failrec->in_validation);
>  
>  	status = tree->ops->submit_bio_hook(tree->private_data, bio, failrec->this_mirror,
> -					 failrec->bio_flags, 0);
> +					 failrec->bio_flags);
>  	if (status) {
>  		free_io_failure(failure_tree, tree, failrec);
>  		bio_put(bio);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 37240e03c4e3..aa18a16a6ed7 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -105,8 +105,7 @@ struct extent_io_ops {
>  	 * pointer will be called unconditionally.
>  	 */
>  	blk_status_t (*submit_bio_hook)(struct inode *inode, struct bio *bio,
> -					int mirror_num, unsigned long bio_flags,
> -					u64 bio_offset);
> +					int mirror_num, unsigned long bio_flags);
>  	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
>  				    struct page *page, u64 start, u64 end,
>  				    int mirror);
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 6e5f1afa7407..476111d44021 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1954,8 +1954,9 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,
>   *    c-3) otherwise:			async submit
>   */
>  static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
> -				 int mirror_num, unsigned long bio_flags,
> -				 u64 bio_offset)
> +					  int mirror_num,
> +					  unsigned long bio_flags)
> +
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> 

[PATCH v2] btrfs: Remove bio_offset argument from submit_bio_hook

[Nikolay Borisov]

None of the implementers of the submit_bio_hook use the bio_offset
parameter, simply remove it. No functional changes.

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

Change since v1: 

 * Remove code dealing with bio internals in submit_one_bio.

 fs/btrfs/disk-io.c   |  4 ++--
 fs/btrfs/extent_io.c | 10 ++--------
 fs/btrfs/extent_io.h |  3 +--
 fs/btrfs/inode.c     |  5 +++--
 4 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ee9e03b8aae3..0a7897adc58f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -885,8 +885,8 @@ static int check_async_write(struct btrfs_inode *bi)
 }
 
 static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
-					  int mirror_num, unsigned long bio_flags,
-					  u64 bio_offset)
+					  int mirror_num,
+					  unsigned long bio_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int async = check_async_write(BTRFS_I(inode));
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 048d6c6fe7b9..9aa79ad794c9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -149,19 +149,13 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 				       unsigned long bio_flags)
 {
 	blk_status_t ret = 0;
-	struct bio_vec *bvec = bio_last_bvec_all(bio);
-	struct bio_vec bv;
 	struct extent_io_tree *tree = bio->bi_private;
-	u64 start;
-
-	mp_bvec_last_segment(bvec, &bv);
-	start = page_offset(bv.bv_page) + bv.bv_offset;
 
 	bio->bi_private = NULL;
 
 	if (tree->ops)
 		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
-					   mirror_num, bio_flags, start);
+						 mirror_num, bio_flags);
 	else
 		btrfsic_submit_bio(bio);
 
@@ -2546,7 +2540,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset,
 		read_mode, failrec->this_mirror, failrec->in_validation);
 
 	status = tree->ops->submit_bio_hook(tree->private_data, bio, failrec->this_mirror,
-					 failrec->bio_flags, 0);
+					 failrec->bio_flags);
 	if (status) {
 		free_io_failure(failure_tree, tree, failrec);
 		bio_put(bio);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 37240e03c4e3..aa18a16a6ed7 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -105,8 +105,7 @@ struct extent_io_ops {
 	 * pointer will be called unconditionally.
 	 */
 	blk_status_t (*submit_bio_hook)(struct inode *inode, struct bio *bio,
-					int mirror_num, unsigned long bio_flags,
-					u64 bio_offset);
+					int mirror_num, unsigned long bio_flags);
 	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
 				    struct page *page, u64 start, u64 end,
 				    int mirror);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 6e5f1afa7407..476111d44021 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1954,8 +1954,9 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio,
  *    c-3) otherwise:			async submit
  */
 static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
-				 int mirror_num, unsigned long bio_flags,
-				 u64 bio_offset)
+					  int mirror_num,
+					  unsigned long bio_flags)
+
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_root *root = BTRFS_I(inode)->root;
-- 
2.17.1

Re: [PATCH 1/6] btrfs: Define submit_bio_hook's type directly

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/6] btrfs: Change submit_bio_hook to taking an inode directly

[Johannes Thumshirn]

On 10/04/2019 16:24, Nikolay Borisov wrote:
> The only possible 'private_data' that is passed to this function is
> actually an inode. Make that explicit by changing the signature of the
> call back. No functional changes.

Can't we change struct extent_io_tree::private_data and
extent_io_tree_init(..., void *private_data) to be an inode as well?

If I didn't overlook something we always pass in an inode or NULL.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 2/6] btrfs: Change submit_bio_hook to taking an inode directly

[Nikolay Borisov]

On 11.04.19 г. 14:18 ч., Johannes Thumshirn wrote:
> On 10/04/2019 16:24, Nikolay Borisov wrote:
>> The only possible 'private_data' that is passed to this function is
>> actually an inode. Make that explicit by changing the signature of the
>> call back. No functional changes.
> 
> Can't we change struct extent_io_tree::private_data and
> extent_io_tree_init(..., void *private_data) to be an inode as well?
> 
> If I didn't overlook something we always pass in an inode or NULL.

You are right, latest refactoring I did made it so. I can send patches
atop this series.

David, how do you like to organise this? Resend the series with 2 more
patches in it or shall I send them as separate once this lands?

> 

Re: [PATCH 3/6] btrfs: Remove 'tree' argument from read_extent_buffer_pages

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/6] btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio

[Johannes Thumshirn]

On 10/04/2019 16:24, Nikolay Borisov wrote:
> Buffered writeback always calls btrfs_csum_one_bio with the last 2
> arguments being 0 irrespective of what the bio_offset has been passed
> to btrfs_submit_bio_start. Make this apparent by explicitly passing 0
> for bio_offset when calling btrfs_wq_submit_bio from
> btrfs_submit_bio_hook. This will allow for further simplifications down
> the line. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/inode.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 07cf7050c99f..6e5f1afa7407 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -1991,8 +1991,7 @@ static blk_status_t btrfs_submit_bio_hook(struct inode *inode, struct bio *bio,
>  			goto mapit;
>  		/* 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);
> +					  0, inode, btrfs_submit_bio_start);
>  		goto out;
>  	} else if (!skip_sum) {
>  		ret = btrfs_csum_one_bio(inode, bio, 0, 0);
> 

I think we can also kill 'async_submit_bio::bio_offset' as no other
value than 0 is ever used.


-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/6] btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio

[Johannes Thumshirn]

On 11/04/2019 15:09, Johannes Thumshirn wrote:
> I think we can also kill 'async_submit_bio::bio_offset' as no other
> value than 0 is ever used.

Right, this is what the following patches do. Ignore this.


-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/6] btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio

[Nikolay Borisov]

On 11.04.19 г. 16:10 ч., Johannes Thumshirn wrote:
> On 11/04/2019 15:09, Johannes Thumshirn wrote:
>> I think we can also kill 'async_submit_bio::bio_offset' as no other
>> value than 0 is ever used.
> 
> Right, this is what the following patches do. Ignore this.

Actually they don't particularly kill async_submit_bio::bio_offset. But
it could be done :)

> 
> 

Re: [PATCH 4/6] btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio

[Johannes Thumshirn]

On 11/04/2019 15:17, Nikolay Borisov wrote:
> 
> 
> On 11.04.19 г. 16:10 ч., Johannes Thumshirn wrote:
>> On 11/04/2019 15:09, Johannes Thumshirn wrote:
>>> I think we can also kill 'async_submit_bio::bio_offset' as no other
>>> value than 0 is ever used.
>>
>> Right, this is what the following patches do. Ignore this.
> 
> Actually they don't particularly kill async_submit_bio::bio_offset. But
> it could be done :)


Yes but that's a follow up I guess.
Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 5/6] btrfs: Always pass 0 bio_offset for btree_submit_bio_start

[Johannes Thumshirn]

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2] btrfs: Remove bio_offset argument from submit_bio_hook

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Here you could also add the removal of bio_offset from async_submit_bio.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2] btrfs: Remove bio_offset argument from submit_bio_hook

[Nikolay Borisov]

On 11.04.19 г. 16:29 ч., Johannes Thumshirn wrote:
> Looks good,
> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
> 
> Here you could also add the removal of bio_offset from async_submit_bio.
> 

Actually no, we can't since for DIO case we do use it from:
btrfs_submit_dio_bio->btrfs_wq_submit_bio

Re: [PATCH 0/6] Simplifications around submit_bio_hook

[David Sterba]

On Wed, Apr 10, 2019 at 05:24:37PM +0300, Nikolay Borisov wrote:
> I've been taking a look at the submit_bio_hook et al  and I saw some
> opportunities for improvement. First I begin by simplifying the definition of 
> submit_bio_hook callback and making one of its arguments explicit. 
> 
> Patch 3 removes a redundant extent_io_tree argument that was passed through 
> several layers of functions and ultimately made it somewhat cumbersome to reason
> what extent_io_tree is being used. 
> 
> Patch 4 and 5 hardcode a 0 being passed  for the bio_offset parameter to
> btrfs_submit_bio_start/btree_submit_bio_start since it only matters for DIO. 
> 
> Patch 6 finally does away with the bio_offset parameter of submit_bio_hook. 
> 
> Here is the output of bloat-o-meter for posterity:
> 
> add/remove: 0/0 grow/shrink: 4/7 up/down: 48/-319 (-271)
> Function                                     old     new   delta
> read_extent_buffer_pages                     784     822     +38
> btrfs_add_ordered_sum                        100     104      +4
> btrfs_submit_bio_hook                        364     367      +3
> btree_submit_bio_hook                        175     178      +3
> end_bio_extent_readpage                     2119    2116      -3
> btrfs_reloc_clone_csums                      272     262     -10
> btrfs_csum_one_bio                          1358    1348     -10
> btree_read_extent_buffer_pages               282     272     -10
> readahead_tree_block                          73      55     -18
> reada_tree_block_flagged                     194     165     -29
> submit_one_bio                               313      74    -239
> Total: Before=1072719, After=1072448, chg -0.03%
> 
> Nikolay Borisov (6):
>   btrfs: Define submit_bio_hook's type directly
>   btrfs: Change submit_bio_hook to taking an inode directly
>   btrfs: Remove 'tree' argument from read_extent_buffer_pages
>   btrfs: Pass 0 for bio_offset to btrfs_wq_submit_bio
>   btrfs: Always pass 0 bio_offset for btree_submit_bio_start
>   btrfs: Remove bio_offset argument from submit_bio_hook

Patchset added to misc-next, thanks.