diff mbox

ext4_da_block_invalidatepages() question

Message ID 20100126153222.GI3187@quack.suse.cz
State New, archived
Headers show

Commit Message

Jan Kara Jan. 26, 2010, 3:32 p.m. UTC 
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

Comments

kbuild test robot Jan. 27, 2010, 1:53 a.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jan Kara Jan. 27, 2010, 12:25 p.m. UTC | #2 
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
kbuild test robot Jan. 27, 2010, 12:34 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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++;
-
 			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;
 }
-- 
1.6.4.2