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@xxxxxxxxxx>
