[PATCH 0/4] POSIX ACL patches for 4.9

[PATCH 0/4] POSIX ACL patches for 4.9

[Andreas Gruenbacher]

Here are some POSIX ACL improvements for the next merge window.  The first
three patches are rather simple.  The fourth patch fixes a potential problem on
filesystems that cache ACLs locally and require network communication for
reading ACLs.  Please review.

Thanks,
Andreas

Alexey Dobriyan (1):
  posix_acl: Better uncached ACL markers

Andreas Gruenbacher (3):
  posix_acl: Improve xattr fixup code
  posix_acl: get_acl: Cleanups
  posix_acl: get_acl: Synchronize readers

 fs/nfs/nfs3acl.c          |  44 ++------------
 fs/posix_acl.c            | 145 ++++++++++++++++++++++++++++++++++------------
 fs/xattr.c                |  29 ++++++++--
 include/linux/fs.h        |   2 +-
 include/linux/posix_acl.h |   4 ++
 include/linux/wait.h      |   1 +
 kernel/sched/wait.c       |   9 +++
 7 files changed, 153 insertions(+), 81 deletions(-)

-- 
2.7.4

[PATCH 1/4] posix_acl: Improve xattr fixup code

[Andreas Gruenbacher]

Both XATTR_NAME_POSIX_ACL_ACCESS and XATTR_NAME_POSIX_ACL_DEFAULT have
the same XATTR_SYSTEM_PREFIX prefix; don't check for the same prefix
repeatedly.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Reviewed-by: Steve French <steve.french@primarydata.com>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
---
 fs/xattr.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index c243905..c037fbb 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -302,6 +302,16 @@ out:
 }
 EXPORT_SYMBOL_GPL(vfs_removexattr);
 
