[PATCH] nfs: Fix nfs_readpages() error path

[PATCH] nfs: Fix nfs_readpages() error path

[OGAWA Hirofumi]

I've got the following oops.

------------[ cut here ]------------
kernel BUG at /devel/linux/works/linux-2.6/mm/readahead.c:315!
invalid opcode: 0000 [#1]
SMP

[...]

EFLAGS: 00210283   (2.6.19-rc3 #3)
EIP is at __do_page_cache_readahead+0x1a4/0x1b7
eax: f260db64   ebx: f8f9aa18   ecx: 00000001   edx: f7710594
esi: f6042b94   edi: f6042b84   ebp: f260db78   esp: f260db0c
ds: 007b   es: 007b   ss: 0068
Process emacs (pid: 3694, ti=f260d000 task=f670caa0 task.ti=f260d000)
Stack: 00000001 000001ac f25dd88c 0000085e 0000001e 00000001 00001000 d7f4a48c
       d7f4a50c d7f4a50c 00001000 d7f4a50c f260db60 00200246 00000001 22222222
       22222222 d7f4a50c d7f4a50c 00001000 d7f4a48c f260db68 c13cb9f8 c13cb9f8
Call Trace:
 [<c0140a3a>] do_page_cache_readahead+0x43/0x4d
 [<c013ce9b>] filemap_nopage+0x14e/0x328
 [<c0145b8e>] __handle_mm_fault+0x146/0x7b6
 [<c01463cc>] get_user_pages+0x1ce/0x293
 [<c017f0a8>] elf_core_dump+0xa05/0xba5
 [<c015b8a7>] do_coredump+0x565/0x5d1
 [<c0123acb>] get_signal_to_deliver+0x701/0x758
 [<c010246b>] do_notify_resume+0x8b/0x6c4
 [<c0102ede>] work_notifysig+0x13/0x19

The a_ops->readpages() is nfs_readpages(), and it seems to don't free
pages list in error path. So, it hit the
BUG_ON(!list_empty(&page_pool)) in __do_page_cache_readahead.

This clears all pages in error path.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 fs/nfs/read.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff -puN fs/nfs/read.c~nfs_readpages-error-path-fix fs/nfs/read.c
--- linux-2.6/fs/nfs/read.c~nfs_readpages-error-path-fix	2006-10-29 23:52:22.000000000 +0900
+++ linux-2.6-hirofumi/fs/nfs/read.c	2006-10-29 23:54:07.000000000 +0900
@@ -687,7 +687,7 @@ int nfs_readpages(struct file *filp, str
 	};
 	struct inode *inode = mapping->host;
 	struct nfs_server *server = NFS_SERVER(inode);
-	int ret = -ESTALE;
+	int ret;
 
 	dprintk("NFS: nfs_readpages (%s/%Ld %d)\n",
 			inode->i_sb->s_id,
@@ -695,13 +695,17 @@ int nfs_readpages(struct file *filp, str
 			nr_pages);
 	nfs_inc_stats(inode, NFSIOS_VFSREADPAGES);
 
-	if (NFS_STALE(inode))
-		goto out;
+	if (NFS_STALE(inode)) {
+		ret = -ESTALE;
+		goto out_error;
+	}
 
 	if (filp == NULL) {
 		desc.ctx = nfs_find_open_context(inode, NULL, FMODE_READ);
-		if (desc.ctx == NULL)
-			return -EBADF;
+		if (desc.ctx == NULL) {
+			ret = -EBADF;
+			goto out_error;
+		}
 	} else
 		desc.ctx = get_nfs_open_context((struct nfs_open_context *)
 				filp->private_data);
@@ -713,7 +717,11 @@ int nfs_readpages(struct file *filp, str
 			ret = err;
 	}
 	put_nfs_open_context(desc.ctx);
-out:
+
+	return ret;
+
+out_error:
+	put_pages_list(pages);
 	return ret;
 }
 
_

Re: [PATCH] nfs: Fix nfs_readpages() error path

[Trond Myklebust]

On Mon, 2006-10-30 at 00:56 +0900, OGAWA Hirofumi wrote:
> I've got the following oops.
> 
> ------------[ cut here ]------------
> kernel BUG at /devel/linux/works/linux-2.6/mm/readahead.c:315!
> invalid opcode: 0000 [#1]
> SMP
> 
> [...]
> 
> EFLAGS: 00210283   (2.6.19-rc3 #3)
> EIP is at __do_page_cache_readahead+0x1a4/0x1b7
> eax: f260db64   ebx: f8f9aa18   ecx: 00000001   edx: f7710594
> esi: f6042b94   edi: f6042b84   ebp: f260db78   esp: f260db0c
> ds: 007b   es: 007b   ss: 0068
> Process emacs (pid: 3694, ti=f260d000 task=f670caa0 task.ti=f260d000)
> Stack: 00000001 000001ac f25dd88c 0000085e 0000001e 00000001 00001000 d7f4a48c
>        d7f4a50c d7f4a50c 00001000 d7f4a50c f260db60 00200246 00000001 22222222
>        22222222 d7f4a50c d7f4a50c 00001000 d7f4a48c f260db68 c13cb9f8 c13cb9f8
> Call Trace:
>  [<c0140a3a>] do_page_cache_readahead+0x43/0x4d
>  [<c013ce9b>] filemap_nopage+0x14e/0x328
>  [<c0145b8e>] __handle_mm_fault+0x146/0x7b6
>  [<c01463cc>] get_user_pages+0x1ce/0x293
>  [<c017f0a8>] elf_core_dump+0xa05/0xba5
>  [<c015b8a7>] do_coredump+0x565/0x5d1
>  [<c0123acb>] get_signal_to_deliver+0x701/0x758
>  [<c010246b>] do_notify_resume+0x8b/0x6c4
>  [<c0102ede>] work_notifysig+0x13/0x19
> 
> The a_ops->readpages() is nfs_readpages(), and it seems to don't free
> pages list in error path. So, it hit the
> BUG_ON(!list_empty(&page_pool)) in __do_page_cache_readahead.

Wait. Why do we have this insane cleanup semantic anyway? I've just
grepped through the various readpages() methods out there. None of them
do anything more sophisticated than to call put_pages_list() in case of
error, and several of them get that wrong (including NFS, and CIFS).

Instead of the BUG_ON(), why can't we just stick a put_pages_list() into
__do_page_cache_readahead() and then get rid of all that duplicated
error handling in mpage_readpages(), nfs_readpages(), fuse_readpages(),
etc?

Cheers,
  Trond

Re: [PATCH] nfs: Fix nfs_readpages() error path

[Andrew Morton]

On Sun, 29 Oct 2006 14:10:38 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> Instead of the BUG_ON(), why can't we just stick a put_pages_list() into
> __do_page_cache_readahead() and then get rid of all that duplicated
> error handling in mpage_readpages(), nfs_readpages(), fuse_readpages(),
> etc?

I don't recall anything which would prevent that from being done.  iirc it
was one of those things which never happen.  Then things changed and it
happened once and was hence a special case.  Then more things changed and
it happened again, etc.

Re: [PATCH] nfs: Fix nfs_readpages() error path

[Trond Myklebust]

On Sun, 2006-10-29 at 12:16 -0800, Andrew Morton wrote:
> On Sun, 29 Oct 2006 14:10:38 -0500
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > Instead of the BUG_ON(), why can't we just stick a put_pages_list() into
> > __do_page_cache_readahead() and then get rid of all that duplicated
> > error handling in mpage_readpages(), nfs_readpages(), fuse_readpages(),
> > etc?
> 
> I don't recall anything which would prevent that from being done.  iirc it
> was one of those things which never happen.  Then things changed and it
> happened once and was hence a special case.  Then more things changed and
> it happened again, etc.

OK. I'll try to get round to doing it tomorrow (got to change to winter
tyres on my car today :-)).

Cheers,
  Trond

Re: [PATCH] nfs: Fix nfs_readpages() error path

[OGAWA Hirofumi]

Trond Myklebust <Trond.Myklebust@netapp.com> writes:

> On Mon, 2006-10-30 at 00:56 +0900, OGAWA Hirofumi wrote:
>> ------------[ cut here ]------------
>> kernel BUG at /devel/linux/works/linux-2.6/mm/readahead.c:315!
>> 
>> The a_ops->readpages() is nfs_readpages(), and it seems to don't free
>> pages list in error path. So, it hit the
>> BUG_ON(!list_empty(&page_pool)) in __do_page_cache_readahead.
>
> Wait. Why do we have this insane cleanup semantic anyway? I've just
> grepped through the various readpages() methods out there. None of them
> do anything more sophisticated than to call put_pages_list() in case of
> error, and several of them get that wrong (including NFS, and CIFS).
>
> Instead of the BUG_ON(), why can't we just stick a put_pages_list() into
> __do_page_cache_readahead() and then get rid of all that duplicated
> error handling in mpage_readpages(), nfs_readpages(), fuse_readpages(),
> etc?

Yes, I thought it too. Probably, author thought passed pages's owner
is readpages side, and owner can use that list with favorite way.

Well, both seems right things for me. So, the patch was done by
minimum change for -rc. If you want it, I'll do.


BTW, umm.. now I think, gfs2_readpages() seems to have a bug in error
path by different way. unlock_page() is really needed?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] nfs: Fix nfs_readpages() error path

[Trond Myklebust]

On Mon, 2006-10-30 at 05:40 +0900, OGAWA Hirofumi wrote:
> Well, both seems right things for me. So, the patch was done by
> minimum change for -rc. If you want it, I'll do.

Feel free. I should have time to work on it tomorrow, in case you don't
find time today.

> BTW, umm.. now I think, gfs2_readpages() seems to have a bug in error
> path by different way. unlock_page() is really needed?

That looks like a definite bug too.

Cheers,
  Trond

Re: [PATCH] nfs: Fix nfs_readpages() error path

[Steven Whitehouse]

Hi,

On Sun, 2006-10-29 at 16:58 -0500, Trond Myklebust wrote:
> On Mon, 2006-10-30 at 05:40 +0900, OGAWA Hirofumi wrote:
> > Well, both seems right things for me. So, the patch was done by
> > minimum change for -rc. If you want it, I'll do.
> 
> Feel free. I should have time to work on it tomorrow, in case you don't
> find time today.
> 
> > BTW, umm.. now I think, gfs2_readpages() seems to have a bug in error
> > path by different way. unlock_page() is really needed?
> 
> That looks like a definite bug too.
> 
> Cheers,
>   Trond

Agreed. It will be fixed fairly shortly as part of a patch related to a
race when reading and truncating "stuffed" files,

Steve.