Fix balance Oops

PATCH In review Yan Zheng 3 participants 6 messages Aug 7, 2009 Btrfs file system

Share & tools

Yan Zheng

author

2009-08-07 06:14 UTC #0 lore raw

PATCH 1/1 Fix balance Oops

invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.

Signed-off-by Yan Zheng <[email protected]>

---

fs/btrfs/relocation.c +7 -2

diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 
 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}
 	if (ret)
 		goto out_unlock;

Jens Axboe

2009-08-07 06:50 UTC #1 lore raw

Quoted 14 lines · Yan Zheng

invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.
Signed-off-by Yan Zheng <[email protected]>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
fs/btrfs/relocation.c +0 -0
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 

fs/btrfs/relocation.c int relocate_inode_pages(struct inode *i · line 2562

Quoted 10 lines

fs/btrfs/relocation.c +7 -2

 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}

If it returns EBUSY, would it not make more sense to call filemap_write_and_wait_range() instead of hammering on invalidate?

Signature · 1 line

Jens Axboe

Yan Zheng

author

2009-08-07 07:16 UTC #2 lore raw

Quoted 15 lines · Jens Axboe

Quoted 14 lines · Yan Zheng
invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.
Signed-off-by Yan Zheng <[email protected]>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
fs/btrfs/relocation.c +0 -0
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 

Quoted 14 lines

fs/btrfs/relocation.c int relocate_inode_pages(struct inode *i · line 2562
Quoted 10 lines
fs/btrfs/relocation.c +7 -2
 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}
If it returns EBUSY, would it not make more sense to call filemap_write_and_wait_range() instead of hammering on invalidate?

The pages to invalidate are not dirty, they are from page read-ahead. Actually I have no idea how invalidate_inode_pages2_range can return -EBUSY here. (the only user of the inode is the balancer, and it does not hold references to the pages)

Regards
Yan, Zheng

Jens Axboe

2009-08-07 07:19 UTC #3 lore raw

Quoted 31 lines · Yan Zheng

Quoted 15 lines · Jens Axboe
Quoted 14 lines · Yan Zheng
invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.
Signed-off-by Yan Zheng <[email protected]>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
fs/btrfs/relocation.c +0 -0
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 
Quoted 14 lines
fs/btrfs/relocation.c int relocate_inode_pages(struct inode *i · line 2562
Quoted 10 lines
fs/btrfs/relocation.c +7 -2
 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}
If it returns EBUSY, would it not make more sense to call filemap_write_and_wait_range() instead of hammering on invalidate?

Quoted 4 lines

The pages to invalidate are not dirty, they are from page read-ahead. Actually I have no idea how invalidate_inode_pages2_range can return -EBUSY here. (the only user of the inode is the balancer, and it does not hold references to the pages)

Weird, I looked it up, and it already does a writeback wait. But I guess that's not your issue. Patch still looks like a hack though, it would be far better to figure out why it returns EBUSY and fix/wait appropriately for that to pass.

Signature · 1 line

Jens Axboe

Yan Zheng

author

2009-08-07 09:07 UTC #4 lore raw

Quoted 35 lines · Jens Axboe

Quoted 29 lines · Yan Zheng
Quoted 15 lines · Jens Axboe
Quoted 14 lines · Yan Zheng
invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.
Signed-off-by Yan Zheng <[email protected]>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
fs/btrfs/relocation.c +0 -0
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 
Quoted 13 lines
fs/btrfs/relocation.c int relocate_inode_pages(struct inode *i · line 2562
Quoted 10 lines
fs/btrfs/relocation.c +7 -2
 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}
If it returns EBUSY, would it not make more sense to call filemap_write_and_wait_range() instead of hammering on invalidate?
Quoted 4 lines
The pages to invalidate are not dirty, they are from page read-ahead. Actually I have no idea how invalidate_inode_pages2_range can return -EBUSY here. (the only user of the inode is the balancer, and it does not hold references to the pages)

Quoted 5 lines

Weird, I looked it up, and it already does a writeback wait. But I guess that's not your issue. Patch still looks like a hack though, it would be far better to figure out why it returns EBUSY and fix/wait appropriately for that to pass.

EBUSY is from the EXTENT_LOCK test in try_release_extent_state. The test can be true is because some codes call lock_extent while corresponding pages are not all locked. (one example is btrfs_finish_ordered_io)

Yan, Zheng

Chris Mason

2009-08-07 12:51 UTC #5 lore raw

Quoted 42 lines · Yan Zheng

Quoted 35 lines · Jens Axboe
Quoted 29 lines · Yan Zheng
Quoted 15 lines · Jens Axboe
Quoted 14 lines · Yan Zheng
invalidate_inode_pages2_range may return -EBUSY occasionally which results Oops. This patch fixes the issue by moving invalidate_inode_pages2_range into a loop and keeping calling it until the return value is not -EBUSY.
Signed-off-by Yan Zheng <[email protected]>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
fs/btrfs/relocation.c +0 -0
@@ -2553,8 +2553,13 @@ int relocate_inode_pages(struct inode *i
 	last_index = (start + len - 1) >> PAGE_CACHE_SHIFT;
 
Quoted 13 lines
fs/btrfs/relocation.c int relocate_inode_pages(struct inode *i · line 2562
Quoted 10 lines
fs/btrfs/relocation.c +7 -2
 	/* make sure the dirty trick played by the caller work */
-	ret = invalidate_inode_pages2_range(inode->i_mapping,
-					    first_index, last_index);
+	while (1) {
+		ret = invalidate_inode_pages2_range(inode->i_mapping,
+						    first_index, last_index);
+		if (ret != -EBUSY)
+			break;
+		cond_resched();
+	}
If it returns EBUSY, would it not make more sense to call filemap_write_and_wait_range() instead of hammering on invalidate?
Quoted 4 lines
The pages to invalidate are not dirty, they are from page read-ahead. Actually I have no idea how invalidate_inode_pages2_range can return -EBUSY here. (the only user of the inode is the balancer, and it does not hold references to the pages)
Quoted 5 lines
Weird, I looked it up, and it already does a writeback wait. But I guess that's not your issue. Patch still looks like a hack though, it would be far better to figure out why it returns EBUSY and fix/wait appropriately for that to pass.

Quoted 3 lines

EBUSY is from the EXTENT_LOCK test in try_release_extent_state. The test can be true is because some codes call lock_extent while corresponding pages are not all locked. (one example is btrfs_finish_ordered_io)

Ok, please use schedule_timeout(HZ/10) instead then.

-chris