* ext4_da_block_invalidatepages() question
@ 2010-01-26 13:36 Wu Fengguang
2010-01-26 15:32 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Wu Fengguang @ 2010-01-26 13:36 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4@vger.kernel.org
Hi Jan,
I noticed that ext4_da_block_invalidatepages() does pagevec_lookup()
without pagevec_release()/put_page(). Is that OK?
Thanks,
Fengguang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ext4_da_block_invalidatepages() question
2010-01-26 13:36 ext4_da_block_invalidatepages() question Wu Fengguang
@ 2010-01-26 15:32 ` Jan Kara
2010-01-27 1:53 ` Wu Fengguang
2010-02-09 15:39 ` Jan Kara
0 siblings, 2 replies; 6+ messages in thread
From: Jan Kara @ 2010-01-26 15:32 UTC (permalink / raw)
To: Wu Fengguang; +Cc: Jan Kara, linux-ext4@vger.kernel.org, tytso
[-- Attachment #1: Type: text/plain, Size: 497 bytes --]
Hi,
On Tue 26-01-10 21:36:08, Wu Fengguang wrote:
> I noticed that ext4_da_block_invalidatepages() does pagevec_lookup()
> without pagevec_release()/put_page(). Is that OK?
Yes, the function looks buggy. Luckily, it is called only in case we are
not able to allocate space for delay-allocated data which is a bug on its
own. So people should never hit it.
Attached patch should fix the issue. Ted, will you merge it please?
Thanks.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
[-- Attachment #2: 0001-ext4-Release-page-references-acquired-in-ext4_da_blo.patch --]
[-- Type: text/x-patch, Size: 1081 bytes --]
>From 47085f1ac03eaca9e4d7a5f8f1e40e87d3879512 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 26 Jan 2010 16:15:19 +0100
Subject: [PATCH] ext4: Release page references acquired in ext4_da_block_invalidatepages
We forget to release page references we acquire in
ext4_da_block_invalidatepages. Luckily, this function gets called only if we
are not able to allocate blocks for delay-allocated data so that function
should better never be called.
Also cleanup handling of index variable.
Reported-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/inode.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c818972..1680007 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2127,17 +2127,16 @@ static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd,
break;
for (i = 0; i < nr_pages; i++) {
struct page *page = pvec.pages[i];
- index = page->index;
- if (index > end)
+ if (page->index > end)
break;
- index++;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: ext4_da_block_invalidatepages() question
2010-01-26 15:32 ` Jan Kara
@ 2010-01-27 1:53 ` Wu Fengguang
2010-01-27 12:25 ` Jan Kara
2010-02-09 15:39 ` Jan Kara
1 sibling, 1 reply; 6+ messages in thread
From: Wu Fengguang @ 2010-01-27 1:53 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4@vger.kernel.org, tytso@mit.edu
On Tue, Jan 26, 2010 at 08:32:22AM -0700, Jan Kara wrote:
> @@ -2127,17 +2127,16 @@ static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd,
> break;
> for (i = 0; i < nr_pages; i++) {
> struct page *page = pvec.pages[i];
> - index = page->index;
> - if (index > end)
> + if (page->index > end)
> break;
> - index++;
> -
> BUG_ON(!PageLocked(page));
> BUG_ON(PageWriteback(page));
> block_invalidatepage(page, 0);
> ClearPageUptodate(page);
> unlock_page(page);
> }
> + index = pvec.pages[nr_pages - 1]->index + 1;
> + pagevec_release(&pvec);
> }
> return;
> }
The patch includes a cleanup and a bug fix, both looks OK to me.
But if we can split it, the bug fix would be good candidate for
the stable kernel?
Thanks,
Fengguang
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: ext4_da_block_invalidatepages() question
2010-01-27 1:53 ` Wu Fengguang
@ 2010-01-27 12:25 ` Jan Kara
2010-01-27 12:34 ` Wu Fengguang
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2010-01-27 12:25 UTC (permalink / raw)
To: Wu Fengguang; +Cc: Jan Kara, linux-ext4@vger.kernel.org, tytso@mit.edu
On Wed 27-01-10 09:53:39, Wu Fengguang wrote:
> On Tue, Jan 26, 2010 at 08:32:22AM -0700, Jan Kara wrote:
>
> > @@ -2127,17 +2127,16 @@ static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd,
> > break;
> > for (i = 0; i < nr_pages; i++) {
> > struct page *page = pvec.pages[i];
> > - index = page->index;
> > - if (index > end)
> > + if (page->index > end)
> > break;
> > - index++;
> > -
> > BUG_ON(!PageLocked(page));
> > BUG_ON(PageWriteback(page));
> > block_invalidatepage(page, 0);
> > ClearPageUptodate(page);
> > unlock_page(page);
> > }
> > + index = pvec.pages[nr_pages - 1]->index + 1;
> > + pagevec_release(&pvec);
> > }
> > return;
> > }
>
> The patch includes a cleanup and a bug fix, both looks OK to me.
> But if we can split it, the bug fix would be good candidate for
> the stable kernel?
I don't think we want to push this to -stable kernel. As I wrote, this
code is called only in case user is going to loose written data which is a
bug on it's own so loosing a few page references is nothing compared to
that.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: ext4_da_block_invalidatepages() question
2010-01-27 12:25 ` Jan Kara
@ 2010-01-27 12:34 ` Wu Fengguang
0 siblings, 0 replies; 6+ messages in thread
From: Wu Fengguang @ 2010-01-27 12:34 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-ext4@vger.kernel.org, tytso@mit.edu
On Wed, Jan 27, 2010 at 04:25:28AM -0800, Jan Kara wrote:
> On Wed 27-01-10 09:53:39, Wu Fengguang wrote:
> > The patch includes a cleanup and a bug fix, both looks OK to me.
> > But if we can split it, the bug fix would be good candidate for
> > the stable kernel?
> I don't think we want to push this to -stable kernel. As I wrote, this
> code is called only in case user is going to loose written data which is a
> bug on it's own so loosing a few page references is nothing compared to
> that.
Ah OK. Thanks for the patch.
Cheers,
Fengguang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: ext4_da_block_invalidatepages() question
2010-01-26 15:32 ` Jan Kara
2010-01-27 1:53 ` Wu Fengguang
@ 2010-02-09 15:39 ` Jan Kara
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kara @ 2010-02-09 15:39 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4@vger.kernel.org
> Hi,
>
> On Tue 26-01-10 21:36:08, Wu Fengguang wrote:
> > I noticed that ext4_da_block_invalidatepages() does pagevec_lookup()
> > without pagevec_release()/put_page(). Is that OK?
> Yes, the function looks buggy. Luckily, it is called only in case we are
> not able to allocate space for delay-allocated data which is a bug on its
> own. So people should never hit it.
> Attached patch should fix the issue. Ted, will you merge it please?
> Thanks.
Ted, could you please merge the patch below? Thanks!
Honza
---
>From 47085f1ac03eaca9e4d7a5f8f1e40e87d3879512 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 26 Jan 2010 16:15:19 +0100
Subject: [PATCH] ext4: Release page references acquired in ext4_da_block_invalidatepages
We forget to release page references we acquire in
ext4_da_block_invalidatepages. Luckily, this function gets called only if we
are not able to allocate blocks for delay-allocated data so that function
should better never be called.
Also cleanup handling of index variable.
Reported-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/inode.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index c818972..1680007 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2127,17 +2127,16 @@ static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd,
break;
for (i = 0; i < nr_pages; i++) {
struct page *page = pvec.pages[i];
- index = page->index;
- if (index > end)
+ if (page->index > end)
break;
- index++;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-02-09 15:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-26 13:36 ext4_da_block_invalidatepages() question Wu Fengguang
2010-01-26 15:32 ` Jan Kara
2010-01-27 1:53 ` Wu Fengguang
2010-01-27 12:25 ` Jan Kara
2010-01-27 12:34 ` Wu Fengguang
2010-02-09 15:39 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).