Re: [PATCH] sunrpc/cache: handle missing listeners better.

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3944 bytes --]

On Fri, Mar 22 2019, J. Bruce Fields wrote:

> I've gotten complaints about the same thing and said "well, in
> retrospect we shouldn't have designed the interface this way, but we
> did, so just stop opening those files".

As the fool who actually "designed" this, I can say with some confidence
that the intention was always the requests would block for at most 30
seconds.
Unfortunately the implementation was sloppy and the testing was
haphazard. 

>
> But maybe this is a reasonable compromise.
>
> One advantage of waiting for mountd to come back is that you could
> upgrade mountd in place.  That shouldn't take 30 seconds, though.  And I
> haven't heard of anyone actually doing that.

Surely upgrading of mountd in-place happens whenever you install a new
version.
That was (iirc) the main reason for not treating a recent close as fatal.

>
> It's too bad that not opening auth.unix.gid is the only way for mountd
> to communicate that gids shouldn't be mapped.

I have a general preference for reusing existing functionality rather
than creating new special-purpose functionality.  I think this has
served me well more often than not.  Maybe this is one case of "not".

If you want to restart mountd without --managed-gids (where previously
it had that option), there is a chance that you will hit this problem.
That is a case where the answer "just stop opening those files" doesn't
really apply.

Thanks,
NeilBrown

>
> --b.
>
> On Fri, Mar 22, 2019 at 01:16:56PM +1100, NeilBrown wrote:
>> If no handler (such as rpc.mountd) has opened
>> a cache 'channel', the sunrpc cache responds to
>> all lookup requests with -ENOENT.  This is particularly
>> important for the auth.unix.gid cache which is
>> optional.
>> 
>> If the channel was open briefly and an upcall was written to it,
>> this upcall remains pending even when the handler closes the
>> channel.  When an upcall is pending, the code currently
>> doesn't check if there are still listeners, it only performs
>> that check before sending an upcall.
>> 
>> As the cache treads a recently closes channel (closed less than
>> 30 seconds ago) as "potentially still open", there is a
>> reasonable sized window when a request can become pending
>> in a closed channel, and thereby block lookups indefinitely.
>> 
>> This can easily be demonstrated by running
>>   cat /proc/net/rpc/auth.unix.gid/channel
>> 
>> and then trying to mount an NFS filesystem from this host.  It
>> will block indefinitely (unless mountd is run with --manage-gids,
>> or krb5 is used).
>> 
>> When cache_check() finds that an upcall is pending, it should
>> perform the "cache_listeners_exist()" exist test.  If no
>> listeners do exist, the request should be negated.
>> 
>> With this change in place, there can still be a 30second wait on
>> mount, until the cache gives up waiting for a handler to come
>> back, but this is much better than an indefinite wait.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  net/sunrpc/cache.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index 12bb23b8e0c5..be9e29385adc 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -40,6 +40,7 @@
>>  
>>  static bool cache_defer_req(struct cache_req *req, struct cache_head *item);
>>  static void cache_revisit_request(struct cache_head *item);
>> +static bool cache_listeners_exist(struct cache_detail *detail);
>>  
>>  static void cache_init(struct cache_head *h, struct cache_detail *detail)
>>  {
>> @@ -303,7 +304,8 @@ int cache_check(struct cache_detail *detail,
>>  				cache_fresh_unlocked(h, detail);
>>  				break;
>>  			}
>> -		}
>> +		} else if (!cache_listeners_exist(detail))
>> +			rv = try_to_negate_entry(detail, h);
>>  	}
>>  
>>  	if (rv == -EAGAIN) {
>> -- 
>> 2.14.0.rc0.dirty
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]