| Message ID | 20180226235302.12708-3-ebiederm@xmission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 26, 2018 at 3:52 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Additionaly update the comment above the call to get_acl itself and > remove the wrong information that an implementation of get_acl can > prevent caching by calling forget_cached_acl. This part is just confusing. First off, that comment is correct: a filesystem _can_ prevent the returning of cached data by just calling forget_cached_acl(). Note that there are two different cases: saying that you _never_ want to cache things (ACL_DONT_CACHE) and saying that there _currently_ is no cached data (ACL_NOT_CACHED). forget_cached_acl() just removes the current cache. You're just replacing one case of "no cached" information with the other. Just explain the two cases, don't try to muddy the waters even more.. PLUS you are just confusing things entirely. That whole new comment of yours: + * ACL_DONT_CACHE is treated as another task updating the acl and + * remains set. is just garbage. The code is very clear - it will only replace a ACL_NOT_CACHED entry. The code is clear: if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) /* fall through */ ; this is basically just an atomic "if *p == ACL_NOT_CACHED then replace it with 'sentinel'". Your comment does not add any clarity at all, and only confuses things. It has nothing to do with "treated as another task updating the acl". The fact is, ACL_DONT_CACHE is treated as if the cache is simply already filled - it's just filled with "no cache". So the only thing special is ACL_NOT_CACHED, which is the only thing we will try to _replace_. So NAK on this patch entirely. It's just adding confusion, not adding clarifications. Linus
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 2fd0fde16fe1..3c24fc263401 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -121,14 +121,17 @@ struct posix_acl *get_acl(struct inode *inode, int type) * 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.) + * + * ACL_DONT_CACHE is treated as another task updating the acl and + * remains set. */ if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) /* 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. + * A filesystem can prevent that by calling setting + * inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE. * * If the filesystem doesn't have a get_acl() function at all, we'll * just create the negative cache entry.
Fuse is about to join overlayfs in relying on get_acl respecting ACL_DONT_CACHE so update the documentation in get_acl to reflect that fact. The comment and this change description should give people a clue that respecting ACL_DONT_CACHE in get_acl is important, and they should audit the filesystems before removing that support. Additionaly update the comment above the call to get_acl itself and remove the wrong information that an implementation of get_acl can prevent caching by calling forget_cached_acl. Replace that with the correct information that to prevent caching all that is necessary is to set inode->i_acl = inode->i_default_acl = ACL_DONT_CACHE when the inode is initialized. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/posix_acl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)