Skip to contentSkip to conversation

Cleanups in extent_io callbacks

PATCH In review David Sterba 2 participants 7 messages Feb 20, 2017 – Mar 16, 2017 Btrfs file system

lore
Share & tools
lore

David Sterba

author maintainer
#0 lore raw

cover Cleanups in extent_io callbacks

Some of the checks for extent_io callbacks can be safely dropped as they're always defined, plus some dummy callback additions so more checks can be dropped. There's more potential for the same cleanup in other callbacks but this would need more evaluation wheather dummy callbacks vs existence checks are really worth it.

David Sterba (4):
btrfs: let writepage_end_io_hook return void
btrfs: document existence of extent_io ops callbacks
btrfs: drop checks for mandatory extent_io_ops callbacks
btrfs: add dummy callback for readpage_io_failed and drop checks

 fs/btrfs/disk-io.c   |  7 +++++--
 fs/btrfs/extent_io.c | 18 +++++++-----------
 fs/btrfs/extent_io.h | 25 +++++++++++++++++--------
 fs/btrfs/inode.c     | 20 ++++++++++++++------
 4 files changed, 43 insertions(+), 27 deletions(-)
author maintainer
#1 lore raw

PATCH 1/4 btrfs: let writepage_end_io_hook return void

There's no error path in any of the instances, always return 0.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 9 +++------
 fs/btrfs/extent_io.h | 2 +-
 fs/btrfs/inode.c     | 6 ++----
 3 files changed, 6 insertions(+), 11 deletions(-)
fs/btrfs/extent_io.c +3 -6
index d15b5ddb6732..8de29aa4d1a2 100644
@@ -2435,12 +2435,9 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
 
 	tree = &BTRFS_I(page->mapping->host)->io_tree;
 
