Re: [PATCH 0/2] NFS: limit use of ACCESS cache for negative responses

From: "NeilBrown" <neilb@suse.de>
To: "Trond Myklebust" <trondmy@hammerspace.com>
Cc: "anna@kernel.org" <anna@kernel.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 0/2] NFS: limit use of ACCESS cache for negative responses
Date: Tue, 17 May 2022 11:22:42 +1000	[thread overview]
Message-ID: <165275056203.17247.1826100963816464474@noble.neil.brown.name> (raw)
In-Reply-To: <3ec50603479c7ee60cfa269aa06ae151e3ebc447.camel@hammerspace.com>

On Tue, 17 May 2022, Trond Myklebust wrote:
> On Tue, 2022-05-17 at 11:05 +1000, NeilBrown wrote:
> > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> > > > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > > > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > > > > 
> > > > > > Hi,
> > > > > >  any thoughts on these patches?
> > > > > 
> > > > > 
> > > > > To me, this problem is simply not worth breaking hot path
> > > > > functionality
> > > > > for. If the credential changes on the server, but not on the
> > > > > client
> > > > > (so
> > > > > that the cred cache comparison still matches), then why do we
> > > > > care?
> > > > > 
> > > > > IOW: I'm a NACK until convinced otherwise.
> > > > 
> > > > In what way is the hot path broken?  It only affect a permission
> > > > test
> > > > failure.  Why is that considered "hot path"??
> > > 
> > > It is a permission test that is critical for caching path
> > > resolution on
> > > a per-user basis.
> > > 
> > > > 
> > > > RFC 1813 says - for NFSv3 at least - 
> > > > 
> > > >       The information returned by the server in response to an
> > > >       ACCESS call is not permanent. It was correct at the exact
> > > >       time that the server performed the checks, but not
> > > >       necessarily afterwards. The server can revoke access
> > > >       permission at any time.
> > > > 
> > > > Clearly the server can allow allow access at any time.
> > > > This seems to argue against caching - or at least against relying
> > > > too
> > > > heavily on the cache.
> > > > 
> > > > RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> > > > 
> > > > "why do we care?" Because the server has changed to grant access,
> > > > but
> > > > the client is ignoring the possibility of that change - so the
> > > > user
> > > > is
> > > > prevented from doing work.
> > > 
> > > The server enforces permissions in NFS. The client permissions
> > > checks
> > > are performed in order to gate access to data that is already in
> > > cache.
> > 
> > So if the permission check fails, then the client should ignore the
> > cache and ask the server for the requested data, so that the server
> > has
> > a chance to enforce the permissions - whether denying or allowing the
> > access.
> > 
> > I completely agree with you, and that is effectively what my patch
> > implements.
> > 
> 
> No. I'm saying that no matter how many spec paragraphs you quote at me,
> I'm not willing to introduce a timeout on a cache that is critical for
> path resolution in order to address a corner case of a corner case.
> 
> I'm saying that if you want this problem fixed, then you need to look
> at a different solution that doesn't have side-effects for the
> 99.99999% cases or more that I do care about.

What, specifically, as the cases that you do care about?

I assumed that the cases you care about are cases where the user *does*
have access, and where this access is correctly cached, so that a 
  permission(..., MAY_EXEC)
call returns immediately with a positive answer.
I care about these cases too, and I've ensured that the patch doesn't
change the behaviour for these cases.

What other cases - cases where permission() returns an error - do you care
about? 

Thanks,
NeilBrown