| Message ID | 87r2p7rvn5.fsf@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
ebiederm@xmission.com (Eric W. Biederman) writes: 2> So the purpose for having a patch in the first place is that > 2a3a2a3f3524 ("ovl: don't cache acl on overlay layer") > which addded ACL_DONT_CACHED did not result in any comment updates > to get_acl. > > Which mean that if you read the comments in get_acl() that you > don't even think of ACL_DONT_CACHED. > > Which means that this comment: > /* > * If the ACL isn't being read yet, set our sentinel. Otherwise, the > * current value of the ACL will not be ACL_NOT_CACHED and so our own > * sentinel will not be set; another task will update the cache. We > * could wait for that other task to complete its job, but it's easier > * to just call ->get_acl to fetch the ACL ourself. (This is going to > * be an unlikely race.) > */ > > Which presumes the only reason the acl could be anything other > ACL_NOT_CACHED is because get_acl() is already being called upon it in > another task. > > I wanted something to mention ACL_DONT_CACHED so someone would at least > think about that case if they ever step up to modify the code. > > The code is perfectly clear, the comment is not. That scares me. > > And I had to read the code about a dozen times before I realized the > ACL_DONT_CACHED case even exists. Not useful when I am need to use > that to preserve historical fuse semantics. > > So something is missing here even if my wording does not improve things. > > > > Then we get this comment: > /* > * Normally, the ACL returned by ->get_acl will be cached. > * A filesystem can prevent that by calling > * forget_cached_acl(inode, type) in ->get_acl. > */ > > Which was added in b8a7a3a66747 ("posix_acl: Inode acl caching fixes") > That comment is and always has been rubbish. > > I don't have a clue what it is trying to say but it is not something > a person can use to write filesystem code with. > > > Truths: > - forget_cached_acl(inode, type) can be used to invalidate the acl > cache. > > - Calling forget_cached_acl from within the filesystems ->get_acl > method won't prevent a cached value from being returend because > ->get_acl will be set. > > - Calling forget_cached_acl from within the filesystems ->get_acl > method won't prevent a returned value from being cached > because it the caching happens after ->get_acl returns. Sigh. Yes it will because we set the special sentinel value, and forget_cached_acl will replace the sentinel value with ACL_NOT_CACHED. It is a terribly brittle and racy thing to do, and it probably won't work to say cache this acl but not this one on a case by case bases in ->get_acl. As such I believe that usage of forget_cached_acl should be subsumed by using ACL_NOT_CACHED. If not we should really come up with a different helper function name to call from ->get_acl. Preferably one that does "cmpxchng(p, sentinel, ACL_NOT_CACHED)" so that we remove the races. > - Setting inode->i_acl = ACL_DONT_CACHE is the only way to prevent > a value from ->get_acl from being cached. > > > In summary I only care about two things. > 1) ACL_NOT_CACHED being mentioned somewhere in get_acl so people looking > at the code, and people updating the code will have a hint that they > need to consider that case. > > 2) That misleading completely bogus comment being removed/fixed. > > > And yes I agree the code is clear. The comments are not. > > > Does this look better as a comment updating patch? > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 2fd0fde16fe1..5453094b8828 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -98,6 +98,11 @@ struct posix_acl *get_acl(struct inode *inode, int type) > struct posix_acl **p; > struct posix_acl *acl; > > + /* > + * To avoid caching the result of ->get_acl > + * set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE; > + */ > + > /* > * The sentinel is used to detect when another operation like > * set_cached_acl() or forget_cached_acl() races with get_acl(). > @@ -126,9 +131,7 @@ struct posix_acl *get_acl(struct inode *inode, int type) > /* fall through */ ; > > /* > - * Normally, the ACL returned by ->get_acl will be cached. > - * A filesystem can prevent that by calling > - * forget_cached_acl(inode, type) in ->get_acl. > + * The ACL returned by ->get_acl will be cached. > * > * If the filesystem doesn't have a get_acl() function at all, we'll > * just create the negative cache entry. > > Eric
On Mon, Feb 26, 2018 at 6:53 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > So the purpose for having a patch in the first place is that > 2a3a2a3f3524 ("ovl: don't cache acl on overlay layer") > which addded ACL_DONT_CACHED did not result in any comment updates > to get_acl. I'm not opposed to just updating the comments. I just think your updates were somewhat misleading. > Which mean that if you read the comments in get_acl() that you > don't even think of ACL_DONT_CACHED. Right. By all means add a comment about ACL_DONT_CACHE disabling the cache entirely. But don't _remove_ the other valid way to flush the cache, and don't make that comment above cmpxchg() be even more confusing than the code is. > Does this look better as a comment updating patch? > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 2fd0fde16fe1..5453094b8828 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -98,6 +98,11 @@ struct posix_acl *get_acl(struct inode *inode, int type) > struct posix_acl **p; > struct posix_acl *acl; > > + /* > + * To avoid caching the result of ->get_acl > + * set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE; > + */ > + > /* > * The sentinel is used to detect when another operation like > * set_cached_acl() or forget_cached_acl() races with get_acl(). > @@ -126,9 +131,7 @@ struct posix_acl *get_acl(struct inode *inode, int type) > /* fall through */ ; > > /* > - * Normally, the ACL returned by ->get_acl will be cached. > - * A filesystem can prevent that by calling > - * forget_cached_acl(inode, type) in ->get_acl. > + * The ACL returned by ->get_acl will be cached. Why do you hate forget_cached_acl()? It's perfectly valid too. Don't remove that comment. Maybe reword it to talk not about "preventing", but about "invalidating the cache". But the old comment that you remove isn't _wrong_, it's just that the "preventing" from returning the cached state with forget_cached_acl() is just a one-time thing. So forget_cached_acl() exists, and it works, and it does exactly what its name says. It is a perfectly valid way to prevent the current entry from being used in the future. See? I object to you removing that, and trying to make it be like ACL_DONT_CACHE is the *onyl* way to not cache something. Because honestly, that's what your comment updates do. They take the comments about _one_ case, and switch it over to be about the _othger_ case. But dammit, there are _two_ ways to not cache things. "Fixing" the comment to talk about one and removing the other isn't a fix. It's just a stupid change that now has the problem the other way around! So fix the comment to really just talk about both things. First: talk about how to avoid caching entirely (ACL_DONT_CACHE). Then, talk about how to invalidate the cache once it has been instantiated (forget_cached_acl()). Don't do this idiotic "remove the valid comment just because you happened to care about the _other_ case" Linus
On Mon, Feb 26, 2018 at 7:14 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > As such I believe that usage of forget_cached_acl should be subsumed by > using ACL_NOT_CACHED. If not we should really come up with a different > helper function name to call from ->get_acl. Preferably one that does > "cmpxchng(p, sentinel, ACL_NOT_CACHED)" so that we remove the races. You make your bias very clear, by simply trying to hide the other case. But for chrissake, that's not the state right now. That other case exists. You can't - and shouldn't - try to just hide it. Besides, that "forget_cached_acl()" approach actually has a valid use case. Maybe you _do_ want to cache ACL's, but with a timeout or revalidation. ACL_DONT_CACHE really is a big hammer that makes caching not work at all. It's not necessarily the right thing to do at all. Linus
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 2fd0fde16fe1..5453094b8828 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -98,6 +98,11 @@ struct posix_acl *get_acl(struct inode *inode, int type) struct posix_acl **p; struct posix_acl *acl; + /* + * To avoid caching the result of ->get_acl + * set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE; + */ + /* * The sentinel is used to detect when another operation like * set_cached_acl() or forget_cached_acl() races with get_acl(). @@ -126,9 +131,7 @@ struct posix_acl *get_acl(struct inode *inode, int type) /* fall through */ ; /* - * Normally, the ACL returned by ->get_acl will be cached. - * A filesystem can prevent that by calling - * forget_cached_acl(inode, type) in ->get_acl. + * The ACL returned by ->get_acl will be cached. * * If the filesystem doesn't have a get_acl() function at all, we'll * just create the negative cache entry.
So the purpose for having a patch in the first place is that 2a3a2a3f3524 ("ovl: don't cache acl on overlay layer") which addded ACL_DONT_CACHED did not result in any comment updates to get_acl. Which mean that if you read the comments in get_acl() that you don't even think of ACL_DONT_CACHED. Which means that this comment: /* * If the ACL isn't being read yet, set our sentinel. Otherwise, the * current value of the ACL will not be ACL_NOT_CACHED and so our own * sentinel will not be set; another task will update the cache. We * could wait for that other task to complete its job, but it's easier * to just call ->get_acl to fetch the ACL ourself. (This is going to * be an unlikely race.) */ Which presumes the only reason the acl could be anything other ACL_NOT_CACHED is because get_acl() is already being called upon it in another task. I wanted something to mention ACL_DONT_CACHED so someone would at least think about that case if they ever step up to modify the code. The code is perfectly clear, the comment is not. That scares me. And I had to read the code about a dozen times before I realized the ACL_DONT_CACHED case even exists. Not useful when I am need to use that to preserve historical fuse semantics. So something is missing here even if my wording does not improve things. Then we get this comment: /* * Normally, the ACL returned by ->get_acl will be cached. * A filesystem can prevent that by calling * forget_cached_acl(inode, type) in ->get_acl. */ Which was added in b8a7a3a66747 ("posix_acl: Inode acl caching fixes") That comment is and always has been rubbish. I don't have a clue what it is trying to say but it is not something a person can use to write filesystem code with. Truths: - forget_cached_acl(inode, type) can be used to invalidate the acl cache. - Calling forget_cached_acl from within the filesystems ->get_acl method won't prevent a cached value from being returend because ->get_acl will be set. - Calling forget_cached_acl from within the filesystems ->get_acl method won't prevent a returned value from being cached because it the caching happens after ->get_acl returns. - Setting inode->i_acl = ACL_DONT_CACHE is the only way to prevent a value from ->get_acl from being cached. In summary I only care about two things. 1) ACL_NOT_CACHED being mentioned somewhere in get_acl so people looking at the code, and people updating the code will have a hint that they need to consider that case. 2) That misleading completely bogus comment being removed/fixed. And yes I agree the code is clear. The comments are not. Does this look better as a comment updating patch? Eric