-	if (tree->ops && tree->ops->writepage_end_io_hook) {
-		ret = tree->ops->writepage_end_io_hook(page, start,
-					       end, NULL, uptodate);
-		if (ret)
-			uptodate = 0;
-	}
+	if (tree->ops && tree->ops->writepage_end_io_hook)
+		tree->ops->writepage_end_io_hook(page, start, end, NULL,
+				uptodate);
 
 	if (!uptodate) {
 		ClearPageUptodate(page);
fs/btrfs/extent_io.h +1 -1
index 270d03be290e..fbc92315b503 100644
@@ -103,7 +103,7 @@ 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 (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
+	void (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
 				      struct extent_state *state, int uptodate);
 	void (*set_bit_hook)(struct inode *inode, struct extent_state *state,
 			     unsigned *bits);
fs/btrfs/inode.c +2 -4
index dae2734a725b..eafadf0851d1 100644
@@ -2977,7 +2977,7 @@ static void finish_ordered_fn(struct btrfs_work *work)
 	btrfs_finish_ordered_io(ordered_extent);
 }
 
-static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
+static void btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 				struct extent_state *state, int uptodate)
 {
 	struct inode *inode = page->mapping->host;

@@ -2991,7 +2991,7 @@ static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 	ClearPagePrivate2(page);
 	if (!btrfs_dec_test_ordered_pending(inode, &ordered_extent, start,
 					    end - start + 1, uptodate))
-		return 0;
+		return;
 
 	if (btrfs_is_free_space_inode(inode)) {
 		wq = fs_info->endio_freespace_worker;

@@ -3004,8 +3004,6 @@ static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 	btrfs_init_work(&ordered_extent->work, func, finish_ordered_fn, NULL,
 			NULL);
 	btrfs_queue_work(wq, &ordered_extent->work);
-
-	return 0;
 }
 
 static int __readpage_endio_check(struct inode *inode,
Signature · 1 line
2.10.1
Reviewed-by on v1 1/4
#2 lore raw

Re: [PATCH 1/4] btrfs: let writepage_end_io_hook return void

Quoted 1 line · David Sterba

There's no error path in any of the instances, always return 0.

Reviewed-by Liu Bo <[email protected]>

Thanks,

-liubo

Quoted 79 lines
Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 9 +++------
 fs/btrfs/extent_io.h | 2 +-
 fs/btrfs/inode.c     | 6 ++----
 3 files changed, 6 insertions(+), 11 deletions(-)
fs/btrfs/extent_io.c +3 -6
index d15b5ddb6732..8de29aa4d1a2 100644
@@ -2435,12 +2435,9 @@ void end_extent_writepage(struct page *page, int err, u64 start, u64 end)
 
 	tree = &BTRFS_I(page->mapping->host)->io_tree;
 
-	if (tree->ops && tree->ops->writepage_end_io_hook) {
-		ret = tree->ops->writepage_end_io_hook(page, start,
-					       end, NULL, uptodate);
-		if (ret)
-			uptodate = 0;
-	}
+	if (tree->ops && tree->ops->writepage_end_io_hook)
+		tree->ops->writepage_end_io_hook(page, start, end, NULL,
+				uptodate);
 
 	if (!uptodate) {
 		ClearPageUptodate(page);
fs/btrfs/extent_io.h +1 -1
index 270d03be290e..fbc92315b503 100644
@@ -103,7 +103,7 @@ 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 (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
+	void (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
 				      struct extent_state *state, int uptodate);
 	void (*set_bit_hook)(struct inode *inode, struct extent_state *state,
 			     unsigned *bits);
fs/btrfs/inode.c +2 -4
index dae2734a725b..eafadf0851d1 100644
@@ -2977,7 +2977,7 @@ static void finish_ordered_fn(struct btrfs_work *work)
 	btrfs_finish_ordered_io(ordered_extent);
 }
 
-static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
+static void btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 				struct extent_state *state, int uptodate)
 {
 	struct inode *inode = page->mapping->host;

@@ -2991,7 +2991,7 @@ static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 	ClearPagePrivate2(page);
 	if (!btrfs_dec_test_ordered_pending(inode, &ordered_extent, start,
 					    end - start + 1, uptodate))
-		return 0;
+		return;
 
 	if (btrfs_is_free_space_inode(inode)) {
 		wq = fs_info->endio_freespace_worker;

@@ -3004,8 +3004,6 @@ static int btrfs_writepage_end_io_hook(struct page *page, u64 start, u64 end,
 	btrfs_init_work(&ordered_extent->work, func, finish_ordered_fn, NULL,
 			NULL);
 	btrfs_queue_work(wq, &ordered_extent->work);
-
-	return 0;
 }
 
 static int __readpage_endio_check(struct inode *inode,
Signature · 6 lines
2.10.1

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

PATCH 2/4 btrfs: document existence of extent_io ops callbacks

In reply to David Sterba

Some of the callbacks defined in btree_extent_io_ops and btrfs_extent_io_ops do always exist so we don't need to check the existence before each call. This patch just reorders the definition and documents which are mandatory/optional.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   |  7 +++++--
 fs/btrfs/extent_io.h | 23 ++++++++++++++++-------
 fs/btrfs/inode.c     |  7 +++++--
 3 files changed, 26 insertions(+), 11 deletions(-)
fs/btrfs/disk-io.c +5 -2
index 2b06f557c176..0715b6f3f686 100644
@@ -4653,9 +4653,12 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
 }
 
 static const struct extent_io_ops btree_extent_io_ops = {
-	.readpage_end_io_hook = btree_readpage_end_io_hook,
-	.readpage_io_failed_hook = btree_io_failed_hook,
+	/* 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,
+
+	/* optional callbacks */
+	.readpage_io_failed_hook = btree_io_failed_hook,
 };
fs/btrfs/extent_io.h +16 -7
index fbc92315b503..5c5e2e6cfb9e 100644
@@ -91,18 +91,27 @@ typedef	int (extent_submit_bio_hook_t)(struct inode *inode, struct bio *bio,
 				       int mirror_num, unsigned long bio_flags,
 				       u64 bio_offset);
 struct extent_io_ops {
-	int (*fill_delalloc)(struct inode *inode, struct page *locked_page,
-			     u64 start, u64 end, int *page_started,
-			     unsigned long *nr_written);
-	int (*writepage_start_hook)(struct page *page, u64 start, u64 end);
+	/*
+	 * The following callbacks must be allways defined, the function
+	 * pointer will be called unconditionally.
+	 */
 	extent_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);
 	int (*merge_bio_hook)(struct page *page, unsigned long offset,
 			      size_t size, struct bio *bio,
 			      unsigned long bio_flags);
+
+	/*
+	 * Optional hooks, called if the pointer is not NULL
+	 */
+	int (*fill_delalloc)(struct inode *inode, struct page *locked_page,
+			     u64 start, u64 end, int *page_started,
+			     unsigned long *nr_written);
 	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
-	int (*readpage_end_io_hook)(struct btrfs_io_bio *io_bio, u64 phy_offset,
-				    struct page *page, u64 start, u64 end,
-				    int mirror);
+
+	int (*writepage_start_hook)(struct page *page, u64 start, u64 end);
 	void (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
 				      struct extent_state *state, int uptodate);
 	void (*set_bit_hook)(struct inode *inode, struct extent_state *state,
fs/btrfs/inode.c +5 -2
index eafadf0851d1..72faf9b5616a 100644
@@ -10541,10 +10541,13 @@ static const struct file_operations btrfs_dir_file_operations = {
 };
 
 static const struct extent_io_ops btrfs_extent_io_ops = {
-	.fill_delalloc = run_delalloc_range,
+	/* mandatory callbacks */
 	.submit_bio_hook = btrfs_submit_bio_hook,
-	.merge_bio_hook = btrfs_merge_bio_hook,
 	.readpage_end_io_hook = btrfs_readpage_end_io_hook,
+	.merge_bio_hook = btrfs_merge_bio_hook,
+
+	/* optional callbacks */
+	.fill_delalloc = run_delalloc_range,
 	.writepage_end_io_hook = btrfs_writepage_end_io_hook,
 	.writepage_start_hook = btrfs_writepage_start_hook,
 	.set_bit_hook = btrfs_set_bit_hook,
Signature · 1 line
2.10.1
author maintainer
#4 lore raw

PATCH 3/4 btrfs: drop checks for mandatory extent_io_ops callbacks

In reply to David Sterba

We know that eadpage_end_io_hook, submit_bio_hook and merge_bio_hook are always defined so we can drop the checks before we call them.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/extent_io.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
fs/btrfs/extent_io.c +3 -4
index 8de29aa4d1a2..f5cff93ab152 100644
@@ -2565,8 +2565,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 		len = bvec->bv_len;
 
 		mirror = io_bio->mirror_num;
-		if (likely(uptodate && tree->ops &&
-			   tree->ops->readpage_end_io_hook)) {
+		if (likely(uptodate && tree->ops)) {
 			ret = tree->ops->readpage_end_io_hook(io_bio, offset,
 							      page, start, end,
 							      mirror);

@@ -2728,7 +2727,7 @@ static int __must_check submit_one_bio(struct bio *bio, int mirror_num,
 	bio->bi_private = NULL;
 	bio_get(bio);
 
-	if (tree->ops && tree->ops->submit_bio_hook)
+	if (tree->ops)
 		ret = tree->ops->submit_bio_hook(page->mapping->host, bio,
 					   mirror_num, bio_flags, start);
 	else

@@ -2743,7 +2742,7 @@ static int merge_bio(struct extent_io_tree *tree, struct page *page,
 		     unsigned long bio_flags)
 {
 	int ret = 0;
-	if (tree->ops && tree->ops->merge_bio_hook)
+	if (tree->ops)
 		ret = tree->ops->merge_bio_hook(page, offset, size, bio,
 						bio_flags);
 	return ret;
Signature · 1 line
2.10.1
author maintainer
#5 lore raw

PATCH 4/4 btrfs: add dummy callback for readpage_io_failed and drop checks

In reply to David Sterba

Make extent_io_ops::readpage_io_failed_hook callback mandatory and define a dummy function for btrfs_extent_io_ops. As the failed IO callback is not performance critical, the branch vs extra trade off does not hurt.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   | 2 +-
 fs/btrfs/extent_io.c | 2 +-
 fs/btrfs/extent_io.h | 2 +-
 fs/btrfs/inode.c     | 7 +++++++
 4 files changed, 10 insertions(+), 3 deletions(-)
fs/btrfs/disk-io.c +1 -1
index 0715b6f3f686..fbf4921f4d60 100644
@@ -4658,7 +4658,7 @@ static const struct extent_io_ops btree_extent_io_ops = {
 	.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,
 
 	/* optional callbacks */
-	.readpage_io_failed_hook = btree_io_failed_hook,
 };
fs/btrfs/extent_io.c +1 -1
index f5cff93ab152..eaee7bb2ff7c 100644
@@ -2578,7 +2578,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 		if (likely(uptodate))
 			goto readpage_ok;
 
-		if (tree->ops && tree->ops->readpage_io_failed_hook) {
+		if (tree->ops) {
 			ret = tree->ops->readpage_io_failed_hook(page, mirror);
 			if (!ret && !bio->bi_error)
 				uptodate = 1;
fs/btrfs/extent_io.h +1 -1
index 5c5e2e6cfb9e..63c8cc970b1c 100644
@@ -102,6 +102,7 @@ struct extent_io_ops {
 	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);
 
 	/*
 	 * Optional hooks, called if the pointer is not NULL

@@ -109,7 +110,6 @@ struct extent_io_ops {
 	int (*fill_delalloc)(struct inode *inode, struct page *locked_page,
 			     u64 start, u64 end, int *page_started,
 			     unsigned long *nr_written);
-	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
 
 	int (*writepage_start_hook)(struct page *page, u64 start, u64 end);
 	void (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
fs/btrfs/inode.c +7 -0
index 72faf9b5616a..a74191fa3934 100644
@@ -10503,6 +10503,12 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 
 }
 
+__attribute__((const))
+static int dummy_readpage_io_failed_hook(struct page *page, int failed_mirror)
+{
+	return 0;
+}
+
 static const struct inode_operations btrfs_dir_inode_operations = {
 	.getattr	= btrfs_getattr,
 	.lookup		= btrfs_lookup,

@@ -10545,6 +10551,7 @@ 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,
 	.merge_bio_hook = btrfs_merge_bio_hook,
+	.readpage_io_failed_hook = dummy_readpage_io_failed_hook,
 
 	/* optional callbacks */
 	.fill_delalloc = run_delalloc_range,
Signature · 1 line
2.10.1
#6 lore raw

Re: [PATCH 4/4] btrfs: add dummy callback for readpage_io_failed and drop checks

Quoted 77 lines · David Sterba

Make extent_io_ops::readpage_io_failed_hook callback mandatory and define a dummy function for btrfs_extent_io_ops. As the failed IO callback is not performance critical, the branch vs extra trade off does not hurt.

Signed-off-by David Sterba <[email protected]>
---
 fs/btrfs/disk-io.c   | 2 +-
 fs/btrfs/extent_io.c | 2 +-
 fs/btrfs/extent_io.h | 2 +-
 fs/btrfs/inode.c     | 7 +++++++
 4 files changed, 10 insertions(+), 3 deletions(-)
fs/btrfs/disk-io.c +1 -1
index 0715b6f3f686..fbf4921f4d60 100644
@@ -4658,7 +4658,7 @@ static const struct extent_io_ops btree_extent_io_ops = {
 	.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,
 
 	/* optional callbacks */
-	.readpage_io_failed_hook = btree_io_failed_hook,
 };
fs/btrfs/extent_io.c +1 -1
index f5cff93ab152..eaee7bb2ff7c 100644
@@ -2578,7 +2578,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 		if (likely(uptodate))
 			goto readpage_ok;
 
-		if (tree->ops && tree->ops->readpage_io_failed_hook) {
+		if (tree->ops) {
 			ret = tree->ops->readpage_io_failed_hook(page, mirror);
 			if (!ret && !bio->bi_error)
 				uptodate = 1;
fs/btrfs/extent_io.h +1 -1
index 5c5e2e6cfb9e..63c8cc970b1c 100644
@@ -102,6 +102,7 @@ struct extent_io_ops {
 	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);
 
 	/*
 	 * Optional hooks, called if the pointer is not NULL

@@ -109,7 +110,6 @@ struct extent_io_ops {
 	int (*fill_delalloc)(struct inode *inode, struct page *locked_page,
 			     u64 start, u64 end, int *page_started,
 			     unsigned long *nr_written);
-	int (*readpage_io_failed_hook)(struct page *page, int failed_mirror);
 
 	int (*writepage_start_hook)(struct page *page, u64 start, u64 end);
 	void (*writepage_end_io_hook)(struct page *page, u64 start, u64 end,
fs/btrfs/inode.c +6 -0
index 72faf9b5616a..a74191fa3934 100644
@@ -10503,6 +10503,12 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 
 }
 
+__attribute__((const))
+static int dummy_readpage_io_failed_hook(struct page *page, int failed_mirror)
+{
+	return 0;
+}
+
 static const struct inode_operations btrfs_dir_inode_operations = {
 	.getattr	= btrfs_getattr,
 	.lookup		= btrfs_lookup,

fs/btrfs/inode.c static const struct extent_io_ops btrfs_extent_io_ops = { · line 10554

Quoted 4 lines
fs/btrfs/inode.c +1 -0
@@ -10545,6 +10551,7 @@ 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,
 	.merge_bio_hook = btrfs_merge_bio_hook,
+	.readpage_io_failed_hook = dummy_readpage_io_failed_hook,

This has made us not call bio_readpage_error() to correct corrupted data...

Thanks,

-liubo

Quoted 10 lines
fs/btrfs/inode.c +0 -0
 	/* optional callbacks */
 	.fill_delalloc = run_delalloc_range,
Signature · 6 lines
2.10.1

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