[v8,1/6] fs/posix_acl: Update the comments and support lightweight cache skipping

Submitted by Eric W. Biederman on March 2, 2018, 9:59 p.m.

Details

Message ID 20180302215919.27207-1-ebiederm@xmission.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Eric W. Biederman March 2, 2018, 9:59 p.m.
The code has been missing a way for a ->get_acl method to not cache
a return value without risking invalidating a cached value
that was set while get_acl() was returning.

Add that support by implementing to_uncachable_acl, to_cachable_acl,
is_uncacheable_acl, and dealing with uncachable acls in get_acl().

Update the comments so that they are a little clearer about
what is going on in get_acl()

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/posix_acl.c            | 50 ++++++++++++++++++++++++++++++++---------------
 include/linux/posix_acl.h | 17 ++++++++++++++++
 2 files changed, 51 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 2fd0fde16fe1..00281bc30854 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -96,12 +96,16 @@  struct posix_acl *get_acl(struct inode *inode, int type)
 {
 	void *sentinel;
 	struct posix_acl **p;
-	struct posix_acl *acl;
+	struct posix_acl *acl, *to_cache;
 
 	/*
 	 * The sentinel is used to detect when another operation like
 	 * set_cached_acl() or forget_cached_acl() races with get_acl().
 	 * It is guaranteed that is_uncached_acl(sentinel) is true.
+	 *
+	 * This is sufficient to prevent races between ->set_acl
+	 * calling set_cached_acl (outside of filesystem specific
+	 * locking) and get_acl() caching the returned acl.
 	 */
 
 	acl = get_cached_acl(inode, type);
@@ -126,12 +130,18 @@  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.
+	 * Normally, the ACL returned by ->get_acl() will be cached.
+	 *
+	 * A filesystem can prevent the acl returned by ->get_acl()
+	 * from being cached by wrapping it with to_uncachable_acl().
 	 *
-	 * If the filesystem doesn't have a get_acl() function at all, we'll
-	 * just create the negative cache entry.
+	 * A filesystem can at anytime effect the cache directly and
+	 * cause in process calls of get_acl() not to update the cache
+	 * by calling forget_cache_acl(inode, type) or
+	 * set_cached_acl(inode, type, acl).
+	 *
+	 * If the filesystem doesn't have a ->get_acl() function at
+	 * all, we'll just create the negative cache entry.
 	 */
 	if (!inode->i_op->get_acl) {
 		set_cached_acl(inode, type, NULL);
@@ -140,20 +150,28 @@  struct posix_acl *get_acl(struct inode *inode, int type)
 	acl = inode->i_op->get_acl(inode, type);
 
 	if (IS_ERR(acl)) {
-		/*
-		 * Remove our sentinel so that we don't block future attempts
-		 * to cache the ACL.
-		 */
-		cmpxchg(p, sentinel, ACL_NOT_CACHED);
-		return acl;
+		/* Don't cache an acl just return an error. */
+		to_cache = ACL_NOT_CACHED;
+	}
+	else if (is_uncacheable_acl(acl)) {
+		/* Don't cache an acl, but return one. */
+		to_cache = ACL_NOT_CACHED;
+		acl = to_cacheable_acl(acl);
+	}
+	else {
+		/* Cache and return the acl. */
+		to_cache = posix_acl_dup(acl);
 	}
 
 	/*
-	 * Cache the result, but only if our sentinel is still in place.
+	 * Remove the sentinel and replace it with the value to
+	 * cache, but only if the sentinel is still in place.
 	 */
-	posix_acl_dup(acl);
-	if (unlikely(cmpxchg(p, sentinel, acl) != sentinel))
-		posix_acl_release(acl);
+	if (unlikely(cmpxchg(p, sentinel, to_cache) != sentinel)) {
+		if (!is_uncached_acl(to_cache))
+			posix_acl_release(to_cache);
+	}
+
 	return acl;
 }
 EXPORT_SYMBOL(get_acl);
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index 540595a321a7..3be8929b9f48 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -56,6 +56,23 @@  posix_acl_release(struct posix_acl *acl)
 		kfree_rcu(acl, a_rcu);
 }
 
+/*
+ * Allow for acls returned from ->get_acl() to not be cached.
+ */
+static inline bool is_uncacheable_acl(struct posix_acl *acl)
+{
+	return ((unsigned long)acl) & 1UL;
+}
+
+static inline struct posix_acl *to_uncacheable_acl(struct posix_acl *acl)
+{
+	return (struct posix_acl *)(((unsigned long)acl) | 1UL);
+}
+
+static inline struct posix_acl *to_cacheable_acl(struct posix_acl *acl)
+{
+	return (struct posix_acl *)(((unsigned long)acl) & ~1UL);
+}
 
 /* posix_acl.c */
 

Comments

Miklos Szeredi March 5, 2018, 9:53 a.m.
On Fri, Mar 2, 2018 at 10:59 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> The code has been missing a way for a ->get_acl method to not cache
> a return value without risking invalidating a cached value
> that was set while get_acl() was returning.
>
> Add that support by implementing to_uncachable_acl, to_cachable_acl,
> is_uncacheable_acl, and dealing with uncachable acls in get_acl().

I don't like the pointer magic here.  Can't the uncachable bit just be
added to struct posix_acl?

 AFAICS that can be done even without increasing the size of that
struct (e.g. by unioning it with the rcu_head).

Thanks,
Miklos
Eric W. Biederman March 5, 2018, 1:53 p.m.
Miklos Szeredi <mszeredi@redhat.com> writes:

> On Fri, Mar 2, 2018 at 10:59 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> The code has been missing a way for a ->get_acl method to not cache
>> a return value without risking invalidating a cached value
>> that was set while get_acl() was returning.
>>
>> Add that support by implementing to_uncachable_acl, to_cachable_acl,
>> is_uncacheable_acl, and dealing with uncachable acls in get_acl().
>
> I don't like the pointer magic here.  Can't the uncachable bit just be
> added to struct posix_acl?
>
>  AFAICS that can be done even without increasing the size of that
> struct (e.g. by unioning it with the rcu_head).

Except that would:
- add a possible cache line miss.
- make it unusable for overlayfs.

I am after very light-weight semantics that say don't cache this return
value but don't have any effects elsewhere.

We are already playing pointer magic games in this code.  This just uses
those games for the last piece of information to keep the logic clean.

I see two possible implementation alternatives:
- Make get_acl return a struct that returns the acl and cachability flag
- Add a helper that does"cmpxchg(p, sentinel, ACL_NOT_CACHED)".
  Such a heleper function seems like a waste, it does side effect magic
  which is never particularly pleasant, and it is more code to execute
  in practice.  Though honestly it is my second choice.

  void dont_cache_my_return_acl(struct inode *inode, int type)
  {
    	/* Valid only inside ->get_acl implementations */
        struct posix_acl **p = get_acl_type(inode, type);
        struct posix_acl *sentinel = uncached_acl_sentinel(current);
        cmpxchg(p, sentinel, ACL_NOT_CACHED);
  }
  EXPORT_SYMBOL(dont_cache_my_return_acl);

  It is just a few instructions more so I guess it isn't that bad.
  Especially for something that is not a common case.

Do you think you could live with dont_cache_my_return_acl?

Otherwise I think I will respin this patch set without the acl
unification.  There is plenty of evidence what it will look like
now.  We can deal with the rest of the patches.  Then we can come back
to exactly what acl unification in fuse should look like.

Eric