Re: [PATCH v2] ceph: fix posix ACL hooks

[Linus Torvalds]

On Wed, Jan 29, 2014 at 8:37 AM, Ilya Dryomov <ilya.dryomov@xxxxxxxxxxx> wrote:
> From: Sage Weil <sage@xxxxxxxxxxx>
>
> The merge of 7221fe4c2 raced with upstream changes in the generic POSIX
> ACL code (2aeccbe95).  Update Ceph to use the new helpers as well by
> dropping the now-generic functions and setting the set_acl inode op.
>
> Signed-off-by: Sage Weil <sage@xxxxxxxxxxx>

Ok, I already committed my untested (and broken) patch yesterday,
because even a broken tree is better than a non-*compiling* tree for
testing.

I created a diff from my current tree to this, and will happily apply
it, but the sign-off chain is now broken (Ilya didn't sign off on his
changes to Sage's patch. Not that it really matters for what is really
a one-liner change, but I thought I'd mention it, since another issue
came up with this patch..

Ceph now does

        struct dentry *dentry = d_find_alias(inode);

in its ceph_set_acl() function, and that's because the VFS layer
doesn't pass down the dentry to the acl code any more.

Christoph - that sounds like a misfeature in the new interfaces. I
realize that for traditional unix filesystems, the path is irrelevant
for an inode operation, but the thing is, from a Linux VFS standpoint
and a conceptual standpoint, the dentry really is the more important
and unique one, and some filesystems want the dentry because they are
*correctly* designed to be about pathnames.

Network filesystems that are pass file descriptors around (ie NFS etc)
are not the "RightWay(tm)" of doing things, so I think that it is
quite reasonable for ceph to want the *path* to the inode and not just
the inode.

So attached is the incremental diff of the patch by Sage and Ilya, and
I'll apply it (delayed a bit to see if I can get the sign-off from
Ilya), but I also think we should fix the (non-cached) ACL functions
that call down to the filesystem layer to also get the dentry.

We already do that for the xattr functions, just not for
get_acl()/set_acl()/acl_chmod().

Christoph, Al? Yes, most filesystems will ignore the dentry pointer, but still..

               Linus
 fs/ceph/acl.c   | 9 ++++-----
 fs/ceph/dir.c   | 1 +
 fs/ceph/inode.c | 2 ++
 fs/ceph/super.h | 3 +++
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
index f6911284c9bd..66d377a12f7c 100644
--- a/fs/ceph/acl.c
+++ b/fs/ceph/acl.c
@@ -107,14 +107,14 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type)
 	return acl;
 }
 
-static int ceph_set_acl(struct dentry *dentry, struct inode *inode,
-				struct posix_acl *acl, int type)
+int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	int ret = 0, size = 0;
 	const char *name = NULL;
 	char *value = NULL;
 	struct iattr newattrs;
 	umode_t new_mode = inode->i_mode, old_mode = inode->i_mode;
+	struct dentry *dentry = d_find_alias(inode);
 
 	if (acl) {
 		ret = posix_acl_valid(acl);
@@ -208,8 +208,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir)
 
 	if (IS_POSIXACL(dir) && acl) {
 		if (S_ISDIR(inode->i_mode)) {
-			ret = ceph_set_acl(dentry, inode, acl,
-						ACL_TYPE_DEFAULT);
+			ret = ceph_set_acl(inode, acl, ACL_TYPE_DEFAULT);
 			if (ret)
 				goto out_release;
 		}
@@ -217,7 +216,7 @@ int ceph_init_acl(struct dentry *dentry, struct inode *inode, struct inode *dir)
 		if (ret < 0)
 			goto out;
 		else if (ret > 0)
-			ret = ceph_set_acl(dentry, inode, acl, ACL_TYPE_ACCESS);
+			ret = ceph_set_acl(inode, acl, ACL_TYPE_ACCESS);
 		else
 			cache_no_acl(inode);
 	} else {
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 619616d585b0..6da4df84ba30 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1303,6 +1303,7 @@ const struct inode_operations ceph_dir_iops = {
 	.listxattr = ceph_listxattr,
 	.removexattr = ceph_removexattr,
 	.get_acl = ceph_get_acl,
+	.set_acl = ceph_set_acl,
 	.mknod = ceph_mknod,
 	.symlink = ceph_symlink,
 	.mkdir = ceph_mkdir,
diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
index 8b8b506636cc..32d519d8a2e2 100644
--- a/fs/ceph/inode.c
+++ b/fs/ceph/inode.c
@@ -97,6 +97,7 @@ const struct inode_operations ceph_file_iops = {
 	.listxattr = ceph_listxattr,
 	.removexattr = ceph_removexattr,
 	.get_acl = ceph_get_acl,
+	.set_acl = ceph_set_acl,
 };
 
 
@@ -1616,6 +1617,7 @@ static const struct inode_operations ceph_symlink_iops = {
 	.listxattr = ceph_listxattr,
 	.removexattr = ceph_removexattr,
 	.get_acl = ceph_get_acl,
+	.set_acl = ceph_set_acl,
 };
 
 /*
diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 345933948b6e..aa260590f615 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -718,6 +718,7 @@ extern void ceph_queue_writeback(struct inode *inode);
 extern int ceph_do_getattr(struct inode *inode, int mask);
 extern int ceph_permission(struct inode *inode, int mask);
 extern int ceph_setattr(struct dentry *dentry, struct iattr *attr);
+extern int ceph_setattr(struct dentry *dentry, struct iattr *attr);
 extern int ceph_getattr(struct vfsmount *mnt, struct dentry *dentry,
 			struct kstat *stat);
 
@@ -741,12 +742,14 @@ extern const struct xattr_handler *ceph_xattr_handlers[];
 #ifdef CONFIG_CEPH_FS_POSIX_ACL
 
 struct posix_acl *ceph_get_acl(struct inode *, int);
+int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type);
 int ceph_init_acl(struct dentry *, struct inode *, struct inode *);
 void ceph_forget_all_cached_acls(struct inode *inode);
 
 #else
 
 #define ceph_get_acl NULL
+#define ceph_set_acl NULL
 
 static inline int ceph_init_acl(struct dentry *dentry, struct inode *inode,
 				struct inode *dir)