[PATCH 0/7] Remove struct extent_io_ops

[PATCH 0/7] Remove struct extent_io_ops

[Nikolay Borisov]

Finally it's time to remove "struct extent_io_ops" and get rid of the indirect
calls to the "hook" functions. Each patch is rather self-explanatory, the basic
idea is to replace indirect calls with an "if" construct (patches 1,3,5,6). The
rest simply remove struct members and the struct it self.

This series survived a full xfstest run.

Nikolay Borisov (7):
  btrfs: Don't call readpage_end_io_hook for the btree inode
  btrfs: Remove extent_io_ops::readpage_end_io_hook
  btrfs: Call submit_bio_hook directly in submit_one_bio
  btrfs: Don't opencode is_data_inode in end_bio_extent_readpage
  btrfs: Stop calling submit_bio_hook for data inodes
  btrfs: Call submit_bio_hook directly for metadata pages
  btrfs: Remove struct extent_io_ops

 fs/btrfs/ctree.h             |  6 ++++--
 fs/btrfs/disk-io.c           | 20 +++++---------------
 fs/btrfs/disk-io.h           |  6 +++++-
 fs/btrfs/extent-io-tree.h    |  1 -
 fs/btrfs/extent_io.c         | 25 +++++++++++++------------
 fs/btrfs/extent_io.h         |  5 +----
 fs/btrfs/inode.c             | 28 ++++------------------------
 fs/btrfs/tests/inode-tests.c |  1 -
 8 files changed, 32 insertions(+), 60 deletions(-)

--
2.17.1

[PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[Nikolay Borisov]

Instead of relying on indirect calls to implement metadata buffer
validation simply check if the inode whose page we are processing equals
the btree inode. If it does call the necessary function.

This is an improvement in 2 directions:
1. We aren't paying the penalty of indirect calls in a post-speculation
   attacks world.

2. The function is now named more explicitly so it's obvious what's
   going on

This is in preparation to removing struct extent_io_ops altogether.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4e667b0565e0..0c58d96b9fb3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2962,6 +2962,8 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode,
 u64 btrfs_file_extent_end(const struct btrfs_path *path);
 
 /* inode.c */
+int btrfs_check_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
+		     struct page *page, u64 start, u64 end, int mirror);
 struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
 					   u64 start, u64 len);
 noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 160b485d2cc0..5ad11c38230f 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -524,9 +524,9 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
 	return 1;
 }
 
-static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
-				      u64 phy_offset, struct page *page,
-				      u64 start, u64 end, int mirror)
+int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
+				   struct page *page, u64 start, u64 end,
+				   int mirror)
 {
 	u64 found_start;
 	int found_level;
@@ -4639,5 +4639,5 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 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,
+	.readpage_end_io_hook = NULL
 };
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 89b6a709a184..bc2e49246199 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -76,7 +76,9 @@ void btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info);
 void btrfs_btree_balance_dirty_nodelay(struct btrfs_fs_info *fs_info);
 void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 				 struct btrfs_root *root);
-
+int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
+				   struct page *page, u64 start, u64 end,
+				   int mirror);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
 #endif
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index afac70ef0cc5..5e47606f7786 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
 
 		mirror = io_bio->mirror_num;
 		if (likely(uptodate)) {
-			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
-							      page, start, end,
-							      mirror);
+			if (data_inode)
+				ret = btrfs_check_csum(io_bio, offset, page,
+						       start, end, mirror);
+			else
+				ret = btrfs_validate_metadata_buffer(io_bio,
+					offset, page, start, end, mirror);
 			if (ret)
 				uptodate = 0;
 			else
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index cb3fdd0798c6..23ac09aa813e 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2817,9 +2817,8 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
  * if there's a match, we allow the bio to finish.  If not, the code in
  * extent_io.c will try to find good copies for us.
  */
