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

[Cluster-devel] 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>

[Cluster-devel] 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

[Cluster-devel] 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.