* [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