-static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
-				      u64 phy_offset, struct page *page,
-				      u64 start, u64 end, int mirror)
+int btrfs_check_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
+		     struct page *page, u64 start, u64 end, int mirror)
 {
 	size_t offset = start - page_offset(page);
 	struct inode *inode = page->mapping->host;
@@ -10249,7 +10248,7 @@ static const struct file_operations btrfs_dir_file_operations = {
 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,
+	.readpage_end_io_hook = NULL
 };
 
 /*
-- 
2.17.1

[PATCH 2/7] btrfs: Remove extent_io_ops::readpage_end_io_hook

[Nikolay Borisov]

It's no longer used so let's remove it.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5ad11c38230f..73937954f464 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4637,7 +4637,5 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 }
 
 static const struct extent_io_ops btree_extent_io_ops = {
-	/* mandatory callbacks */
-	.submit_bio_hook = btree_submit_bio_hook,
-	.readpage_end_io_hook = NULL
+	.submit_bio_hook = btree_submit_bio_hook
 };
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 133487a5e6b8..d9119bd555a9 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -77,13 +77,10 @@ typedef blk_status_t (extent_submit_bio_start_t)(void *private_data,
 
 struct extent_io_ops {
 	/*
-	 * The following callbacks must be always defined, the function
+	 * The following callback must be always defined, the function
 	 * pointer will be called unconditionally.
 	 */
 	submit_bio_hook_t *submit_bio_hook;
-	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 23ac09aa813e..41565c5a05ef 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -10246,9 +10246,7 @@ static const struct file_operations btrfs_dir_file_operations = {
 };
 
 static const struct extent_io_ops btrfs_extent_io_ops = {
-	/* mandatory callbacks */
-	.submit_bio_hook = btrfs_submit_bio_hook,
-	.readpage_end_io_hook = NULL
+	.submit_bio_hook = btrfs_submit_bio_hook
 };
 
 /*
-- 
2.17.1

[PATCH 3/7] btrfs: Call submit_bio_hook directly in submit_one_bio

[Nikolay Borisov]

BTRFS has 2 inode types (for the purposes of the code in submit_one_bio)
- ordinary data inodes (including the freespace inode) and the btree
inode. Both of these implement submit_bio_hook so btrfsic_submit_bio can
never be called from submit_one_bio so just remove it.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 5e47606f7786..6e976bd86600 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -168,11 +168,8 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 
 	bio->bi_private = NULL;
 
-	if (tree->ops)
-		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
-						 mirror_num, bio_flags);
-	else
-		btrfsic_submit_bio(bio);
+	ret = tree->ops->submit_bio_hook(tree->private_data, bio, mirror_num,
+					 bio_flags);
 
 	return blk_status_to_errno(ret);
 }
-- 
2.17.1

[PATCH 4/7] btrfs: Don't opencode is_data_inode in end_bio_extent_readpage

[Nikolay Borisov]

Use the is_data_inode helper.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 6e976bd86600..26b002e2f3b3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2816,8 +2816,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 		struct page *page = bvec->bv_page;
 		struct inode *inode = page->mapping->host;
 		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-		bool data_inode = btrfs_ino(BTRFS_I(inode))
-			!= BTRFS_BTREE_INODE_OBJECTID;
+		bool data_inode = is_data_inode(inode);
 
 		btrfs_debug(fs_info,
 			"end_bio_extent_readpage: bi_sector=%llu, err=%d, mirror=%u",
-- 
2.17.1

[PATCH 5/7] btrfs: Stop calling submit_bio_hook for data inodes

[Nikolay Borisov]

Instead export and rename the function to btrfs_submit_data_bio and
call it directly in submit_one_bio. This avoids paying the cost for
speculative attacks mitigations and improves code readability.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h     |  2 ++
 fs/btrfs/extent_io.c | 10 +++++++---
 fs/btrfs/inode.c     |  7 +++----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0c58d96b9fb3..5fc18b7ab771 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -2962,6 +2962,8 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode,
 u64 btrfs_file_extent_end(const struct btrfs_path *path);
 
 /* inode.c */
+blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
+				   int mirror_num, unsigned long bio_flags);
 int btrfs_check_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
 		     struct page *page, u64 start, u64 end, int mirror);
 struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 26b002e2f3b3..8cabcb7642a9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -168,8 +168,12 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 
 	bio->bi_private = NULL;
 
-	ret = tree->ops->submit_bio_hook(tree->private_data, bio, mirror_num,
-					 bio_flags);
+	if (is_data_inode(tree->private_data))
+		ret = btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
+					    bio_flags);
+	else
+		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
+						 mirror_num, bio_flags);
 
 	return blk_status_to_errno(ret);
 }
@@ -2880,7 +2884,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 			if (!btrfs_submit_read_repair(inode, bio, offset, page,
 						start - page_offset(page),
 						start, end, mirror,
-						tree->ops->submit_bio_hook)) {
+						btrfs_submit_data_bio)) {
 				uptodate = !bio->bi_status;
 				offset += len;
 				continue;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 41565c5a05ef..955a66207fec 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2169,9 +2169,8 @@ 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)
+blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
+				   int mirror_num, unsigned long bio_flags)
 
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
@@ -10246,7 +10245,7 @@ static const struct file_operations btrfs_dir_file_operations = {
 };
 
 static const struct extent_io_ops btrfs_extent_io_ops = {
-	.submit_bio_hook = btrfs_submit_bio_hook
+	.submit_bio_hook = NULL
 };
 
 /*
-- 
2.17.1

[PATCH 6/7] btrfs: Call submit_bio_hook directly for metadata pages

[Nikolay Borisov]

No need to go through a function pointer indirection simply call
submit_bio_hook directly by exporting and renaming the helper to
btrfs_submit_metadata_bio. This makes the code more readable and should
result in somewhat faster code due to no longer paying the price for
specualtive attack mitigations that come with indirect function calls.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 73937954f464..54f2b95cc305 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -815,9 +815,8 @@ static int check_async_write(struct btrfs_fs_info *fs_info,
 	return 1;
 }
 
-static blk_status_t btree_submit_bio_hook(struct inode *inode, struct bio *bio,
-					  int mirror_num,
-					  unsigned long bio_flags)
+blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
+				       int mirror_num, unsigned long bio_flags)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	int async = check_async_write(fs_info, BTRFS_I(inode));
@@ -4637,5 +4636,5 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 }
 
 static const struct extent_io_ops btree_extent_io_ops = {
-	.submit_bio_hook = btree_submit_bio_hook
+	.submit_bio_hook = NULL
 };
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index bc2e49246199..fee69ced58b4 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -79,6 +79,8 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
 int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
 				   struct page *page, u64 start, u64 end,
 				   int mirror);
+blk_status_t btrfs_submit_metadata_bio(struct inode *inode, struct bio *bio,
+				       int mirror_num, unsigned long bio_flags);
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
 #endif
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8cabcb7642a9..4a00cfd4082f 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -172,8 +172,8 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 		ret = btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
 					    bio_flags);
 	else
-		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
-						 mirror_num, bio_flags);
+		ret = btrfs_submit_metadata_bio(tree->private_data, bio,
+						mirror_num, bio_flags);
 
 	return blk_status_to_errno(ret);
 }
-- 
2.17.1

[PATCH 7/7] btrfs: Remove struct extent_io_ops

[Nikolay Borisov]

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h             |  2 --
 fs/btrfs/disk-io.c           |  7 -------
 fs/btrfs/extent-io-tree.h    |  1 -
 fs/btrfs/extent_io.c         |  2 --
 fs/btrfs/inode.c             | 16 ----------------
 fs/btrfs/tests/inode-tests.c |  1 -
 6 files changed, 29 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 5fc18b7ab771..8e811debae57 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3576,9 +3576,7 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
 
 /* Sanity test specific functions */
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-void btrfs_test_inode_set_ops(struct inode *inode);
 void btrfs_test_destroy_inode(struct inode *inode);
-
 static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
 {
 	return test_bit(BTRFS_FS_STATE_DUMMY_FS_INFO, &fs_info->fs_state);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 54f2b95cc305..85b59797a4a4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -50,7 +50,6 @@
 				 BTRFS_SUPER_FLAG_METADUMP |\
 				 BTRFS_SUPER_FLAG_METADUMP_V2)
 
-static const struct extent_io_ops btree_extent_io_ops;
 static void end_workqueue_fn(struct btrfs_work *work);
 static void btrfs_destroy_ordered_extents(struct btrfs_root *root);
 static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
@@ -2066,8 +2065,6 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info)
 	BTRFS_I(inode)->io_tree.track_uptodate = false;
 	extent_map_tree_init(&BTRFS_I(inode)->extent_tree);
 
-	BTRFS_I(inode)->io_tree.ops = &btree_extent_io_ops;
-
 	BTRFS_I(inode)->root = btrfs_grab_root(fs_info->tree_root);
 	memset(&BTRFS_I(inode)->location, 0, sizeof(struct btrfs_key));
 	set_bit(BTRFS_INODE_DUMMY, &BTRFS_I(inode)->runtime_flags);
@@ -4634,7 +4631,3 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 
 	return 0;
 }
-
-static const struct extent_io_ops btree_extent_io_ops = {
-	.submit_bio_hook = NULL
-};
diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index 250b8cbaaf97..b1b737e7ef5b 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -62,7 +62,6 @@ struct extent_io_tree {
 	u8 owner;
 
 	spinlock_t lock;
-	const struct extent_io_ops *ops;
 };
 
 struct extent_state {
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 4a00cfd4082f..b5e00b62bcb1 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -281,7 +281,6 @@ void extent_io_tree_init(struct btrfs_fs_info *fs_info,
 {
 	tree->fs_info = fs_info;
 	tree->state = RB_ROOT;
-	tree->ops = NULL;
 	tree->dirty_bytes = 0;
 	spin_lock_init(&tree->lock);
 	tree->private_data = private_data;
@@ -3056,7 +3055,6 @@ static int submit_extent_page(unsigned int opf,
 		else
 			contig = bio_end_sector(bio) == sector;
 
-		ASSERT(tree->ops);
 		if (btrfs_bio_fits_in_stripe(page, page_size, bio, bio_flags))
 			can_merge = false;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 955a66207fec..befbe19996b8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -71,7 +71,6 @@ static const struct inode_operations btrfs_special_inode_operations;
 static const struct inode_operations btrfs_file_inode_operations;
 static const struct address_space_operations btrfs_aops;
 static const struct file_operations btrfs_dir_file_operations;
-static const struct extent_io_ops btrfs_extent_io_ops;
 
 static struct kmem_cache *btrfs_inode_cachep;
 struct kmem_cache *btrfs_trans_handle_cachep;
@@ -141,13 +140,6 @@ static inline void btrfs_cleanup_ordered_extents(struct btrfs_inode *inode,
 
 static int btrfs_dirty_inode(struct inode *inode);
 
-#ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-void btrfs_test_inode_set_ops(struct inode *inode)
-{
-	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
-}
-#endif
-
 static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
 				     struct inode *inode,  struct inode *dir,
 				     const struct qstr *qstr)
@@ -3376,7 +3368,6 @@ static int btrfs_read_locked_inode(struct inode *inode,
 	switch (inode->i_mode & S_IFMT) {
 	case S_IFREG:
 		inode->i_mapping->a_ops = &btrfs_aops;
-		BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
 		inode->i_fop = &btrfs_file_operations;
 		inode->i_op = &btrfs_file_inode_operations;
 		break;
@@ -6292,7 +6283,6 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry,
 	if (err)
 		goto out_unlock;
 
-	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
 	d_instantiate_new(dentry, inode);
 
 out_unlock:
@@ -9504,7 +9494,6 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry,
 	inode->i_fop = &btrfs_file_operations;
 	inode->i_op = &btrfs_file_inode_operations;
 	inode->i_mapping->a_ops = &btrfs_aops;
-	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
 
 	err = btrfs_init_inode_security(trans, inode, dir, &dentry->d_name);
 	if (err)
@@ -9826,7 +9815,6 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 	inode->i_op = &btrfs_file_inode_operations;
 
 	inode->i_mapping->a_ops = &btrfs_aops;
-	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
 
 	ret = btrfs_init_inode_security(trans, inode, dir, NULL);
 	if (ret)
@@ -10244,10 +10232,6 @@ static const struct file_operations btrfs_dir_file_operations = {
 	.fsync		= btrfs_sync_file,
 };
 
-static const struct extent_io_ops btrfs_extent_io_ops = {
-	.submit_bio_hook = NULL
-};
-
 /*
  * btrfs doesn't support the bmap operation because swapfiles
  * use bmap to make a mapping of extents in the file.  They assume
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index cc54d4973a74..e6719f7db386 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -949,7 +949,6 @@ static int test_extent_accounting(u32 sectorsize, u32 nodesize)
 	}
 
 	BTRFS_I(inode)->root = root;
-	btrfs_test_inode_set_ops(inode);
 
 	/* [BTRFS_MAX_EXTENT_SIZE] */
 	ret = btrfs_set_extent_delalloc(BTRFS_I(inode), 0,
-- 
2.17.1

Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[Nikolay Borisov]

On 18.09.20 г. 16:34 ч., Nikolay Borisov wrote:
> Instead of relying on indirect calls to implement metadata buffer
> validation simply check if the inode whose page we are processing equals
> the btree inode. If it does call the necessary function.
> 
> This is an improvement in 2 directions:
> 1. We aren't paying the penalty of indirect calls in a post-speculation
>    attacks world.
> 
> 2. The function is now named more explicitly so it's obvious what's
>    going on
> 
> This is in preparation to removing struct extent_io_ops altogether.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---

So this patch does a bit more than it states because it's also modifying
the readpage_end_io_hook for data nodes as well. Other than that the
code is correct. I'd reword the changelog to the following:

Subject: Call readpage_end_io_hook directly

"
Instead of relying on indirect calls to implement post-read processing
simply distinguish between data/metadata pages and call the
corresponding function. This patch also renames and exports the 2 hooks
giving them more clear names.

This is an improvement in 2 directions:
1. We aren't paying the penalty of indirect calls in a post-speculation
   attacks world.

2. The function is now named more explicitly so it's obvious what's
    going on

 This is in preparation to removing struct extent_io_ops altogether.
"

>  fs/btrfs/ctree.h     | 2 ++
>  fs/btrfs/disk-io.c   | 8 ++++----
>  fs/btrfs/disk-io.h   | 4 +++-
>  fs/btrfs/extent_io.c | 9 ++++++---
>  fs/btrfs/inode.c     | 7 +++----
>  5 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4e667b0565e0..0c58d96b9fb3 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -2962,6 +2962,8 @@ void btrfs_inode_safe_disk_i_size_write(struct btrfs_inode *inode,
>  u64 btrfs_file_extent_end(const struct btrfs_path *path);
>  
>  /* inode.c */
> +int btrfs_check_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
> +		     struct page *page, u64 start, u64 end, int mirror);
>  struct extent_map *btrfs_get_extent_fiemap(struct btrfs_inode *inode,
>  					   u64 start, u64 len);
>  noinline int can_nocow_extent(struct inode *inode, u64 offset, u64 *len,
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 160b485d2cc0..5ad11c38230f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -524,9 +524,9 @@ static int check_tree_block_fsid(struct extent_buffer *eb)
>  	return 1;
>  }
>  
> -static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> -				      u64 phy_offset, struct page *page,
> -				      u64 start, u64 end, int mirror)
> +int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
> +				   struct page *page, u64 start, u64 end,
> +				   int mirror)
>  {
>  	u64 found_start;
>  	int found_level;
> @@ -4639,5 +4639,5 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
>  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,
> +	.readpage_end_io_hook = NULL
>  };
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 89b6a709a184..bc2e49246199 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -76,7 +76,9 @@ void btrfs_btree_balance_dirty(struct btrfs_fs_info *fs_info);
>  void btrfs_btree_balance_dirty_nodelay(struct btrfs_fs_info *fs_info);
>  void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>  				 struct btrfs_root *root);
> -
> +int btrfs_validate_metadata_buffer(struct btrfs_io_bio *io_bio, u64 phy_offset,
> +				   struct page *page, u64 start, u64 end,
> +				   int mirror);
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  struct btrfs_root *btrfs_alloc_dummy_root(struct btrfs_fs_info *fs_info);
>  #endif
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index afac70ef0cc5..5e47606f7786 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
>  
>  		mirror = io_bio->mirror_num;
>  		if (likely(uptodate)) {
> -			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
> -							      page, start, end,
> -							      mirror);
> +			if (data_inode)
> +				ret = btrfs_check_csum(io_bio, offset, page,
> +						       start, end, mirror);
> +			else
> +				ret = btrfs_validate_metadata_buffer(io_bio,
> +					offset, page, start, end, mirror);
>  			if (ret)
>  				uptodate = 0;
>  			else
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index cb3fdd0798c6..23ac09aa813e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2817,9 +2817,8 @@ static int check_data_csum(struct inode *inode, struct btrfs_io_bio *io_bio,
>   * if there's a match, we allow the bio to finish.  If not, the code in
>   * extent_io.c will try to find good copies for us.
>   */
> -static int btrfs_readpage_end_io_hook(struct btrfs_io_bio *io_bio,
> -				      u64 phy_offset, struct page *page,
> -				      u64 start, u64 end, int mirror)
> +int btrfs_check_csum(struct btrfs_io_bio *io_bio, u64 phy_offset,
> +		     struct page *page, u64 start, u64 end, int mirror)
>  {
>  	size_t offset = start - page_offset(page);
>  	struct inode *inode = page->mapping->host;
> @@ -10249,7 +10248,7 @@ static const struct file_operations btrfs_dir_file_operations = {
>  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,
> +	.readpage_end_io_hook = NULL
>  };
>  
>  /*
> 

Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[Johannes Thumshirn]

On 18/09/2020 15:34, Nikolay Borisov wrote:
>  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,
> +	.readpage_end_io_hook = NULL

[...]

> @@ -10249,7 +10248,7 @@ static const struct file_operations btrfs_dir_file_operations = {
>  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,
> +	.readpage_end_io_hook = NULL

After your patch both implementations of 
extent_io_ops->readpage_end_io_hook() are gone, why don't you kill the
definition from struct extent_io_ops as well?

Re: [PATCH 2/7] btrfs: Remove extent_io_ops::readpage_end_io_hook

[Johannes Thumshirn]

And here it is. I think these two patches can be merged.

Re: [PATCH 6/7] btrfs: Call submit_bio_hook directly for metadata pages

[Johannes Thumshirn]

On 18/09/2020 15:34, Nikolay Borisov wrote:
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8cabcb7642a9..4a00cfd4082f 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -172,8 +172,8 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
>  		ret = btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
>  					    bio_flags);
>  	else
> -		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
> -						 mirror_num, bio_flags);
> +		ret = btrfs_submit_metadata_bio(tree->private_data, bio,
> +						mirror_num, bio_flags);
>  


Hmm we could even turn this into a little helper calling either
btrfs_submit_data_bio or btrfs_submit_metadata_bio. But that's just stylistic
preference I guess.

Re: [PATCH 0/7] Remove struct extent_io_ops

[Johannes Thumshirn]

On 18/09/2020 15:34, Nikolay Borisov wrote:
> Finally it's time to remove "struct extent_io_ops" and get rid of the indirect
> calls to the "hook" functions. Each patch is rather self-explanatory, the basic
> idea is to replace indirect calls with an "if" construct (patches 1,3,5,6). The
> rest simply remove struct members and the struct it self.
> 
> This series survived a full xfstest run.
> 
> Nikolay Borisov (7):
>   btrfs: Don't call readpage_end_io_hook for the btree inode
>   btrfs: Remove extent_io_ops::readpage_end_io_hook
>   btrfs: Call submit_bio_hook directly in submit_one_bio
>   btrfs: Don't opencode is_data_inode in end_bio_extent_readpage
>   btrfs: Stop calling submit_bio_hook for data inodes
>   btrfs: Call submit_bio_hook directly for metadata pages
>   btrfs: Remove struct extent_io_ops
> 
>  fs/btrfs/ctree.h             |  6 ++++--
>  fs/btrfs/disk-io.c           | 20 +++++---------------
>  fs/btrfs/disk-io.h           |  6 +++++-
>  fs/btrfs/extent-io-tree.h    |  1 -
>  fs/btrfs/extent_io.c         | 25 +++++++++++++------------
>  fs/btrfs/extent_io.h         |  5 +----
>  fs/btrfs/inode.c             | 28 ++++------------------------
>  fs/btrfs/tests/inode-tests.c |  1 -
>  8 files changed, 32 insertions(+), 60 deletions(-)
> 
> --
> 2.17.1
> 
> 

Apart from some nitpicks,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[David Sterba]

On Fri, Sep 18, 2020 at 04:34:33PM +0300, Nikolay Borisov wrote:
> Instead of relying on indirect calls to implement metadata buffer
> validation simply check if the inode whose page we are processing equals
> the btree inode. If it does call the necessary function.
> 
> This is an improvement in 2 directions:
> 1. We aren't paying the penalty of indirect calls in a post-speculation
>    attacks world.
> 
> 2. The function is now named more explicitly so it's obvious what's
>    going on

The new naming is not making things clear, btrfs_check_csum sounds very
generic while it does a very specific thing just by the number and type
of the parameters. Similar for btrfs_validate_metadata_buffer.

> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
>  
>  		mirror = io_bio->mirror_num;
>  		if (likely(uptodate)) {
> -			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
> -							      page, start, end,
> -							      mirror);
> +			if (data_inode)
> +				ret = btrfs_check_csum(io_bio, offset, page,
> +						       start, end, mirror);
> +			else
> +				ret = btrfs_validate_metadata_buffer(io_bio,
> +					offset, page, start, end, mirror);

In the context where the functions are used I'd expect some symmetry,
data/metadata. Something like btrfs_validate_data_bio.

Re: [PATCH 4/7] btrfs: Don't opencode is_data_inode in end_bio_extent_readpage

[David Sterba]

On Fri, Sep 18, 2020 at 04:34:36PM +0300, Nikolay Borisov wrote:
> Use the is_data_inode helper.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent_io.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 6e976bd86600..26b002e2f3b3 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2816,8 +2816,7 @@ static void end_bio_extent_readpage(struct bio *bio)
>  		struct page *page = bvec->bv_page;
>  		struct inode *inode = page->mapping->host;
>  		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -		bool data_inode = btrfs_ino(BTRFS_I(inode))
> -			!= BTRFS_BTREE_INODE_OBJECTID;
> +		bool data_inode = is_data_inode(inode);

I think you can remove the temporary variable and call is_data_inode
directly in the later code, there's only one use.

Re: [PATCH 6/7] btrfs: Call submit_bio_hook directly for metadata pages

[David Sterba]

On Mon, Sep 21, 2020 at 03:04:54PM +0000, Johannes Thumshirn wrote:
> On 18/09/2020 15:34, Nikolay Borisov wrote:
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index 8cabcb7642a9..4a00cfd4082f 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -172,8 +172,8 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
> >  		ret = btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
> >  					    bio_flags);
> >  	else
> > -		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
> > -						 mirror_num, bio_flags);
> > +		ret = btrfs_submit_metadata_bio(tree->private_data, bio,
> > +						mirror_num, bio_flags);
> >  
> 
> 
> Hmm we could even turn this into a little helper calling either
> btrfs_submit_data_bio or btrfs_submit_metadata_bio. But that's just stylistic
> preference I guess.

Yeah a helper could be here but I think it's fine without that extra
indirection, the code is clear that for data inode it's some "data"
function, with the same set of parameters. If there's just one location
switching the two, the helper would not help much IMHO.

Re: [PATCH 7/7] btrfs: Remove struct extent_io_ops

[David Sterba]

On Fri, Sep 18, 2020 at 04:34:39PM +0300, Nikolay Borisov wrote:

You should really write changelogs for patches that not obviously
trivial.

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

Re: [PATCH 4/7] btrfs: Don't opencode is_data_inode in end_bio_extent_readpage

[Nikolay Borisov]

On 21.09.20 г. 23:29 ч., David Sterba wrote:
> On Fri, Sep 18, 2020 at 04:34:36PM +0300, Nikolay Borisov wrote:
>> Use the is_data_inode helper.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent_io.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 6e976bd86600..26b002e2f3b3 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2816,8 +2816,7 @@ static void end_bio_extent_readpage(struct bio *bio)
>>  		struct page *page = bvec->bv_page;
>>  		struct inode *inode = page->mapping->host;
>>  		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>> -		bool data_inode = btrfs_ino(BTRFS_I(inode))
>> -			!= BTRFS_BTREE_INODE_OBJECTID;
>> +		bool data_inode = is_data_inode(inode);
> 
> I think you can remove the temporary variable and call is_data_inode
> directly in the later code, there's only one use.
> 

Actually it's used twice, yet I did an experiment to remove it and
bloat-o-meter indicates it's a win to call the inline function twice:


add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-161 (-161)
Function                                     old     new   delta
end_bio_extent_readpage.cold                 117     104     -13
end_bio_extent_readpage                     1614    1466    -148
Total: Before=45527, After=45366, chg -0.35%


diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b5e00b62bcb1..0994fb56e39a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2819,7 +2819,6 @@ static void end_bio_extent_readpage(struct bio *bio)
                struct page *page = bvec->bv_page;
                struct inode *inode = page->mapping->host;
                struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-               bool data_inode = is_data_inode(inode);

                btrfs_debug(fs_info,
                        "end_bio_extent_readpage: bi_sector=%llu,
err=%d, mirror=%u",
@@ -2850,7 +2849,7 @@ static void end_bio_extent_readpage(struct bio *bio)

                mirror = io_bio->mirror_num;
                if (likely(uptodate)) {
-                       if (data_inode)
+                       if (is_data_inode(inode))
                                ret = btrfs_check_csum(io_bio, offset, page,
                                                       start, end, mirror);
                        else
@@ -2868,7 +2867,7 @@ static void end_bio_extent_readpage(struct bio *bio)
                if (likely(uptodate))
                        goto readpage_ok;

-               if (data_inode) {
+               if (is_data_inode(inode)) {

                        /*
                         * The generic bio_readpage_error handles errors the



So I will fold this into the next version.

Re: [PATCH 6/7] btrfs: Call submit_bio_hook directly for metadata pages

[Nikolay Borisov]

On 21.09.20 г. 23:32 ч., David Sterba wrote:
> On Mon, Sep 21, 2020 at 03:04:54PM +0000, Johannes Thumshirn wrote:
>> On 18/09/2020 15:34, Nikolay Borisov wrote:
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 8cabcb7642a9..4a00cfd4082f 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -172,8 +172,8 @@ int __must_check submit_one_bio(struct bio *bio, int mirror_num,
>>>  		ret = btrfs_submit_data_bio(tree->private_data, bio, mirror_num,
>>>  					    bio_flags);
>>>  	else
>>> -		ret = tree->ops->submit_bio_hook(tree->private_data, bio,
>>> -						 mirror_num, bio_flags);
>>> +		ret = btrfs_submit_metadata_bio(tree->private_data, bio,
>>> +						mirror_num, bio_flags);
>>>  
>>
>>
>> Hmm we could even turn this into a little helper calling either
>> btrfs_submit_data_bio or btrfs_submit_metadata_bio. But that's just stylistic
>> preference I guess.
> 
> Yeah a helper could be here but I think it's fine without that extra
> indirection, the code is clear that for data inode it's some "data"
> function, with the same set of parameters. If there's just one location
> switching the two, the helper would not help much IMHO.
> 

<NOD> The idea here is to have less functions in the way, if there were
more places that this change was necessary I would have gone the helper
way.

Re: [PATCH 7/7] btrfs: Remove struct extent_io_ops

[Nikolay Borisov]

On 21.09.20 г. 23:38 ч., David Sterba wrote:
> On Fri, Sep 18, 2020 at 04:34:39PM +0300, Nikolay Borisov wrote:
> 
> You should really write changelogs for patches that not obviously
> trivial.
> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 


I thought this patch was rather self-explanatory - just removing no
longer used struct and related functions but I guess that's just me
given I have written the previous 6 patches, so will add a changelog in
the next iteration. Thanks.

Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[Nikolay Borisov]

On 21.09.20 г. 20:45 ч., David Sterba wrote:
> On Fri, Sep 18, 2020 at 04:34:33PM +0300, Nikolay Borisov wrote:
>> Instead of relying on indirect calls to implement metadata buffer
>> validation simply check if the inode whose page we are processing equals
>> the btree inode. If it does call the necessary function.
>>
>> This is an improvement in 2 directions:
>> 1. We aren't paying the penalty of indirect calls in a post-speculation
>>    attacks world.
>>
>> 2. The function is now named more explicitly so it's obvious what's
>>    going on
> 
> The new naming is not making things clear, btrfs_check_csum sounds very
> generic while it does a very specific thing just by the number and type
> of the parameters. Similar for btrfs_validate_metadata_buffer.
> 
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
>>  
>>  		mirror = io_bio->mirror_num;
>>  		if (likely(uptodate)) {
>> -			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
>> -							      page, start, end,
>> -							      mirror);
>> +			if (data_inode)
>> +				ret = btrfs_check_csum(io_bio, offset, page,
>> +						       start, end, mirror);
>> +			else
>> +				ret = btrfs_validate_metadata_buffer(io_bio,
>> +					offset, page, start, end, mirror);
> 
> In the context where the functions are used I'd expect some symmetry,
> data/metadata. Something like btrfs_validate_data_bio.
> 

The reason for this naming is that btrfs_vlidate_metadata_buffer
actually validates as in "tree-checker style validation" of the extent
buffer not simply calculating the checksum. So to me it feels like a
more complete,heavyweight operations hence "validating", whlist
btrfs_check_csum just checks the csum of a single sector/blocksize in
the bio. I think the metadata function's name conveys what it's doing in
full:

1. It's doing validation as per aforementioned explanation
2. It's doing it for a whole extent buffer and not just a chunk of it.

I agree that the data function's name is somewhat generic, perhahps it
could be renamed so that it points to the fact it's validating a single
sector/blocksize? I.e btrfs_check_ blocksize_csum or something like that ?

Re: [PATCH 1/7] btrfs: Don't call readpage_end_io_hook for the btree inode

[David Sterba]

On Wed, Sep 23, 2020 at 09:29:00AM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.09.20 г. 20:45 ч., David Sterba wrote:
> > On Fri, Sep 18, 2020 at 04:34:33PM +0300, Nikolay Borisov wrote:
> >> Instead of relying on indirect calls to implement metadata buffer
> >> validation simply check if the inode whose page we are processing equals
> >> the btree inode. If it does call the necessary function.
> >>
> >> This is an improvement in 2 directions:
> >> 1. We aren't paying the penalty of indirect calls in a post-speculation
> >>    attacks world.
> >>
> >> 2. The function is now named more explicitly so it's obvious what's
> >>    going on
> > 
> > The new naming is not making things clear, btrfs_check_csum sounds very
> > generic while it does a very specific thing just by the number and type
> > of the parameters. Similar for btrfs_validate_metadata_buffer.
> > 
> >> --- a/fs/btrfs/extent_io.c
> >> +++ b/fs/btrfs/extent_io.c
> >> @@ -2851,9 +2851,12 @@ static void end_bio_extent_readpage(struct bio *bio)
> >>  
> >>  		mirror = io_bio->mirror_num;
> >>  		if (likely(uptodate)) {
> >> -			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
> >> -							      page, start, end,
> >> -							      mirror);
> >> +			if (data_inode)
> >> +				ret = btrfs_check_csum(io_bio, offset, page,
> >> +						       start, end, mirror);
> >> +			else
> >> +				ret = btrfs_validate_metadata_buffer(io_bio,
> >> +					offset, page, start, end, mirror);
> > 
> > In the context where the functions are used I'd expect some symmetry,
> > data/metadata. Something like btrfs_validate_data_bio.
> > 
> 
> The reason for this naming is that btrfs_vlidate_metadata_buffer
> actually validates as in "tree-checker style validation" of the extent
> buffer not simply calculating the checksum. So to me it feels like a
> more complete,heavyweight operations hence "validating", whlist
> btrfs_check_csum just checks the csum of a single sector/blocksize in
> the bio. I think the metadata function's name conveys what it's doing in
> full:
> 
> 1. It's doing validation as per aforementioned explanation
> 2. It's doing it for a whole extent buffer and not just a chunk of it.

No problem with the metadata function name, I agree with the reasoning
above.

> I agree that the data function's name is somewhat generic, perhahps it
> could be renamed so that it points to the fact it's validating a single
> sector/blocksize? I.e btrfs_check_ blocksize_csum or something like that ?

Yeah, that the data have a simpler validation maybe does not deserve to
be called like that. We should not use 'sector' here as bios use that
too. So btrfs_check_data_block_csum or btrfs_check_block_csum?

Re: [PATCH 4/7] btrfs: Don't opencode is_data_inode in end_bio_extent_readpage

[David Sterba]

On Wed, Sep 23, 2020 at 09:23:48AM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.09.20 г. 23:29 ч., David Sterba wrote:
> > On Fri, Sep 18, 2020 at 04:34:36PM +0300, Nikolay Borisov wrote:
> >> Use the is_data_inode helper.
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>  fs/btrfs/extent_io.c | 3 +--
> >>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> >> index 6e976bd86600..26b002e2f3b3 100644
> >> --- a/fs/btrfs/extent_io.c
> >> +++ b/fs/btrfs/extent_io.c
> >> @@ -2816,8 +2816,7 @@ static void end_bio_extent_readpage(struct bio *bio)
> >>  		struct page *page = bvec->bv_page;
> >>  		struct inode *inode = page->mapping->host;
> >>  		struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> >> -		bool data_inode = btrfs_ino(BTRFS_I(inode))
> >> -			!= BTRFS_BTREE_INODE_OBJECTID;
> >> +		bool data_inode = is_data_inode(inode);
> > 
> > I think you can remove the temporary variable and call is_data_inode
> > directly in the later code, there's only one use.
> > 
> 
> Actually it's used twice, yet I did an experiment to remove it and
> bloat-o-meter indicates it's a win to call the inline function twice:

My oversight, sorry.

> add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-161 (-161)
> Function                                     old     new   delta
> end_bio_extent_readpage.cold                 117     104     -13
> end_bio_extent_readpage                     1614    1466    -148
> Total: Before=45527, After=45366, chg -0.35%

Ok, I'll update it to call the helper.

Re: [PATCH 7/7] btrfs: Remove struct extent_io_ops

[David Sterba]

On Wed, Sep 23, 2020 at 09:25:50AM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.09.20 г. 23:38 ч., David Sterba wrote:
> > On Fri, Sep 18, 2020 at 04:34:39PM +0300, Nikolay Borisov wrote:
> > 
> > You should really write changelogs for patches that not obviously
> > trivial.
> > 
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > 
> 
> 
> I thought this patch was rather self-explanatory - just removing no
> longer used struct and related functions but I guess that's just me
> given I have written the previous 6 patches, so will add a changelog in
> the next iteration. Thanks.

The subject says "remove some struct", but when somebody reads it it's
missing the "..., because ..." part. It becomes clear after reading the
patch that is' not used anymore, but there should be some clue in the
changelog from the beginning.

Re: [PATCH 7/7] btrfs: Remove struct extent_io_ops

[Nikolay Borisov]

On 23.09.20 г. 17:19 ч., David Sterba wrote:
> On Wed, Sep 23, 2020 at 09:25:50AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 21.09.20 г. 23:38 ч., David Sterba wrote:
>>> On Fri, Sep 18, 2020 at 04:34:39PM +0300, Nikolay Borisov wrote:
>>>
>>> You should really write changelogs for patches that not obviously
>>> trivial.
>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>
>>
>>
>> I thought this patch was rather self-explanatory - just removing no
>> longer used struct and related functions but I guess that's just me
>> given I have written the previous 6 patches, so will add a changelog in
>> the next iteration. Thanks.
> 
> The subject says "remove some struct", but when somebody reads it it's
> missing the "..., because ..." part. It becomes clear after reading the
> patch that is' not used anymore, but there should be some clue in the
> changelog from the beginning.
> 

Since you are going to update the patches inline can you add the following:

"
Since it's no longer used just remove the function and any related code
which was initialising it for inodes. No functional changes.
"

Re: [PATCH 7/7] btrfs: Remove struct extent_io_ops

[David Sterba]

On Wed, Sep 23, 2020 at 05:23:05PM +0300, Nikolay Borisov wrote:
> 
> 
> On 23.09.20 г. 17:19 ч., David Sterba wrote:
> > On Wed, Sep 23, 2020 at 09:25:50AM +0300, Nikolay Borisov wrote:
> >>
> >>
> >> On 21.09.20 г. 23:38 ч., David Sterba wrote:
> >>> On Fri, Sep 18, 2020 at 04:34:39PM +0300, Nikolay Borisov wrote:
> >>>
> >>> You should really write changelogs for patches that not obviously
> >>> trivial.
> >>>
> >>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >>>
> >>
> >>
> >> I thought this patch was rather self-explanatory - just removing no
> >> longer used struct and related functions but I guess that's just me
> >> given I have written the previous 6 patches, so will add a changelog in
> >> the next iteration. Thanks.
> > 
> > The subject says "remove some struct", but when somebody reads it it's
> > missing the "..., because ..." part. It becomes clear after reading the
> > patch that is' not used anymore, but there should be some clue in the
> > changelog from the beginning.
> > 
> 
> Since you are going to update the patches inline can you add the following:
> 
> "
> Since it's no longer used just remove the function and any related code
> which was initialising it for inodes. No functional changes.
> "

Will update. There are no direct functional changes, but the
extent_io_tree is shrunk by 8 bytes and there are eg. 3 instances in the
btrfs_inode, so this removes 24 bytes in total.

Re: [PATCH 0/7] Remove struct extent_io_ops

[David Sterba]

On Fri, Sep 18, 2020 at 04:34:32PM +0300, Nikolay Borisov wrote:
> Finally it's time to remove "struct extent_io_ops" and get rid of the indirect
> calls to the "hook" functions. Each patch is rather self-explanatory, the basic
> idea is to replace indirect calls with an "if" construct (patches 1,3,5,6). The
> rest simply remove struct members and the struct it self.
> 
> This series survived a full xfstest run.
> 
> Nikolay Borisov (7):
>   btrfs: Don't call readpage_end_io_hook for the btree inode
>   btrfs: Remove extent_io_ops::readpage_end_io_hook
>   btrfs: Call submit_bio_hook directly in submit_one_bio
>   btrfs: Don't opencode is_data_inode in end_bio_extent_readpage
>   btrfs: Stop calling submit_bio_hook for data inodes
>   btrfs: Call submit_bio_hook directly for metadata pages
>   btrfs: Remove struct extent_io_ops

With the fixups added to misc-next, thanks.