Re: [PATCH 1/2] NFS: Fix a bug in nfs_fscache_release_page()

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Sun, 2010-02-07 at 16:56 +0530, Suresh Jayaraman wrote: 
> On 02/06/2010 04:13 AM, Trond Myklebust wrote:
> > Not having an fscache cookie is perfectly valid if the user didn't mount
> > with the fscache option.
> > 
> > This patch fixes http://bugzilla.kernel.org/show_bug.cgi?id=15234
> > 
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> > Cc: stable@kernel.org
> > ---
> >  fs/nfs/fscache.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> > index fa58800..534adb8 100644
> > --- a/fs/nfs/fscache.c
> > +++ b/fs/nfs/fscache.c
> > @@ -355,11 +355,11 @@ void nfs_fscache_reset_inode_cookie(struct inode *inode)
> >  int nfs_fscache_release_page(struct page *page, gfp_t gfp)
> >  {
> >  	struct nfs_inode *nfsi = NFS_I(page->mapping->host);
> > -	struct fscache_cookie *cookie = nfsi->fscache;
> > -
> > -	BUG_ON(!cookie);
> >  
> >  	if (PageFsCache(page)) {
> > +		struct fscache_cookie *cookie = nfsi->fscache;
> > +
> > +		BUG_ON(!cookie);
> >  		dfprintk(FSCACHE, "NFS: fscache releasepage (0x%p/0x%p/0x%p)\n",
> >  			 cookie, page, nfsi);
> >  
> 
> There are only two callers for nfs_fscache_release_page() -
> nfs_release_page() and nfs_migrate_page(). nfs_migrate_page already does
> this:
> 
>        if (PageFsCache(page))
>                 nfs_fscache_release_page(page, GFP_KERNEL);
> 
> and the assumption in nfs_release_page() is that the page should have
> either PG_private set or PG_fscache set and nfs_fscache_release_page
> gets called only if PG_private is not set.

...or if it gets cleared.

> I think the idea is that nfs_fscache_release_page should not get called
> if fsc option is not used. So it appears to me this patch is fixing the
> symptom not the actual issue. Perhaps, this the assumption in
> nfs_release_page is wrong or the PageFsCache() check should be moved to
> nfs_release_page?

No. We should rather get rid of the redundant check for PageFsCache() in
nfs_migrate_page. PageFsCache() is particular to fscache, so the test
belongs in the fscache code.

Trond