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

[Suresh Jayaraman <sjayaraman@suse.de>]

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.

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?



Thanks,

-- 
Suresh Jayaraman