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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2022-08-29 at 09:32 +1000, NeilBrown wrote:
> On Sat, 27 Aug 2022, Trond Myklebust wrote:
> > On Sat, 2022-08-27 at 09:39 +1000, NeilBrown wrote:
> > > On Sat, 27 Aug 2022, Trond Myklebust wrote:
> > > > On Fri, 2022-08-26 at 10:59 -0400, Benjamin Coddington wrote:
> > > > > On 16 May 2022, at 21:36, Trond Myklebust wrote:
> > > > > > So until you have a different solution that doesn't impact the
> > > > > > client's
> > > > > > ability to cache permissions, then the answer is going to be
> > > > > > "no"
> > > > > > to
> > > > > > these patches.
> > > > > 
> > > > > Hi Trond,
> > > > > 
> > > > > We have some folks negatively impacted by this issue as well. 
> > > > > Are
> > > > > you
> > > > > willing to consider this via a mount option?
> > > > > 
> > > > > Ben
> > > > > 
> > > > 
> > > > I don't see how that answers my concern.
> > > 
> > > Could you please spell out again what your concerns are?  I still
> > > don't
> > > understand. 
> > > The only performance impact is when a permission test fails.  In what
> > > circumstance is permission failure expected on a fast-path?
> > > 
> > 
> > You're treating the problem as if it were a timeout issue, when clearly
> > it has nothing at all to do with timeouts. There is no problem of
> > 'group membership changes on a regular basis' to be solved.
> 
> You are the one who suggested a timeout.  I quote:
> 
> > That way, you have a mechanism that serves all purposes: it can do an
> > immediate one-time only flush, or you can set up a userspace job that
> > issues a global flush once every so often, e.g. using a cron job.
> 
> "every so often" == "timeout".  I thought that maybe this was somewhere
> that we could find some agreement.  As I said, I would set the timeout
> to zero.  I don't really want the timeout - not more than the ac
> timeouts we already have.
> 
> > 
> > The problem to be solved is that on the very rare occasion when a group
> > membership does change, then the server and the client may update their
> > view of that membership at completely different times. In the
> > particular case when the client updates its view of the group
> > membership before the server does, then the access cache is polluted,
> > and there is no remedy.
> 
> Agreed.
> 
> > 
> > So my concerns are around the mismatch of problem and solution. I see
> > multiple issues.
> > 
> >    1. Your timeouts are per inode. That means that if inode A sees the
> >       problem being solved, then there is no guarantee that inode B
> >       sees the same problem as being solved (and the converse is true
> >       as well).
> 
> Is that a problem?  Is that even anything new?
> If I chmod file A and file B on the server, then the client may see
> the change to file A before the change to file B (or vice-versa).
> i.e.  the inconsistent-cache problem might be solved for one but not the
> other.  It has always been this way.
> 
> >    2. There is no quick on-the-spot solution. If your admin updates the
> >       group membership, then you are only guaranteed that the client
> >       and server are in sync once the server has picked up the solution
> >       (however you arrange that), and the client cache has expired.
> >       IOW: your only solution is to wait 1 client cache expiration
> >       period after the server is known to be fixed (or to reboot the
> >       client).
> 
> This is also the only solution to seeing other changes that have been
> made to inodes.
> For file/directory content you can open the file/directory and this
> triggers CTO consistency checks.  But stat() or access() doesn't.
> 
> Hmmm.. What if we add an ACCESS check to the OPEN request for files, and
> the equivalent GETATTR for directories?  That would provide a direct
> way to force a refresh without adding any extra RPC requests??
> 
> >    3. There is no solution at all for the positive cache case. If your
> >       sysadmin is trying to revoke an access due to a group membership
> >       change, their only solution is to reboot the client.
> 
> Yes.  Revoking read/execute access that you have already granted is not
> really possible.  The application may have already read the file.  It
> might even have emailed the content to $BLACKHAT.  Even rebooting the
> client isn't really a solution.
> Revoking write access already works fine as does revoking read access to
> a file before putting new content in it - the new content is safe.
> 
> >    4. You are tying the access cache timeout to the completely
> >       unrelated 'acregmin' and 'acdirmin' values. Not only does that
> >       mean that the default values for regular files are extremely
> >       small (3 seconds), meaning that we have to refresh extremely
> >       often. However it also means that you have to explain why
> >       directories behave differently (longer default timeouts) despite
> >       the fact that the group membership changed at exactly the same
> >       time for both types of object.
> >          1. Bonus points for explaining why our default values are designed
> >             for a group membership that changes every 3 seconds.
> 
> I don't see why you treat the access information as different from all
> the other attributes.  "bob has group x access to the directory" and
> "file size if 42000 bytes" are just attributes of the inode.  We collect
> them different ways, but they are not deeply different.
> 
> The odd thing here is that we cache these "access" attributes
> indefinitely when ctime doesn't change - even though there is no
> guarantee that ctime captures access changes.  I think that choice needs
> to be justified.

Maybe I'm being pedantic, but I don't see the first as an inode
attribute. There are really 2 pieces to that access control example:

- bob is a member of group x
- group x has access to the directory

The first has nothing directly to do with the inode and so it's no
surprise that its ctime and i_version aren't affected when group
membership changes.

> Using the cached value indefinitely when it grants access is defensible
> because it is a frequent operation (checking x access in a directory
> while following a patch).  I think that adequately justifies the choice.
> I cannot see the justification when the access is denied by the cache.
> 
> >    5. 'noac' suddenly now turns off access caching, but only for
> >       negative cached values.
> 
> Is this a surprise?
> 
> But what do you think of adding an ACCESS check when opening a dir/file?
> At least for NFSv4?

-- 
Jeff Layton <jlayton@kernel.org>