ext4_da_block_invalidatepages() question
Hi Jan,
I noticed that ext4_da_block_invalidatepages() does pagevec_lookup() without pagevec_release()/put_page(). Is that OK?
Thanks,
Fengguang
Jan Kara
reviewerHi,
Quoted 2 lines · Wu Fengguang
I noticed that
ext4_da_block_invalidatepages()doespagevec_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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 1 line
From
47085f1ac03eaca9e4d7a5f8f1e40e87d3879512Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
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.
--- fs/ext4/inode.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
fs/ext4/inode.c +1 -3
@@ -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++;
Quoted 11 lines · Jan Kara
(quoted diff) +1
-4@@ -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));
Review static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd, · line 2142
Quoted 10 lines
(quoted diff) +2
-0BUG_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
Jan Kara
reviewerQuoted 19 lines · Wu Fengguang
Quoted 17 lines · Jan Kara
fs/ext4/inode.c +2
-4@@ -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 ;
Quoted 8 lines
Review static void ext4_da_block_invalidatepages(struct mpage_da_data *mpd, · line 2142
Quoted 4 lines
(quoted diff) +1
-0+ 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
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 9 lines · Jan Kara
Quoted 3 lines · Wu Fengguang
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
Jan Kara
reviewerQuoted 10 lines
Hi,
Quoted 2 lines · Wu Fengguang
I noticed that
ext4_da_block_invalidatepages()doespagevec_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
---
Quoted 1 line
From
47085f1ac03eaca9e4d7a5f8f1e40e87d3879512Mon Sep 17 00:00:00 2001
From: Jan Kara <[email protected]>
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.
--- fs/ext4/inode.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
fs/ext4/inode.c +1 -3
@@ -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++;