Re: [PATCH 08/15] SUNRPC: Ensure rpcauth_prune_expired() respects the nr_to_scan parameter

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

On Fri, 2010-05-14 at 13:32 -0400, Chuck Lever wrote: 
> On 05/14/10 01:13 PM, Trond Myklebust wrote:
> > On Fri, 2010-05-14 at 12:34 -0400, Chuck Lever wrote:
> >> On 05/13/10 05:08 PM, Trond Myklebust wrote:
> >>> Signed-off-by: Trond Myklebust<Trond.Myklebust@netapp.com>
> >>> ---
> >>>    net/sunrpc/auth.c |    5 ++---
> >>>    1 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> >>> index 2213dc5..5fb02ac 100644
> >>> --- a/net/sunrpc/auth.c
> >>> +++ b/net/sunrpc/auth.c
> >>> @@ -236,6 +236,8 @@ rpcauth_prune_expired(struct list_head *free, int nr_to_scan)
> >>>
> >>>    	list_for_each_entry_safe(cred, next,&cred_unused, cr_lru) {
> >>>
> >>> +		if (nr_to_scan-- == 0)
> >>> +			break;
> >>>    		/*
> >>>    		 * Enforce a 60 second garbage collection moratorium
> >>>    		 * Note that the cred_unused list must be time-ordered.
> >>> @@ -255,11 +257,8 @@ rpcauth_prune_expired(struct list_head *free, int nr_to_scan)
> >>>    			get_rpccred(cred);
> >>>    			list_add_tail(&cred->cr_lru, free);
> >>>    			rpcauth_unhash_cred_locked(cred);
> >>> -			nr_to_scan--;
> >>>    		}
> >>>    		spin_unlock(cache_lock);
> >>> -		if (nr_to_scan == 0)
> >>> -			break;
> >>>    	}
> >>>    	return (number_cred_unused / 100) * sysctl_vfs_cache_pressure;
> >>>    }
> >>
> >> It looks to me like the mm calls our cache shrinker with nr_to_scan set
> >> to zero when it just wants this return value, and nothing more.  But the
> >> logic here seems to assume that nr_to_scan == 0 means shrink as much as
> >> you can.  Am I reading this correctly?
> >
> > Look more carefully: the comparison contains a post-decrement operation,
> > so if the nr_to_scan == 0, then we immediately exit the loop (after
> > decrementing nr_to_scan, but who cares about that)...
> 
> When mm calls with nr_to_scan set to zero, it doesn't expect a -1 
> return, it just uses the returned value.  mm checks for a -1 return only 
> when a non-zero scan count argument is passed.
> 
> So the check you added here (and in the access cache shrinker) to return 
> -1 when the gfp_mask doesn't contain GFP_KERNEL could cause some 
> trouble.  It would be safer if we return -1 _after_ checking for 
> nr_to_scan == 0.

Oh... You're referring to the change that was added in Patch 6/15
SUNRPC: Dont run rpcauth_cache_shrinker() when gfp_mask is GFP_NOFS? I
got confused...

I can perhaps rather add a check for nr_to_scan != 0 in that patch...