+static void
+fix_xattr_from_user(const char *kname, void *kvalue, size_t size)
+{
+	if (strncmp(kname, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
+		return;
+	kname += XATTR_SYSTEM_PREFIX_LEN;
+	if (!strcmp(kname, XATTR_POSIX_ACL_ACCESS) ||
+	    !strcmp(kname, XATTR_POSIX_ACL_DEFAULT))
+		posix_acl_fix_xattr_from_user(kvalue, size);
+}
 
 /*
  * Extended attribute SET operations
@@ -336,9 +346,7 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
 			error = -EFAULT;
 			goto out;
 		}
-		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
-		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_from_user(kvalue, size);
+		fix_xattr_from_user(kname, kvalue, size);
 	}
 
 	error = vfs_setxattr(d, kname, kvalue, size, flags);
@@ -403,6 +411,17 @@ SYSCALL_DEFINE5(fsetxattr, int, fd, const char __user *, name,
 	return error;
 }
 
+static void
+fix_xattr_to_user(const char *kname, void *kvalue, size_t size)
+{
+	if (strncmp(kname, XATTR_SYSTEM_PREFIX, XATTR_SYSTEM_PREFIX_LEN))
+		return;
+	kname += XATTR_SYSTEM_PREFIX_LEN;
+	if (!strcmp(kname, XATTR_POSIX_ACL_ACCESS) ||
+	    !strcmp(kname, XATTR_POSIX_ACL_DEFAULT))
+		posix_acl_fix_xattr_to_user(kvalue, size);
+}
+
 /*
  * Extended attribute GET operations
  */
@@ -433,9 +452,7 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
 
 	error = vfs_getxattr(d, kname, kvalue, size);
 	if (error > 0) {
-		if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
-		    (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
-			posix_acl_fix_xattr_to_user(kvalue, size);
+		fix_xattr_to_user(kname, kvalue, size);
 		if (size && copy_to_user(value, kvalue, error))
 			error = -EFAULT;
 	} else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
-- 
2.7.4

[PATCH 2/4] posix_acl: Better uncached ACL markers

[Andreas Gruenbacher]

From: Alexey Dobriyan <adobriyan@gmail.com>

Instead of adding one to the pointer to set bit 0, use bit-or; this
produces minimally better code.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 include/linux/fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3523bf6..64fc2ee 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -578,7 +578,7 @@ struct posix_acl;
 static inline struct posix_acl *
 uncached_acl_sentinel(struct task_struct *task)
 {
-	return (void *)task + 1;
+	return (struct posix_acl *)((unsigned long)task | 1);
 }
 
 static inline bool
-- 
2.7.4

[PATCH 3/4] posix_acl: get_acl: Cleanups

[Andreas Gruenbacher]

Move the prepare_get_acl, complete_get_acl, and abort_get_acl helpers
from fs/nfs/nfs3acl.c to fs/posix_acl.c: using the same helpers in
get_acl and in nfs3_get_acl makes the code easier to read.

In get_acl, move the check for NULL get_acl inode operations above
__prepare_get_acl.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/nfs/nfs3acl.c          |  44 +++---------------
 fs/posix_acl.c            | 116 ++++++++++++++++++++++++++++++++--------------
 include/linux/posix_acl.h |   4 ++
 3 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index 720d92f5..b623801 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -11,38 +11,6 @@
 
 #define NFSDBG_FACILITY	NFSDBG_PROC
 
-/*
- * nfs3_prepare_get_acl, nfs3_complete_get_acl, nfs3_abort_get_acl: Helpers for
- * caching get_acl results in a race-free way.  See fs/posix_acl.c:get_acl()
- * for explanations.
- */
-static void nfs3_prepare_get_acl(struct posix_acl **p)
-{
-	struct posix_acl *sentinel = uncached_acl_sentinel(current);
-
-	if (cmpxchg(p, ACL_NOT_CACHED, sentinel) != ACL_NOT_CACHED) {
-		/* Not the first reader or sentinel already in place. */
-	}
-}
-
-static void nfs3_complete_get_acl(struct posix_acl **p, struct posix_acl *acl)
-{
-	struct posix_acl *sentinel = uncached_acl_sentinel(current);
-
-	/* Only cache the ACL if our sentinel is still in place. */
-	posix_acl_dup(acl);
-	if (cmpxchg(p, sentinel, acl) != sentinel)
-		posix_acl_release(acl);
-}
-
-static void nfs3_abort_get_acl(struct posix_acl **p)
-{
-	struct posix_acl *sentinel = uncached_acl_sentinel(current);
-
-	/* Remove our sentinel upon failure. */
-	cmpxchg(p, sentinel, ACL_NOT_CACHED);
-}
-
 struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 {
 	struct nfs_server *server = NFS_SERVER(inode);
@@ -88,9 +56,9 @@ struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 		return ERR_PTR(-ENOMEM);
 
 	if (args.mask & NFS_ACL)
-		nfs3_prepare_get_acl(&inode->i_acl);
+		prepare_get_acl(inode, ACL_TYPE_ACCESS);
 	if (args.mask & NFS_DFACL)
-		nfs3_prepare_get_acl(&inode->i_default_acl);
+		prepare_get_acl(inode, ACL_TYPE_DEFAULT);
 
 	status = rpc_call_sync(server->client_acl, &msg, 0);
 	dprintk("NFS reply getacl: %d\n", status);
@@ -126,12 +94,12 @@ struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 	}
 
 	if (res.mask & NFS_ACL)
-		nfs3_complete_get_acl(&inode->i_acl, res.acl_access);
+		complete_get_acl(inode, ACL_TYPE_ACCESS, res.acl_access);
 	else
 		forget_cached_acl(inode, ACL_TYPE_ACCESS);
 
 	if (res.mask & NFS_DFACL)
-		nfs3_complete_get_acl(&inode->i_default_acl, res.acl_default);
+		complete_get_acl(inode, ACL_TYPE_DEFAULT, res.acl_default);
 	else
 		forget_cached_acl(inode, ACL_TYPE_DEFAULT);
 
@@ -145,8 +113,8 @@ struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 	}
 
 getout:
-	nfs3_abort_get_acl(&inode->i_acl);
-	nfs3_abort_get_acl(&inode->i_default_acl);
+	abort_get_acl(inode, ACL_TYPE_ACCESS);
+	abort_get_acl(inode, ACL_TYPE_DEFAULT);
 	posix_acl_release(res.acl_access);
 	posix_acl_release(res.acl_default);
 	nfs_free_fattr(res.fattr);
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 59d47ab0..0be840e 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -91,18 +91,80 @@ void forget_all_cached_acls(struct inode *inode)
 }
 EXPORT_SYMBOL(forget_all_cached_acls);
 
+static bool __prepare_get_acl(struct posix_acl **p)
+{
+	struct posix_acl *sentinel = uncached_acl_sentinel(current);
+
+	return cmpxchg(p, ACL_NOT_CACHED, sentinel) == ACL_NOT_CACHED;
+}
+
+bool prepare_get_acl(struct inode *inode, int type)
+{
+	return __prepare_get_acl(acl_by_type(inode, type));
+}
+EXPORT_SYMBOL(prepare_get_acl);
+
+/* Only cache the ACL if our sentinel is still in place. */
+static void __complete_get_acl(struct posix_acl **p, struct posix_acl *acl)
+{
+	struct posix_acl *sentinel = uncached_acl_sentinel(current);
+
+	posix_acl_dup(acl);
+	if (cmpxchg(p, sentinel, acl) != sentinel)
+		posix_acl_release(acl);
+}
+
+void complete_get_acl(struct inode *inode, int type, struct posix_acl *acl)
+{
+	return __complete_get_acl(acl_by_type(inode, type), acl);
+}
+EXPORT_SYMBOL(complete_get_acl);
+
+/* Remove our sentinel upon failure. */
+/*
+ * Remove our sentinel so that we don't block future attempts
+ * to cache the ACL.
+ */
+static void __abort_get_acl(struct posix_acl **p)
+{
+	struct posix_acl *sentinel = uncached_acl_sentinel(current);
+
+	cmpxchg(p, sentinel, ACL_NOT_CACHED);
+}
+
+void abort_get_acl(struct inode *inode, int type)
+{
+	__abort_get_acl(acl_by_type(inode, type));
+}
+EXPORT_SYMBOL(abort_get_acl);
+
+
+/**
+ * get_acl  -  Get the ACL of an inode
+ *
+ * This function is called without holding the inode mutex, so when fetching an
+ * uncached ACL from the filesystem, we can race with other get_acl or set_acl
+ * requests.  We must make sure not to override ACLs set with set_acl in the
+ * meantime.
+ *
+ * To do that, we change ->i_acl (or ->i_default_acl) from ACL_NOT_CACHED to a
+ * sentinel value that indicates that get_acl is in progress (prepare_get_acl).
+ * For these sentinel values, is_uncached_acl(sentinel) is always true.
+ *
+ * When we get back an ACL from the filesystem, we update the cached ACL only
+ * if our sentinel value is still in place ( complete_get_acl).
+ *
+ * A competing set_acl will replace our sentinel with the new ACL, and
+ * complete_get_acl will leave that new ACL untouched.
+ *
+ * When get_acl fails, we remove our sentinel so that future attempts to cache
+ * the ACL can succeed (abort_get_acl).
+ */
 struct posix_acl *get_acl(struct inode *inode, int type)
 {
-	void *sentinel;
 	struct posix_acl **p;
 	struct posix_acl *acl;
 
-	/*
-	 * 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.
-	 */
-
 	acl = get_cached_acl(inode, type);
 	if (!is_uncached_acl(acl))
 		return acl;
@@ -110,25 +172,7 @@ struct posix_acl *get_acl(struct inode *inode, int type)
 	if (!IS_POSIXACL(inode))
 		return NULL;
 
-	sentinel = uncached_acl_sentinel(current);
-	p = acl_by_type(inode, type);
-
 	/*
-	 * 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.)
-	 */
-	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.
-	 *
 	 * If the filesystem doesn't have a get_acl() function at all, we'll
 	 * just create the negative cache entry.
 	 */
@@ -136,23 +180,23 @@ struct posix_acl *get_acl(struct inode *inode, int type)
 		set_cached_acl(inode, type, NULL);
 		return NULL;
 	}
+
+	p = acl_by_type(inode, type);
+	__prepare_get_acl(p);
+
+	/*
+	 * 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.
+	 */
 	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);
