* [PATCH] Fix balance Oops
@ 2009-08-07 6:14 Yan Zheng
2009-08-07 6:50 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Yan Zheng @ 2009-08-07 6:14 UTC (permalink / raw)
To: linux-btrfs, Chris Mason
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 <zheng.yan@oracle.com>
---
diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
--- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
+++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
@@ -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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix balance Oops
2009-08-07 6:14 [PATCH] Fix balance Oops Yan Zheng
@ 2009-08-07 6:50 ` Jens Axboe
2009-08-07 7:16 ` Yan Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2009-08-07 6:50 UTC (permalink / raw)
To: Yan Zheng; +Cc: linux-btrfs, Chris Mason
On Fri, Aug 07 2009, Yan Zheng wrote:
> 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 <zheng.yan@oracle.com>
>
> ---
> diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
> --- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
> +++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
> @@ -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 it returns EBUSY, would it not make more sense to call
filemap_write_and_wait_range() instead of hammering on invalidate?
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix balance Oops
2009-08-07 6:50 ` Jens Axboe
@ 2009-08-07 7:16 ` Yan Zheng
2009-08-07 7:19 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Yan Zheng @ 2009-08-07 7:16 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-btrfs, Chris Mason
On 08/07/2009 02:50 PM, Jens Axboe wrote:
> On Fri, Aug 07 2009, Yan Zheng wrote:
>> 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 <zheng.yan@oracle.com>
>>
>> ---
>> diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
>> --- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
>> +++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
>> @@ -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 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix balance Oops
2009-08-07 7:16 ` Yan Zheng
@ 2009-08-07 7:19 ` Jens Axboe
2009-08-07 9:07 ` Yan Zheng
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2009-08-07 7:19 UTC (permalink / raw)
To: Yan Zheng; +Cc: linux-btrfs, Chris Mason
On Fri, Aug 07 2009, Yan Zheng wrote:
> On 08/07/2009 02:50 PM, Jens Axboe wrote:
> > On Fri, Aug 07 2009, Yan Zheng wrote:
> >> 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 <zheng.yan@oracle.com>
> >>
> >> ---
> >> diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
> >> --- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
> >> +++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
> >> @@ -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 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)
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix balance Oops
2009-08-07 7:19 ` Jens Axboe
@ 2009-08-07 9:07 ` Yan Zheng
2009-08-07 12:51 ` Chris Mason
0 siblings, 1 reply; 6+ messages in thread
From: Yan Zheng @ 2009-08-07 9:07 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-btrfs, Chris Mason
On 08/07/2009 03:19 PM, Jens Axboe wrote:
> On Fri, Aug 07 2009, Yan Zheng wrote:
>> On 08/07/2009 02:50 PM, Jens Axboe wrote:
>>> On Fri, Aug 07 2009, Yan Zheng wrote:
>>>> 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 <zheng.yan@oracle.com>
>>>>
>>>> ---
>>>> diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
>>>> --- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
>>>> +++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
>>>> @@ -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 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)
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix balance Oops
2009-08-07 9:07 ` Yan Zheng
@ 2009-08-07 12:51 ` Chris Mason
0 siblings, 0 replies; 6+ messages in thread
From: Chris Mason @ 2009-08-07 12:51 UTC (permalink / raw)
To: Yan Zheng; +Cc: Jens Axboe, linux-btrfs
On Fri, Aug 07, 2009 at 05:07:32PM +0800, Yan Zheng wrote:
> On 08/07/2009 03:19 PM, Jens Axboe wrote:
> > On Fri, Aug 07 2009, Yan Zheng wrote:
> >> On 08/07/2009 02:50 PM, Jens Axboe wrote:
> >>> On Fri, Aug 07 2009, Yan Zheng wrote:
> >>>> 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 <zheng.yan@oracle.com>
> >>>>
> >>>> ---
> >>>> diff -urp 1/fs/btrfs/relocation.c 2/fs/btrfs/relocation.c
> >>>> --- 1/fs/btrfs/relocation.c 2009-07-29 10:03:04.367858774 +0800
> >>>> +++ 2/fs/btrfs/relocation.c 2009-08-07 13:26:43.882147138 +0800
> >>>> @@ -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 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)
> >
> > 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)
Ok, please use schedule_timeout(HZ/10) instead then.
-chris
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-08-07 12:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-07 6:14 [PATCH] Fix balance Oops Yan Zheng
2009-08-07 6:50 ` Jens Axboe
2009-08-07 7:16 ` Yan Zheng
2009-08-07 7:19 ` Jens Axboe
2009-08-07 9:07 ` Yan Zheng
2009-08-07 12:51 ` Chris Mason
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox