diff mbox series

[v5.3,08/11] btrfs: extent_io: Kill the BUG_ON() in lock_extent_buffer_for_io()

Message ID 20190320062749.30953-9-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: tree-checker: Write time tree checker | expand

Commit Message

Qu Wenruo March 20, 2019, 6:27 a.m. UTC 
This function needs some extra check on locked pages and eb.

For error handling we need to unlock locked pages and the eb.

Also add comment for possible return values of lock_extent_buffer_for_io().

There is a rare >0 return value branch, where all pages get locked
while write bio is not flushed.

Thankfully it's handled by the only caller, btree_write_cache_pages(),
as later write_one_eb() call will trigger submit_one_bio().
So there shouldn't be any problem.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

David Sterba March 21, 2019, 1:30 p.m. UTC | #1 
On Wed, Mar 20, 2019 at 02:27:46PM +0800, Qu Wenruo wrote:
> This function needs some extra check on locked pages and eb.
> 
> For error handling we need to unlock locked pages and the eb.
> 
> Also add comment for possible return values of lock_extent_buffer_for_io().
> 
> There is a rare >0 return value branch, where all pages get locked
> while write bio is not flushed.
> 
> Thankfully it's handled by the only caller, btree_write_cache_pages(),
> as later write_one_eb() call will trigger submit_one_bio().
> So there shouldn't be any problem.

Right, I'd suggest to add a comment to btree_write_cache_pages
explaining that and possibly changing the condition to make it more
explicit that all 3 types of values are really handled.
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c688049ccfc3..5de18900a3c3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3527,19 +3527,25 @@  void wait_on_extent_buffer_writeback(struct extent_buffer *eb)
 		       TASK_UNINTERRUPTIBLE);
 }
 
+/*
+ * Return 0 if nothing went wrong, its pages get locked and submitted.
+ * Return >0 is mostly the same as 0, except bio is not submitted.
+ * Return <0 if something went wrong, no page get locked.
+ */
 static noinline_for_stack int
 lock_extent_buffer_for_io(struct extent_buffer *eb,
 			  struct btrfs_fs_info *fs_info,
 			  struct extent_page_data *epd)
 {
-	int i, num_pages;
+	int i, num_pages, failed_page_nr;
 	int flush = 0;
 	int ret = 0;
 
 	if (!btrfs_try_tree_write_lock(eb)) {
-		flush = 1;
 		ret = flush_write_bio(epd);
-		BUG_ON(ret < 0);
+		if (ret < 0)
+			return ret;
+		flush = 1;
 		btrfs_tree_lock(eb);
 	}
 
@@ -3549,7 +3555,8 @@  lock_extent_buffer_for_io(struct extent_buffer *eb,
 			return 0;
 		if (!flush) {
 			ret = flush_write_bio(epd);
-			BUG_ON(ret < 0);
+			if (ret < 0)
+				return ret;
 			flush = 1;
 		}
 		while (1) {
@@ -3591,7 +3598,10 @@  lock_extent_buffer_for_io(struct extent_buffer *eb,
 		if (!trylock_page(p)) {
 			if (!flush) {
 				ret = flush_write_bio(epd);
-				BUG_ON(ret < 0);
+				if (ret < 0) {
+					failed_page_nr = i;
+					goto err_unlock;
+				}
 				flush = 1;
 			}
 			lock_page(p);
@@ -3599,6 +3609,11 @@  lock_extent_buffer_for_io(struct extent_buffer *eb,
 	}
 
 	return ret;
+err_unlock:
+	/* Unlock these already locked pages */
+	for (i = 0; i < failed_page_nr; i++)
+		unlock_page(eb->pages[i]);
+	return ret;
 }
 
 static void end_extent_buffer_writeback(struct extent_buffer *eb)