+		__abort_get_acl(p);
 		return acl;
 	}
 
-	/*
-	 * Cache the result, but only if our sentinel is still in place.
-	 */
-	posix_acl_dup(acl);
-	if (unlikely(cmpxchg(p, sentinel, acl) != sentinel))
-		posix_acl_release(acl);
+	__complete_get_acl(p, acl);
 	return acl;
 }
 EXPORT_SYMBOL(get_acl);
diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h
index d5d3d74..b150e76 100644
--- a/include/linux/posix_acl.h
+++ b/include/linux/posix_acl.h
@@ -136,6 +136,10 @@ static inline void forget_all_cached_acls(struct inode *inode)
 }
 #endif /* CONFIG_FS_POSIX_ACL */
 
+bool prepare_get_acl(struct inode *, int);
+void complete_get_acl(struct inode *, int, struct posix_acl *);
+void abort_get_acl(struct inode *, int);
+
 struct posix_acl *get_acl(struct inode *inode, int type);
 
 #endif  /* __LINUX_POSIX_ACL_H */
-- 
2.7.4

[PATCH 4/4] posix_acl: get_acl: Synchronize readers

[Andreas Gruenbacher]

Currently, while an ACL is not cached yet, the initial caller to get_acl
is responsible for reading the ACL and updating the cache; any
concurrent readers only read the ACL without updating the cache.  This
works reasonably well for local filesystems where reading ACLs is fast
once the underlying disk blocks are cached, but not so well for
filesystems where reading an ACL requires network communication, for
example.

