[RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[Suresh Jayaraman]

We seem to be assuming fscache_acquire_cookie will always succeed and will
obtain a valid cookie. But, under low memory conditions kmem_cache_alloc
could fail and that would leave nfsi->fscache being set to NULL.

This could lead to a possibility of a page with PG_fscache flag set but
cookie not set(i.e. NULL). This inconsistency could trigger a kernel BUG in
nfs_fscache_release_page() and other places as we do BUG_ON(!cookie), I think.

I'm aware that there are other places where we would need such checks, but
wanted to validate this patch before getting on to them.

Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
---

 fs/nfs/client.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index ee77713..7776e8a 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -155,6 +155,8 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
 		clp->cl_machine_cred = cred;
 
 	nfs_fscache_get_client_cookie(clp);
+	if (!clp->fscache)
+		goto error_cleanup;
 
 	return clp;
 
-- 
Suresh Jayaraman

Re: [RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[Trond Myklebust]

On Wed, 2010-01-20 at 17:51 +0530, Suresh Jayaraman wrote: 
> We seem to be assuming fscache_acquire_cookie will always succeed and will
> obtain a valid cookie. But, under low memory conditions kmem_cache_alloc
> could fail and that would leave nfsi->fscache being set to NULL.
> 
> This could lead to a possibility of a page with PG_fscache flag set but
> cookie not set(i.e. NULL). This inconsistency could trigger a kernel BUG in
> nfs_fscache_release_page() and other places as we do BUG_ON(!cookie), I think.
> 
> I'm aware that there are other places where we would need such checks, but
> wanted to validate this patch before getting on to them.
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman@suse.de>
> ---
> 
>  fs/nfs/client.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index ee77713..7776e8a 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -155,6 +155,8 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
>  		clp->cl_machine_cred = cred;
>  
>  	nfs_fscache_get_client_cookie(clp);
> +	if (!clp->fscache)
> +		goto error_cleanup;
>  
>  	return clp;
>  

That looks like it will always fail when mounting without -ofscache
and/or compiling without CONFIG_NFS_FSCACHE.

It seems like a better idea to have nfs_fscache_get_client_cookie()
explicitly set clp->fscache to ERR_PTR(-ENOMEM) value if the
fscache_acquire_cookie fails.

Cheers
  Trond

Re: [RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[David Howells]

Suresh Jayaraman <sjayaraman@suse.de> wrote:

> +	if (!clp->fscache)
> +		goto error_cleanup;

This should be unnecessary.  FS-Cache's API functions in linux/fscache.h
should handle a NULL cookie and do the right thing.

David

Re: [RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[Suresh Jayaraman]

On 01/21/2010 04:37 PM, David Howells wrote:
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> +	if (!clp->fscache)
>> +		goto error_cleanup;
> 
> This should be unnecessary.  FS-Cache's API functions in linux/fscache.h
> should handle a NULL cookie and do the right thing.
> 

Did you mean __fscache_acquire_cookie() function or any other post
operations after the cookie has been acquired?

Because, nfs_fscache_get_client_cookie() calls fscache_acquire_cookie()
which inturn calls __fscache_acquire_cookie() that returns NULL if
kmem_cache_alloc fails and hence clp->fscache gets a negative cookie in
nfs_fscache_get_client_cookie.

I happened to stumble upon this code when I tried to investigate a bug
that reported a kerbel BUG in 2.6.32 while running LTP testsuite.

--snip--
kernel BUG at fs/nfs/fscache.c:360!

Here's the backtrace:

 #0 [ffff88001cc573e0] machine_kexec at ffffffff81021532
 #1 [ffff88001cc57430] crash_kexec at ffffffff81094280
 #2 [ffff88001cc57500] oops_end at ffffffff814b0290
 #3 [ffff88001cc57520] do_invalid_op at ffffffff81004af4
 #4 [ffff88001cc575c0] invalid_op at ffffffff81003cd5
 #5 [ffff88001cc57648] nfs_fscache_release_page at ffffffffa028a2cd
 #6 [ffff88001cc57690] shrink_page_list at ffffffff810e525e
 #7 [ffff88001cc57710] isolate_lru_pages at ffffffff810e47f3
 #8 [ffff88001cc577a0] shrink_inactive_list at ffffffff810e5669
 #9 [ffff88001cc57930] shrink_zone at ffffffff810e62c4
#10 [ffff88001cc579d0] shrink_zones at ffffffff810e63c3
#11 [ffff88001cc57a10] do_try_to_free_pages at ffffffff810e747d
#12 [ffff88001cc57a70] try_to_free_pages at ffffffff810e7832
#13 [ffff88001cc57ad0] __alloc_pages_slowpath at ffffffff810ddef3
#14 [ffff88001cc57b90] __alloc_pages_nodemask at ffffffff810de35c
#15 [ffff88001cc57bb0] load_balance_newidle at ffffffff81048c69
#16 [ffff88001cc57c80] ondemand_readahead at ffffffff810e1d43
#17 [ffff88001cc57cc0] do_generic_file_read at ffffffff810d8cfe
#18 [ffff88001cc57d40] generic_file_aio_read at ffffffff810d9536
#19 [ffff88001cc57dd0] do_sync_read at ffffffff81122f53
#20 [ffff88001cc57f00] vfs_read at ffffffff811237a7
#21 [ffff88001cc57f30] sys_read at ffffffff81123980
#22 [ffff88001cc57f80] system_call_fastpath at ffffffff81002f3b
    RIP: 00002b6e30d4b2b0  RSP: 00007fff30463528  RFLAGS: 00010206

--snip--

My interpretation of the BUG is:

Under low (seemingly very low) memory conditions, when we try to do a
NFS read and hence ondemand_readahead(), while trying to preallocate
memory, we find that get_page_from_freelist() didn't manage to find
pages from the free list and hence __alloc_pages_slowpath() is being
invoked.

Direct reclaim kicks in as our attempt to get free list fails even after
the background reclaim. Direct reclaim attempts to shrink caches. And in
shrink_page_list() we check if either page->PG_private or
page->PG_private1 (i.e. PG_fscache) flag is set before calling
try_to_release_page().

try_to_release_page() calls the nfs release_page method where the check
for PG_private fails and nfs_fscache_release_page() gets called and the
kernel BUGs when it finds nfsi->fscache is NULL.

The fact PG_private is not set in nfs_release_page implies that
PG_fscache is set for the page because of check in shrink_page_list()
which in turn implies it's a cache object and cookies should not be
NULL. It appears that the chances of a race condition is also unlikely
because we seem to be holding page lock in try_to_release_page().

This lead to the question of whether we are setting the cookie properly
in all cases and the allocation failure in fscache_acquire_cookie()
seemed like a potential place where it could still get set to NULL and
remain as it is.

Perhaps, my hypothesis has a hole or two or I'm miss something.



Thanks,

-- 
Suresh Jayaraman

Re: [RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[Suresh Jayaraman]

On 01/21/2010 04:37 PM, David Howells wrote:
> Suresh Jayaraman <sjayaraman@suse.de> wrote:
> 
>> +	if (!clp->fscache)
>> +		goto error_cleanup;
> 
> This should be unnecessary.  FS-Cache's API functions in linux/fscache.h
> should handle a NULL cookie and do the right thing.
> 

Also, while poking around the code, noticed that nfs_clear_inode() calls
nfs_fscache_release_inode_cookie() which doesn't seem to check whether
"fsc" mount option (i.e. NFS_FSCACHE(inode)) was used or not and goes
ahead to call fscache_relinquish_cookie(). As I understand we should not
be calling any fscache related functions when the client doesn't use fsc
mount option (though CONFIG_NFS_FSCACHE is set), right?

Thanks,

-- 
Suresh Jayaraman

Re: [RFC][PATCH] nfs: don't assume fscache_acquire_cookie will always succeed

[David Howells]

Suresh Jayaraman <sjayaraman@suse.de> wrote:

> As I understand we should not be calling any fscache related functions when
> the client doesn't use fsc mount option (though CONFIG_NFS_FSCACHE is set),
> right?

Not necessarily.  It can be done that way, but if you just set the top-level
cookie to NULL, fscache.h will just od the right thing all the way down.  It
adds a test and jump to each path.

Anyway, I can't really look at it now, I'm on holiday in NZ for a couple more
weeks.

David