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

["NeilBrown" <neilb@suse.de>]

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.
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?

Thanks,
NeilBrown