To address that, make subsequent concurrent readers wait for the initial
reader instead.  Any blocked readers are woken up as soon as an ACL is
available (or the initial read has failed).

(Filesystems can still force the VFS not to cache ACLs by calling
forget_cached_acl() in ->get_acl.  This will allow any concurrent
readers waiting for the initial reader to proceed.  As of now, only
Lustre is doing that.)

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Cc: Oleg Drokin <oleg.drokin@intel.com>
Cc: Andreas Dilger <andreas.dilger@intel.com>
---
 fs/posix_acl.c       | 37 +++++++++++++++++++++++++++++++++----
 include/linux/wait.h |  1 +
 kernel/sched/wait.c  |  9 +++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 0be840e..c02c7ca 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -58,6 +58,11 @@ struct posix_acl *get_cached_acl_rcu(struct inode *inode, int type)
 }
 EXPORT_SYMBOL(get_cached_acl_rcu);
 
+static bool is_uncached_acl_sentinel(struct posix_acl *acl)
+{
+	return is_uncached_acl(acl) && acl != ACL_NOT_CACHED;
+}
+
 void set_cached_acl(struct inode *inode, int type, struct posix_acl *acl)
 {
 	struct posix_acl **p = acl_by_type(inode, type);
@@ -66,6 +71,8 @@ void set_cached_acl(struct inode *inode, int type, struct posix_acl *acl)
 	old = xchg(p, posix_acl_dup(acl));
 	if (!is_uncached_acl(old))
 		posix_acl_release(old);
+	else if (is_uncached_acl_sentinel(old))
+		wake_up_all(generic_waitqueue(p));
 }
 EXPORT_SYMBOL(set_cached_acl);
 
@@ -76,6 +83,8 @@ static void __forget_cached_acl(struct posix_acl **p)
 	old = xchg(p, ACL_NOT_CACHED);
 	if (!is_uncached_acl(old))
 		posix_acl_release(old);
