Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 26 Mar 2020 21:50:12 -0400
From:   "J. Bruce Fields" <bfields@redhat.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "bfields@fieldses.org" <bfields@fieldses.org>,
        "kinglongmee@gmail.com" <kinglongmee@gmail.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] SUNRPC/cache: Allow garbage collection of invalid cache
 entries
Message-ID: <20200327015012.GA107036@pick.fieldses.org>
References: <20200114165738.922961-1-trond.myklebust@hammerspace.com>
 <20200206163322.GB2244@fieldses.org>
 <8dc1ed17de98e4b59fb9e408692c152456863a20.camel@hammerspace.com>
 <20200207181817.GC17036@fieldses.org>
 <20200326204001.GA25053@fieldses.org>
 <1a0ce8bb1150835f7a25126df2524e8a8fb0e112.camel@hammerspace.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1a0ce8bb1150835f7a25126df2524e8a8fb0e112.camel@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

On Thu, Mar 26, 2020 at 09:42:19PM +0000, Trond Myklebust wrote:
> On Thu, 2020-03-26 at 16:40 -0400, bfields@fieldses.org wrote:
> > Maybe the cache_is_expired() logic should be something more like:
> > 
> > 	if (h->expiry_time < seconds_since_boot())
> > 		return true;
> > 	if (!test_bit(CACHE_VALID, &h->flags))
> > 		return false;
> > 	return h->expiry_time < seconds_since_boot();
> > 
> > So invalid cache entries (which are waiting for a reply from mountd)
> > can
> > expire, but they can't be flushed.  If that makes sense.
> > 
> > As a stopgap we may want to revert or drop the "Allow garbage
> > collection" patch, as the (preexisting) memory leak seems lower
> > impact
> > than the server hang.
> 
> I believe you were probably seeing the effect of the
> cache_listeners_exist() test, which is just wrong for all cache upcall
> users except idmapper and svcauth_gss. We should not be creating
> negative cache entries just because the rpc.mountd daemon happens to be
> slow to connect to the upcall pipes when starting up, or because it
> crashes and fails to restart correctly.
> 
> That's why, when I resubmitted this patch, I included 
> https://git.linux-nfs.org/?p=cel/cel-2.6.git;a=commitdiff;h=b840228cd6096bebe16b3e4eb5d93597d0e02c6d
> 
> which turns off that particular test for all the upcalls to rpc.mountd.

The hangs persist with that patch, but go away with the change to the
cache_is_expired() logic above.

--b.