+	else if (is_uncached_acl_sentinel(old))
+		wake_up_all(generic_waitqueue(p));
 }
 
 void forget_cached_acl(struct inode *inode, int type)
@@ -110,7 +119,9 @@ static void __complete_get_acl(struct posix_acl **p, struct posix_acl *acl)
 	struct posix_acl *sentinel = uncached_acl_sentinel(current);
 
 	posix_acl_dup(acl);
-	if (cmpxchg(p, sentinel, acl) != sentinel)
+	if (cmpxchg(p, sentinel, acl) == sentinel)
+		wake_up_all(generic_waitqueue(p));
+	else
 		posix_acl_release(acl);
 }
 
@@ -129,7 +140,8 @@ static void __abort_get_acl(struct posix_acl **p)
 {
 	struct posix_acl *sentinel = uncached_acl_sentinel(current);
 
-	cmpxchg(p, sentinel, ACL_NOT_CACHED);
+	if (cmpxchg(p, sentinel, ACL_NOT_CACHED) == sentinel)
+		wake_up_all(generic_waitqueue(p));
 }
 
 void abort_get_acl(struct inode *inode, int type)
@@ -165,6 +177,7 @@ struct posix_acl *get_acl(struct inode *inode, int type)
 	struct posix_acl **p;
 	struct posix_acl *acl;
 
+repeat:
 	acl = get_cached_acl(inode, type);
 	if (!is_uncached_acl(acl))
 		return acl;
@@ -182,12 +195,28 @@ struct posix_acl *get_acl(struct inode *inode, int type)
 	}
 
 	p = acl_by_type(inode, type);
-	__prepare_get_acl(p);
+	if (!__prepare_get_acl(p)) {
+		wait_queue_head_t *wq = generic_waitqueue(p);
+		DEFINE_WAIT(wait);
+
+		for(;;) {
+			prepare_to_wait(wq, &wait, TASK_INTERRUPTIBLE);
+			smp_mb();
+			if (!is_uncached_acl_sentinel(*p))
+				break;
+			io_schedule();
+		}
+		finish_wait(wq, &wait);
+		if (signal_pending(current))
+			return ERR_PTR(-ERESTARTSYS);
+		goto repeat;
+	}
 
 	/*
 	 * 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.
+	 * forget_cached_acl(inode, type) in ->get_acl, preferably
+	 * early in ->get_acl to avoid serializing concurrent readers.
 	 */
 	acl = inode->i_op->get_acl(inode, type);
 
diff --git a/include/linux/wait.h b/include/linux/wait.h
index c3ff74d..adfeb3c 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -212,6 +212,7 @@ int out_of_line_wait_on_bit_timeout(void *, int, wait_bit_action_f *, unsigned,
 int out_of_line_wait_on_bit_lock(void *, int, wait_bit_action_f *, unsigned);
 int out_of_line_wait_on_atomic_t(atomic_t *, int (*)(atomic_t *), unsigned);
 wait_queue_head_t *bit_waitqueue(void *, int);
+wait_queue_head_t *generic_waitqueue(void *);
 
 #define wake_up(x)			__wake_up(x, TASK_NORMAL, 1, NULL)
 #define wake_up_nr(x, nr)		__wake_up(x, TASK_NORMAL, nr, NULL)
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index f15d6b6..baf2700 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -493,6 +493,15 @@ wait_queue_head_t *bit_waitqueue(void *word, int bit)
 }
 EXPORT_SYMBOL(bit_waitqueue);
 
+wait_queue_head_t *generic_waitqueue(void *ptr)
+{
+	const struct zone *zone = page_zone(virt_to_page(ptr));
+	unsigned long val = (unsigned long)ptr;
+
+	return &zone->wait_table[hash_long(val, zone->wait_table_bits)];
+}
+EXPORT_SYMBOL(generic_waitqueue);
+
 /*
  * Manipulate the atomic_t address to produce a better bit waitqueue table hash
  * index (we're keying off bit -1, but that would produce a horrible hash
-- 
2.7.4