[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0009-f2fs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131201/a3947f13/attachment.ksh>

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Jaegeuk Kim]

2013-12-01 (?), 03:59 -0800, Christoph Hellwig:

> f2fs has some weird mode bit handling, so still using the old
> chmod code for now.

f2fs caches a new mode bit for a while to make the consistency between
xattr's acl mode and the inode mode.
Anyway, it's a very good job.
Thanks,

You can add:
Reviewed-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/f2fs/acl.c   |  140 +++++++++----------------------------------------------
>  fs/f2fs/acl.h   |    1 +
>  fs/f2fs/file.c  |    1 +
>  fs/f2fs/namei.c |    2 +
>  fs/f2fs/xattr.c |    9 ++--
>  fs/f2fs/xattr.h |    2 -
>  6 files changed, 30 insertions(+), 125 deletions(-)
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 45e8430..4f52fe0f 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -205,7 +205,7 @@ struct posix_acl *f2fs_get_acl(struct inode *inode, int type)
>  	return acl;
>  }
>  
> -static int f2fs_set_acl(struct inode *inode, int type,
> +static int __f2fs_set_acl(struct inode *inode, int type,
>  			struct posix_acl *acl, struct page *ipage)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> @@ -261,37 +261,32 @@ static int f2fs_set_acl(struct inode *inode, int type,
>  	return error;
>  }
>  
> +int f2fs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
> +{
> +	return __f2fs_set_acl(inode, type, acl, NULL);
> +}
> +
>  int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage)
>  {
> -	struct f2fs_sb_info *sbi = F2FS_SB(dir->i_sb);
> -	struct posix_acl *acl = NULL;
> +	struct posix_acl *default_acl, *acl;
>  	int error = 0;
>  
> -	if (!S_ISLNK(inode->i_mode)) {
> -		if (test_opt(sbi, POSIX_ACL)) {
> -			acl = f2fs_get_acl(dir, ACL_TYPE_DEFAULT);
> -			if (IS_ERR(acl))
> -				return PTR_ERR(acl);
> -		}
> -		if (!acl)
> -			inode->i_mode &= ~current_umask();
> -	}
> -
> -	if (!test_opt(sbi, POSIX_ACL) || !acl)
> -		goto cleanup;
> +	error = posix_acl_create(dir, &inode->i_mode, &default_acl, &acl);
> +	if (error)
> +		return error;
>  
> -	if (S_ISDIR(inode->i_mode)) {
> -		error = f2fs_set_acl(inode, ACL_TYPE_DEFAULT, acl, ipage);
> +	if (default_acl) {
> +		error = __f2fs_set_acl(inode, ACL_TYPE_DEFAULT, default_acl,
> +				       ipage);
> +		posix_acl_release(default_acl);
> +	}
> +	if (acl) {
>  		if (error)
> -			goto cleanup;
> +			error = __f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl,
> +					       ipage);
> +		posix_acl_release(acl);
>  	}
> -	error = __posix_acl_create(&acl, GFP_KERNEL, &inode->i_mode);
> -	if (error < 0)
> -		return error;
> -	if (error > 0)
> -		error = f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, ipage);
> -cleanup:
> -	posix_acl_release(acl);
> +
>  	return error;
>  }
>  
> @@ -315,100 +310,7 @@ int f2fs_acl_chmod(struct inode *inode)
>  	if (error)
>  		return error;
>  
> -	error = f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, NULL);
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -static size_t f2fs_xattr_list_acl(struct dentry *dentry, char *list,
> -		size_t list_size, const char *name, size_t name_len, int type)
> -{
> -	struct f2fs_sb_info *sbi = F2FS_SB(dentry->d_sb);
> -	const char *xname = POSIX_ACL_XATTR_DEFAULT;
> -	size_t size;
> -
> -	if (!test_opt(sbi, POSIX_ACL))
> -		return 0;
> -
> -	if (type == ACL_TYPE_ACCESS)
> -		xname = POSIX_ACL_XATTR_ACCESS;
> -
> -	size = strlen(xname) + 1;
> -	if (list && size <= list_size)
> -		memcpy(list, xname, size);
> -	return size;
> -}
> -
> -static int f2fs_xattr_get_acl(struct dentry *dentry, const char *name,
> -		void *buffer, size_t size, int type)
> -{
> -	struct f2fs_sb_info *sbi = F2FS_SB(dentry->d_sb);
> -	struct posix_acl *acl;
> -	int error;
> -
> -	if (strcmp(name, "") != 0)
> -		return -EINVAL;
> -	if (!test_opt(sbi, POSIX_ACL))
> -		return -EOPNOTSUPP;
> -
> -	acl = f2fs_get_acl(dentry->d_inode, type);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl)
> -		return -ENODATA;
> -	error = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> -	posix_acl_release(acl);
> -
> -	return error;
> -}
> -
> -static int f2fs_xattr_set_acl(struct dentry *dentry, const char *name,
> -		const void *value, size_t size, int flags, int type)
> -{
> -	struct f2fs_sb_info *sbi = F2FS_SB(dentry->d_sb);
> -	struct inode *inode = dentry->d_inode;
> -	struct posix_acl *acl = NULL;
> -	int error;
> -
> -	if (strcmp(name, "") != 0)
> -		return -EINVAL;
> -	if (!test_opt(sbi, POSIX_ACL))
> -		return -EOPNOTSUPP;
> -	if (!inode_owner_or_capable(inode))
> -		return -EPERM;
> -
> -	if (value) {
> -		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -		if (IS_ERR(acl))
> -			return PTR_ERR(acl);
> -		if (acl) {
> -			error = posix_acl_valid(acl);
> -			if (error)
> -				goto release_and_out;
> -		}
> -	} else {
> -		acl = NULL;
> -	}
> -
> -	error = f2fs_set_acl(inode, type, acl, NULL);
> -
> -release_and_out:
> +	error = __f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, NULL);
>  	posix_acl_release(acl);
>  	return error;
>  }
> -
> -const struct xattr_handler f2fs_xattr_acl_default_handler = {
> -	.prefix = POSIX_ACL_XATTR_DEFAULT,
> -	.flags = ACL_TYPE_DEFAULT,
> -	.list = f2fs_xattr_list_acl,
> -	.get = f2fs_xattr_get_acl,
> -	.set = f2fs_xattr_set_acl,
> -};
> -
> -const struct xattr_handler f2fs_xattr_acl_access_handler = {
> -	.prefix = POSIX_ACL_XATTR_ACCESS,
> -	.flags = ACL_TYPE_ACCESS,
> -	.list = f2fs_xattr_list_acl,
> -	.get = f2fs_xattr_get_acl,
> -	.set = f2fs_xattr_set_acl,
> -};
> diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h
> index 4963313..2af31fe 100644
> --- a/fs/f2fs/acl.h
> +++ b/fs/f2fs/acl.h
> @@ -37,6 +37,7 @@ struct f2fs_acl_header {
>  #ifdef CONFIG_F2FS_FS_POSIX_ACL
>  
>  extern struct posix_acl *f2fs_get_acl(struct inode *, int);
> +extern int f2fs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  extern int f2fs_acl_chmod(struct inode *);
>  extern int f2fs_init_acl(struct inode *, struct inode *, struct page *);
>  #else
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 7d714f4..13eff60 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -405,6 +405,7 @@ const struct inode_operations f2fs_file_inode_operations = {
>  	.getattr	= f2fs_getattr,
>  	.setattr	= f2fs_setattr,
>  	.get_acl	= f2fs_get_acl,
> +	.set_acl	= f2fs_set_acl,
>  #ifdef CONFIG_F2FS_FS_XATTR
>  	.setxattr	= generic_setxattr,
>  	.getxattr	= generic_getxattr,
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 575adac..5846eeb 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -496,6 +496,7 @@ const struct inode_operations f2fs_dir_inode_operations = {
>  	.getattr	= f2fs_getattr,
>  	.setattr	= f2fs_setattr,
>  	.get_acl	= f2fs_get_acl,
> +	.set_acl	= f2fs_set_acl,
>  #ifdef CONFIG_F2FS_FS_XATTR
>  	.setxattr	= generic_setxattr,
>  	.getxattr	= generic_getxattr,
> @@ -522,6 +523,7 @@ const struct inode_operations f2fs_special_inode_operations = {
>  	.getattr	= f2fs_getattr,
>  	.setattr        = f2fs_setattr,
>  	.get_acl	= f2fs_get_acl,
> +	.set_acl	= f2fs_set_acl,
>  #ifdef CONFIG_F2FS_FS_XATTR
>  	.setxattr       = generic_setxattr,
>  	.getxattr       = generic_getxattr,
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index aa7a3f1..e2b9299 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -21,6 +21,7 @@
>  #include <linux/rwsem.h>
>  #include <linux/f2fs_fs.h>
>  #include <linux/security.h>
> +#include <linux/posix_acl_xattr.h>
>  #include "f2fs.h"
>  #include "xattr.h"
>  
> @@ -216,8 +217,8 @@ const struct xattr_handler f2fs_xattr_security_handler = {
>  static const struct xattr_handler *f2fs_xattr_handler_map[] = {
>  	[F2FS_XATTR_INDEX_USER] = &f2fs_xattr_user_handler,
>  #ifdef CONFIG_F2FS_FS_POSIX_ACL
> -	[F2FS_XATTR_INDEX_POSIX_ACL_ACCESS] = &f2fs_xattr_acl_access_handler,
> -	[F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT] = &f2fs_xattr_acl_default_handler,
> +	[F2FS_XATTR_INDEX_POSIX_ACL_ACCESS] = &posix_acl_access_xattr_handler,
> +	[F2FS_XATTR_INDEX_POSIX_ACL_DEFAULT] = &posix_acl_default_xattr_handler,
>  #endif
>  	[F2FS_XATTR_INDEX_TRUSTED] = &f2fs_xattr_trusted_handler,
>  #ifdef CONFIG_F2FS_FS_SECURITY
> @@ -229,8 +230,8 @@ static const struct xattr_handler *f2fs_xattr_handler_map[] = {
>  const struct xattr_handler *f2fs_xattr_handlers[] = {
>  	&f2fs_xattr_user_handler,
>  #ifdef CONFIG_F2FS_FS_POSIX_ACL
> -	&f2fs_xattr_acl_access_handler,
> -	&f2fs_xattr_acl_default_handler,
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
>  #endif
>  	&f2fs_xattr_trusted_handler,
>  #ifdef CONFIG_F2FS_FS_SECURITY
> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
> index 02a08fb..b21d9eb 100644
> --- a/fs/f2fs/xattr.h
> +++ b/fs/f2fs/xattr.h
> @@ -108,8 +108,6 @@ struct f2fs_xattr_entry {
>  #ifdef CONFIG_F2FS_FS_XATTR
>  extern const struct xattr_handler f2fs_xattr_user_handler;
>  extern const struct xattr_handler f2fs_xattr_trusted_handler;
> -extern const struct xattr_handler f2fs_xattr_acl_access_handler;
> -extern const struct xattr_handler f2fs_xattr_acl_default_handler;
>  extern const struct xattr_handler f2fs_xattr_advise_handler;
>  extern const struct xattr_handler f2fs_xattr_security_handler;
>  

-- 
Jaegeuk Kim
Samsung

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 10:37:34AM +0900, Jaegeuk Kim wrote:
> f2fs caches a new mode bit for a while to make the consistency between
> xattr's acl mode and the inode mode.

Can you explain what exactly you're trying to do there?  I've been
trying to unwrap what's going on and can't really see the point:

 - i_acl_mode and FI_ACL_MODE get set in __setattr_copy, but right
   after that call, still under i_mutex and before marking the inode
   dirty f2fs_acl_chmod makes use of it, and it gets cleared right
   after. Is there any race that could happen with a locked inode
   not marked dirty yet on f2fs?  We could pass a mode argument
   to posix_acl_create, but I'd prefer to avoid that if we can.
 - on the set_acl side it gets set in __f2fs_set_acl, and then
   i_mode is update in __f2fs_setxattr which could easily done with
   a stack variable.

RFC patch below:


diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
index 4f52fe0f..6647545 100644
--- a/fs/f2fs/acl.c
+++ b/fs/f2fs/acl.c
@@ -17,9 +17,6 @@
 #include "xattr.h"
 #include "acl.h"
 
-#define get_inode_mode(i)	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
-					(F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
-
 static inline size_t f2fs_acl_size(int count)
 {
 	if (count <= 4) {
@@ -209,11 +206,11 @@ static int __f2fs_set_acl(struct inode *inode, int type,
 			struct posix_acl *acl, struct page *ipage)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
-	struct f2fs_inode_info *fi = F2FS_I(inode);
 	int name_index;
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	umode_t mode = 0;
 
 	if (!test_opt(sbi, POSIX_ACL))
 		return 0;
@@ -224,10 +221,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
 	case ACL_TYPE_ACCESS:
 		name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_equiv_mode(acl, &inode->i_mode);
+			mode = inode->i_mode;
+			error = posix_acl_equiv_mode(acl, &mode);
 			if (error < 0)
 				return error;
-			set_acl_inode(fi, inode->i_mode);
 			if (error == 0)
 				acl = NULL;
 		}
@@ -245,19 +242,15 @@ static int __f2fs_set_acl(struct inode *inode, int type,
 
 	if (acl) {
 		value = f2fs_acl_to_disk(acl, &size);
-		if (IS_ERR(value)) {
-			cond_clear_inode_flag(fi, FI_ACL_MODE);
+		if (IS_ERR(value))
 			return (int)PTR_ERR(value);
-		}
 	}
 
-	error = f2fs_setxattr(inode, name_index, "", value, size, ipage);
+	error = f2fs_setxattr(inode, name_index, "", value, size, ipage, mode);
 
 	kfree(value);
 	if (!error)
 		set_cached_acl(inode, type, acl);
-
-	cond_clear_inode_flag(fi, FI_ACL_MODE);
 	return error;
 }
 
@@ -289,28 +282,3 @@ int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage)
 
 	return error;
 }
-
-int f2fs_acl_chmod(struct inode *inode)
-{
-	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
-	struct posix_acl *acl;
-	int error;
-	umode_t mode = get_inode_mode(inode);
-
-	if (!test_opt(sbi, POSIX_ACL))
-		return 0;
-	if (S_ISLNK(mode))
-		return -EOPNOTSUPP;
-
-	acl = f2fs_get_acl(inode, ACL_TYPE_ACCESS);
-	if (IS_ERR(acl) || !acl)
-		return PTR_ERR(acl);
-
-	error = __posix_acl_chmod(&acl, GFP_KERNEL, mode);
-	if (error)
-		return error;
-
-	error = __f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, NULL);
-	posix_acl_release(acl);
-	return error;
-}
diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h
index 2af31fe..e086465 100644
--- a/fs/f2fs/acl.h
+++ b/fs/f2fs/acl.h
@@ -38,18 +38,12 @@ struct f2fs_acl_header {
 
 extern struct posix_acl *f2fs_get_acl(struct inode *, int);
 extern int f2fs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
-extern int f2fs_acl_chmod(struct inode *);
 extern int f2fs_init_acl(struct inode *, struct inode *, struct page *);
 #else
 #define f2fs_check_acl	NULL
 #define f2fs_get_acl	NULL
 #define f2fs_set_acl	NULL
 
-static inline int f2fs_acl_chmod(struct inode *inode)
-{
-	return 0;
-}
-
 static inline int f2fs_init_acl(struct inode *inode, struct inode *dir,
 							struct page *page)
 {
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 89dc750..1e774e6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -181,7 +181,6 @@ struct f2fs_inode_info {
 	unsigned char i_advise;		/* use to give file attribute hints */
 	unsigned int i_current_depth;	/* use only in directory structure */
 	unsigned int i_pino;		/* parent inode number */
-	umode_t i_acl_mode;		/* keep file acl mode temporarily */
 
 	/* Use below internally in f2fs*/
 	unsigned long flags;		/* use to pass per-file flags */
@@ -872,7 +871,6 @@ enum {
 	FI_NEW_INODE,		/* indicate newly allocated inode */
 	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
 	FI_INC_LINK,		/* need to increment i_nlink */
-	FI_ACL_MODE,		/* indicate acl mode */
 	FI_NO_ALLOC,		/* should not allocate any blocks */
 	FI_UPDATE_DIR,		/* should update inode block for consistency */
 	FI_DELAY_IPUT,		/* used for the recovery */
@@ -894,21 +892,6 @@ static inline void clear_inode_flag(struct f2fs_inode_info *fi, int flag)
 	clear_bit(flag, &fi->flags);
 }
 
-static inline void set_acl_inode(struct f2fs_inode_info *fi, umode_t mode)
-{
-	fi->i_acl_mode = mode;
-	set_inode_flag(fi, FI_ACL_MODE);
-}
-
-static inline int cond_clear_inode_flag(struct f2fs_inode_info *fi, int flag)
-{
-	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
-		clear_inode_flag(fi, FI_ACL_MODE);
-		return 1;
-	}
-	return 0;
-}
-
 static inline void get_inline_info(struct f2fs_inode_info *fi,
 					struct f2fs_inode *ri)
 {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 13eff60..80ef669 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -339,41 +339,9 @@ int f2fs_getattr(struct vfsmount *mnt,
 	return 0;
 }
 
-#ifdef CONFIG_F2FS_FS_POSIX_ACL
-static void __setattr_copy(struct inode *inode, const struct iattr *attr)
-{
-	struct f2fs_inode_info *fi = F2FS_I(inode);
-	unsigned int ia_valid = attr->ia_valid;
-
-	if (ia_valid & ATTR_UID)
-		inode->i_uid = attr->ia_uid;
-	if (ia_valid & ATTR_GID)
-		inode->i_gid = attr->ia_gid;
-	if (ia_valid & ATTR_ATIME)
-		inode->i_atime = timespec_trunc(attr->ia_atime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_MTIME)
-		inode->i_mtime = timespec_trunc(attr->ia_mtime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_CTIME)
-		inode->i_ctime = timespec_trunc(attr->ia_ctime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_MODE) {
-		umode_t mode = attr->ia_mode;
-
-		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-			mode &= ~S_ISGID;
-		set_acl_inode(fi, mode);
-	}
-}
-#else
-#define __setattr_copy setattr_copy
-#endif
-
 int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
-	struct f2fs_inode_info *fi = F2FS_I(inode);
 	int err;
 
 	err = inode_change_ok(inode, attr);
@@ -387,15 +355,9 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
 		f2fs_balance_fs(F2FS_SB(inode->i_sb));
 	}
 
-	__setattr_copy(inode, attr);
-
-	if (attr->ia_valid & ATTR_MODE) {
-		err = f2fs_acl_chmod(inode);
-		if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
-			inode->i_mode = fi->i_acl_mode;
-			clear_inode_flag(fi, FI_ACL_MODE);
-		}
-	}
+	setattr_copy(inode, attr);
+	if (attr->ia_valid & ATTR_MODE)
+		err = posix_acl_chmod(inode);
 
 	mark_inode_dirty(inode);
 	return err;
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index e2b9299..8820857 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -108,7 +108,7 @@ static int f2fs_xattr_generic_set(struct dentry *dentry, const char *name,
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
 
-	return f2fs_setxattr(dentry->d_inode, type, name, value, size, NULL);
+	return f2fs_setxattr(dentry->d_inode, type, name, value, size, NULL, 0);
 }
 
 static size_t f2fs_xattr_advise_list(struct dentry *dentry, char *list,
@@ -157,7 +157,7 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
 #ifdef CONFIG_F2FS_FS_SECURITY
 static int __f2fs_setxattr(struct inode *inode, int name_index,
 			const char *name, const void *value, size_t value_len,
-			struct page *ipage);
+			struct page *ipage, umode_t mode);
 static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
 		void *page)
 {
@@ -167,7 +167,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
 	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
 		err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
 				xattr->name, xattr->value,
-				xattr->value_len, (struct page *)page);
+				xattr->value_len, (struct page *)page, 0);
 		if (err < 0)
 			break;
 	}
@@ -475,9 +475,8 @@ cleanup:
 
 static int __f2fs_setxattr(struct inode *inode, int name_index,
 			const char *name, const void *value, size_t value_len,
-			struct page *ipage)
+			struct page *ipage, umode_t mode)
 {
-	struct f2fs_inode_info *fi = F2FS_I(inode);
 	struct f2fs_xattr_entry *here, *last;
 	void *base_addr;
 	int found, newsize;
@@ -566,10 +565,9 @@ static int __f2fs_setxattr(struct inode *inode, int name_index,
 	if (error)
 		goto exit;
 
-	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
-		inode->i_mode = fi->i_acl_mode;
+	if (mode) {
+		inode->i_mode = mode;
 		inode->i_ctime = CURRENT_TIME;
-		clear_inode_flag(fi, FI_ACL_MODE);
 	}
 
 	if (ipage)
@@ -582,7 +580,8 @@ exit:
 }
 
 int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
-			const void *value, size_t value_len, struct page *ipage)
+			const void *value, size_t value_len, struct page *ipage,
+			umode_t mode)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
 	int err;
@@ -590,7 +589,8 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
 	f2fs_balance_fs(sbi);
 
 	f2fs_lock_op(sbi);
-	err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
+	err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage,
+			      mode);
 	f2fs_unlock_op(sbi);
 
 	return err;
diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
index b21d9eb..c73588a 100644
--- a/fs/f2fs/xattr.h
+++ b/fs/f2fs/xattr.h
@@ -114,14 +114,15 @@ extern const struct xattr_handler f2fs_xattr_security_handler;
 extern const struct xattr_handler *f2fs_xattr_handlers[];
 
 extern int f2fs_setxattr(struct inode *, int, const char *,
-				const void *, size_t, struct page *);
+				const void *, size_t, struct page *, umode_t);
 extern int f2fs_getxattr(struct inode *, int, const char *, void *, size_t);
 extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t);
 #else
 
 #define f2fs_xattr_handlers	NULL
 static inline int f2fs_setxattr(struct inode *inode, int name_index,
-		const char *name, const void *value, size_t value_len)
+		const char *name, const void *value, size_t value_len,
+		umode_t mode)
 {
 	return -EOPNOTSUPP;
 }

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Jaegeuk Kim]

2013-12-08 (?), 01:14 -0800, Christoph Hellwig:
> On Fri, Dec 06, 2013 at 10:37:34AM +0900, Jaegeuk Kim wrote:
> > f2fs caches a new mode bit for a while to make the consistency between
> > xattr's acl mode and the inode mode.
> 
> Can you explain what exactly you're trying to do there?  I've been
> trying to unwrap what's going on and can't really see the point:
> 
>  - i_acl_mode and FI_ACL_MODE get set in __setattr_copy, but right
>    after that call, still under i_mutex and before marking the inode
>    dirty f2fs_acl_chmod makes use of it, and it gets cleared right
>    after. Is there any race that could happen with a locked inode
>    not marked dirty yet on f2fs?

As you guess, there is no race problem, but the problem is on acl
consistency between xattr->mode and inode->mode.

Previously, f2fs_setattr triggers:
              new_mode inode->mode xattr->mode iblock->mode
f2fs_setattr     x    ->    x           y         y       
[update_inode]              x    ---  [ y ]  ---> x
[checkpoint]                x           y         x
__f2fs_setxattr             x     ->    x         x

In this flow, f2fs is able to break the consistency between xattr->mode
and iblock->mode after checkpoint followed by sudden-power-off.

So, fi->mode was introduced to address the problem.
The new f2fs_setattr triggers:
              new_mode inode->mode fi->mode xattr->mode iblock->mode
f2fs_setattr     x    ---  [y]  --->   x          y          y
[update_inode]              y          x          y          y
[checkpoint]                y          x          y          y
__f2fs_setxattr             x    <-    x    ->    x     ->   x

Finally, __f2fs_setxattr synchronizes inode->mode, xattr->mode, and
iblock->mode all together.

The root question is "is it possible to call update_inode in the
i_mutex-covered region like f2fs_setattr?".
The update_inode of f2fs is called from a bunch of places so currently
I'm not sure it can be impossible.

Thanks,

> We could pass a mode argument
>    to posix_acl_create, but I'd prefer to avoid that if we can.
>  - on the set_acl side it gets set in __f2fs_set_acl, and then
>    i_mode is update in __f2fs_setxattr which could easily done with
>    a stack variable.
> 
> RFC patch below:
> 
> 
> diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c
> index 4f52fe0f..6647545 100644
> --- a/fs/f2fs/acl.c
> +++ b/fs/f2fs/acl.c
> @@ -17,9 +17,6 @@
>  #include "xattr.h"
>  #include "acl.h"
>  
> -#define get_inode_mode(i)	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
> -					(F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
> -
>  static inline size_t f2fs_acl_size(int count)
>  {
>  	if (count <= 4) {
> @@ -209,11 +206,11 @@ static int __f2fs_set_acl(struct inode *inode, int type,
>  			struct posix_acl *acl, struct page *ipage)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> -	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	int name_index;
>  	void *value = NULL;
>  	size_t size = 0;
>  	int error;
> +	umode_t mode = 0;
>  
>  	if (!test_opt(sbi, POSIX_ACL))
>  		return 0;
> @@ -224,10 +221,10 @@ static int __f2fs_set_acl(struct inode *inode, int type,
>  	case ACL_TYPE_ACCESS:
>  		name_index = F2FS_XATTR_INDEX_POSIX_ACL_ACCESS;
>  		if (acl) {
> -			error = posix_acl_equiv_mode(acl, &inode->i_mode);
> +			mode = inode->i_mode;
> +			error = posix_acl_equiv_mode(acl, &mode);
>  			if (error < 0)
>  				return error;
> -			set_acl_inode(fi, inode->i_mode);
>  			if (error == 0)
>  				acl = NULL;
>  		}
> @@ -245,19 +242,15 @@ static int __f2fs_set_acl(struct inode *inode, int type,
>  
>  	if (acl) {
>  		value = f2fs_acl_to_disk(acl, &size);
> -		if (IS_ERR(value)) {
> -			cond_clear_inode_flag(fi, FI_ACL_MODE);
> +		if (IS_ERR(value))
>  			return (int)PTR_ERR(value);
> -		}
>  	}
>  
> -	error = f2fs_setxattr(inode, name_index, "", value, size, ipage);
> +	error = f2fs_setxattr(inode, name_index, "", value, size, ipage, mode);
>  
>  	kfree(value);
>  	if (!error)
>  		set_cached_acl(inode, type, acl);
> -
> -	cond_clear_inode_flag(fi, FI_ACL_MODE);
>  	return error;
>  }
>  
> @@ -289,28 +282,3 @@ int f2fs_init_acl(struct inode *inode, struct inode *dir, struct page *ipage)
>  
>  	return error;
>  }
> -
> -int f2fs_acl_chmod(struct inode *inode)
> -{
> -	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
> -	struct posix_acl *acl;
> -	int error;
> -	umode_t mode = get_inode_mode(inode);
> -
> -	if (!test_opt(sbi, POSIX_ACL))
> -		return 0;
> -	if (S_ISLNK(mode))
> -		return -EOPNOTSUPP;
> -
> -	acl = f2fs_get_acl(inode, ACL_TYPE_ACCESS);
> -	if (IS_ERR(acl) || !acl)
> -		return PTR_ERR(acl);
> -
> -	error = __posix_acl_chmod(&acl, GFP_KERNEL, mode);
> -	if (error)
> -		return error;
> -
> -	error = __f2fs_set_acl(inode, ACL_TYPE_ACCESS, acl, NULL);
> -	posix_acl_release(acl);
> -	return error;
> -}
> diff --git a/fs/f2fs/acl.h b/fs/f2fs/acl.h
> index 2af31fe..e086465 100644
> --- a/fs/f2fs/acl.h
> +++ b/fs/f2fs/acl.h
> @@ -38,18 +38,12 @@ struct f2fs_acl_header {
>  
>  extern struct posix_acl *f2fs_get_acl(struct inode *, int);
>  extern int f2fs_set_acl(struct inode *inode, struct posix_acl *acl, int type);
> -extern int f2fs_acl_chmod(struct inode *);
>  extern int f2fs_init_acl(struct inode *, struct inode *, struct page *);
>  #else
>  #define f2fs_check_acl	NULL
>  #define f2fs_get_acl	NULL
>  #define f2fs_set_acl	NULL
>  
> -static inline int f2fs_acl_chmod(struct inode *inode)
> -{
> -	return 0;
> -}
> -
>  static inline int f2fs_init_acl(struct inode *inode, struct inode *dir,
>  							struct page *page)
>  {
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 89dc750..1e774e6 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -181,7 +181,6 @@ struct f2fs_inode_info {
>  	unsigned char i_advise;		/* use to give file attribute hints */
>  	unsigned int i_current_depth;	/* use only in directory structure */
>  	unsigned int i_pino;		/* parent inode number */
> -	umode_t i_acl_mode;		/* keep file acl mode temporarily */
>  
>  	/* Use below internally in f2fs*/
>  	unsigned long flags;		/* use to pass per-file flags */
> @@ -872,7 +871,6 @@ enum {
>  	FI_NEW_INODE,		/* indicate newly allocated inode */
>  	FI_DIRTY_INODE,		/* indicate inode is dirty or not */
>  	FI_INC_LINK,		/* need to increment i_nlink */
> -	FI_ACL_MODE,		/* indicate acl mode */
>  	FI_NO_ALLOC,		/* should not allocate any blocks */
>  	FI_UPDATE_DIR,		/* should update inode block for consistency */
>  	FI_DELAY_IPUT,		/* used for the recovery */
> @@ -894,21 +892,6 @@ static inline void clear_inode_flag(struct f2fs_inode_info *fi, int flag)
>  	clear_bit(flag, &fi->flags);
>  }
>  
> -static inline void set_acl_inode(struct f2fs_inode_info *fi, umode_t mode)
> -{
> -	fi->i_acl_mode = mode;
> -	set_inode_flag(fi, FI_ACL_MODE);
> -}
> -
> -static inline int cond_clear_inode_flag(struct f2fs_inode_info *fi, int flag)
> -{
> -	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> -		clear_inode_flag(fi, FI_ACL_MODE);
> -		return 1;
> -	}
> -	return 0;
> -}
> -
>  static inline void get_inline_info(struct f2fs_inode_info *fi,
>  					struct f2fs_inode *ri)
>  {
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 13eff60..80ef669 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -339,41 +339,9 @@ int f2fs_getattr(struct vfsmount *mnt,
>  	return 0;
>  }
>  
> -#ifdef CONFIG_F2FS_FS_POSIX_ACL
> -static void __setattr_copy(struct inode *inode, const struct iattr *attr)
> -{
> -	struct f2fs_inode_info *fi = F2FS_I(inode);
> -	unsigned int ia_valid = attr->ia_valid;
> -
> -	if (ia_valid & ATTR_UID)
> -		inode->i_uid = attr->ia_uid;
> -	if (ia_valid & ATTR_GID)
> -		inode->i_gid = attr->ia_gid;
> -	if (ia_valid & ATTR_ATIME)
> -		inode->i_atime = timespec_trunc(attr->ia_atime,
> -						inode->i_sb->s_time_gran);
> -	if (ia_valid & ATTR_MTIME)
> -		inode->i_mtime = timespec_trunc(attr->ia_mtime,
> -						inode->i_sb->s_time_gran);
> -	if (ia_valid & ATTR_CTIME)
> -		inode->i_ctime = timespec_trunc(attr->ia_ctime,
> -						inode->i_sb->s_time_gran);
> -	if (ia_valid & ATTR_MODE) {
> -		umode_t mode = attr->ia_mode;
> -
> -		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> -			mode &= ~S_ISGID;
> -		set_acl_inode(fi, mode);
> -	}
> -}
> -#else
> -#define __setattr_copy setattr_copy
> -#endif
> -
>  int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
>  {
>  	struct inode *inode = dentry->d_inode;
> -	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	int err;
>  
>  	err = inode_change_ok(inode, attr);
> @@ -387,15 +355,9 @@ int f2fs_setattr(struct dentry *dentry, struct iattr *attr)
>  		f2fs_balance_fs(F2FS_SB(inode->i_sb));
>  	}
>  
> -	__setattr_copy(inode, attr);
> -
> -	if (attr->ia_valid & ATTR_MODE) {
> -		err = f2fs_acl_chmod(inode);
> -		if (err || is_inode_flag_set(fi, FI_ACL_MODE)) {
> -			inode->i_mode = fi->i_acl_mode;
> -			clear_inode_flag(fi, FI_ACL_MODE);
> -		}
> -	}
> +	setattr_copy(inode, attr);
> +	if (attr->ia_valid & ATTR_MODE)
> +		err = posix_acl_chmod(inode);
>  
>  	mark_inode_dirty(inode);
>  	return err;
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index e2b9299..8820857 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -108,7 +108,7 @@ static int f2fs_xattr_generic_set(struct dentry *dentry, const char *name,
>  	if (strcmp(name, "") == 0)
>  		return -EINVAL;
>  
> -	return f2fs_setxattr(dentry->d_inode, type, name, value, size, NULL);
> +	return f2fs_setxattr(dentry->d_inode, type, name, value, size, NULL, 0);
>  }
>  
>  static size_t f2fs_xattr_advise_list(struct dentry *dentry, char *list,
> @@ -157,7 +157,7 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
>  #ifdef CONFIG_F2FS_FS_SECURITY
>  static int __f2fs_setxattr(struct inode *inode, int name_index,
>  			const char *name, const void *value, size_t value_len,
> -			struct page *ipage);
> +			struct page *ipage, umode_t mode);
>  static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
>  		void *page)
>  {
> @@ -167,7 +167,7 @@ static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
>  	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>  		err = __f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY,
>  				xattr->name, xattr->value,
> -				xattr->value_len, (struct page *)page);
> +				xattr->value_len, (struct page *)page, 0);
>  		if (err < 0)
>  			break;
>  	}
> @@ -475,9 +475,8 @@ cleanup:
>  
>  static int __f2fs_setxattr(struct inode *inode, int name_index,
>  			const char *name, const void *value, size_t value_len,
> -			struct page *ipage)
> +			struct page *ipage, umode_t mode)
>  {
> -	struct f2fs_inode_info *fi = F2FS_I(inode);
>  	struct f2fs_xattr_entry *here, *last;
>  	void *base_addr;
>  	int found, newsize;
> @@ -566,10 +565,9 @@ static int __f2fs_setxattr(struct inode *inode, int name_index,
>  	if (error)
>  		goto exit;
>  
> -	if (is_inode_flag_set(fi, FI_ACL_MODE)) {
> -		inode->i_mode = fi->i_acl_mode;
> +	if (mode) {
> +		inode->i_mode = mode;
>  		inode->i_ctime = CURRENT_TIME;
> -		clear_inode_flag(fi, FI_ACL_MODE);
>  	}
>  
>  	if (ipage)
> @@ -582,7 +580,8 @@ exit:
>  }
>  
>  int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
> -			const void *value, size_t value_len, struct page *ipage)
> +			const void *value, size_t value_len, struct page *ipage,
> +			umode_t mode)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
>  	int err;
> @@ -590,7 +589,8 @@ int f2fs_setxattr(struct inode *inode, int name_index, const char *name,
>  	f2fs_balance_fs(sbi);
>  
>  	f2fs_lock_op(sbi);
> -	err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage);
> +	err = __f2fs_setxattr(inode, name_index, name, value, value_len, ipage,
> +			      mode);
>  	f2fs_unlock_op(sbi);
>  
>  	return err;
> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
> index b21d9eb..c73588a 100644
> --- a/fs/f2fs/xattr.h
> +++ b/fs/f2fs/xattr.h
> @@ -114,14 +114,15 @@ extern const struct xattr_handler f2fs_xattr_security_handler;
>  extern const struct xattr_handler *f2fs_xattr_handlers[];
>  
>  extern int f2fs_setxattr(struct inode *, int, const char *,
> -				const void *, size_t, struct page *);
> +				const void *, size_t, struct page *, umode_t);
>  extern int f2fs_getxattr(struct inode *, int, const char *, void *, size_t);
>  extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t);
>  #else
>  
>  #define f2fs_xattr_handlers	NULL
>  static inline int f2fs_setxattr(struct inode *inode, int name_index,
> -		const char *name, const void *value, size_t value_len)
> +		const char *name, const void *value, size_t value_len,
> +		umode_t mode)
>  {
>  	return -EOPNOTSUPP;
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jaegeuk Kim
Samsung

[Cluster-devel] [PATCH 00/18] Consolidate Posix ACL implementation V2

[Christoph Hellwig]

This series consolidates the various cut'n'pasted Posix ACL implementations
into a single common one based on the ->get_acl method Linus added a while
ago and a new ->set_acl counterpart.

This remove ~1800 lines of code and provides a single place to implement
various nasty little gems of the semantics.

Unfortunately the 9p code is still left out - it implements the ACLs
in two very weird ways, one using the common code but on the client only,
and one pasing things straight through to the server.  We could easily
convert it to the new code on the write side if ->set_acl took a dentry,
but there's no cance to do that on the ->get_acl side.  Ideas how to
handle it welcome.

After that we'd be ready to never go into the fs for the ACL attributes
and branch straight to the ACL code below the syscall, repairing the
old API braindamage of overloading ACLs onto the xattrs.


Changes from V1:
 - check for symlinks in the ACL code and remove checks in the lower
   level functions.
 - remove get_acl instances for symlinks in a few filesystems
 - pass a umode_t mode argument to posix_acl_chmod to accomodate f2fs
 - various cosemtic bits from the reviews.

Note that I still haven't heard from ocfs2 folks, so the patch is left
unchanged.

[Cluster-devel] [PATCH 01/18] reiserfs: prefix ACL symbols with reiserfs_

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0001-reiserfs-prefix-ACL-symbols-with-reiserfs_.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/35841bc7/attachment.ksh>

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0002-fs-add-get_acl-helper.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2311bc1c/attachment.ksh>

[Cluster-devel] [PATCH 03/18] fs: add a set_acl inode operation

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0003-fs-add-a-set_acl-inode-operation.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/12450327/attachment.ksh>

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0004-fs-add-generic-xattr_acl-handlers.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/91db43bc/attachment.ksh>

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0005-fs-make-posix_acl_chmod-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/176a07d3/attachment.ksh>

[Cluster-devel] [PATCH 06/18] fs: make posix_acl_create more useful

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0006-fs-make-posix_acl_create-more-useful.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/eb09193e/attachment.ksh>

[Cluster-devel] [PATCH 07/18] btrfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0007-btrfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/876d757d/attachment.ksh>

[Cluster-devel] [PATCH 08/18] ext2/3/4: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0008-ext2-3-4-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6d2a563e/attachment.ksh>

[Cluster-devel] [PATCH 09/18] f2fs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0009-f2fs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/2cec2935/attachment.ksh>

[Cluster-devel] [PATCH 10/18] hfsplus: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0010-hfsplus-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/d51bfb09/attachment.ksh>

[Cluster-devel] [PATCH 11/18] jffs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0011-jffs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/a3efba51/attachment.ksh>

[Cluster-devel] [PATCH 12/18] ocfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0012-ocfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/88e1778c/attachment.ksh>

[Cluster-devel] [PATCH 13/18] reiserfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0013-reiserfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/6aa889e6/attachment.ksh>

[Cluster-devel] [PATCH 14/18] xfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0014-xfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/9a6a7104/attachment.ksh>

[Cluster-devel] [PATCH 15/18] jfs: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0015-jfs-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/c6ddfc3b/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0016-gfs2-use-generic-posix-ACL-infrastructure.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/aafe34fb/attachment.ksh>

[Cluster-devel] [PATCH 17/18] nfs: use generic posix ACL infrastructure for v3 Posix ACLs

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0017-nfs-use-generic-posix-ACL-infrastructure-for-v3-Posi.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ce18263a/attachment.ksh>

[Cluster-devel] [PATCH 18/18] fs: remove generic_acl

[Christoph Hellwig]

An embedded and charset-unspecified text was scrubbed...
Name: 0018-fs-remove-generic_acl.patch
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20131211/ff393c13/attachment.ksh>

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Steven Whitehouse]

Hi,

On Wed, 2013-12-11 at 02:42 -0800, Christoph Hellwig wrote:
> plain text document attachment
> (0016-gfs2-use-generic-posix-ACL-infrastructure.patch)
> This contains some major refactoring for the create path so that
> inodes are created with the right mode to start with instead of
> fixing it up later.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Steven Whitehouse <swhiteho@redhat.com>

A really nice clean up - this is a very useful step forward in
simplifying the create path. Thanks for sorting this out,

Steve.

> ---
>  fs/gfs2/acl.c   |  234 +++++++------------------------------------------------
>  fs/gfs2/acl.h   |    4 +-
>  fs/gfs2/inode.c |   33 ++++++--
>  fs/gfs2/xattr.c |    4 +-
>  4 files changed, 62 insertions(+), 213 deletions(-)
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index e82e4ac..ba94566 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -49,10 +49,6 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	if (!ip->i_eattr)
>  		return NULL;
>  
> -	acl = get_cached_acl(&ip->i_inode, type);
> -	if (acl != ACL_NOT_CACHED)
> -		return acl;
> -
>  	name = gfs2_acl_name(type);
>  	if (name == NULL)
>  		return ERR_PTR(-EINVAL);
> @@ -80,7 +76,7 @@ static int gfs2_set_mode(struct inode *inode, umode_t mode)
>  	return error;
>  }
>  
> -static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
> +int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  {
>  	int error;
>  	int len;
> @@ -88,219 +84,49 @@ static int gfs2_acl_set(struct inode *inode, int type, struct posix_acl *acl)
>  	const char *name = gfs2_acl_name(type);
>  
>  	BUG_ON(name == NULL);
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	if (len == 0)
> -		return 0;
> -	data = kmalloc(len, GFP_NOFS);
> -	if (data == NULL)
> -		return -ENOMEM;
> -	error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	if (error < 0)
> -		goto out;
> -	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> -	if (!error)
> -		set_cached_acl(inode, type, acl);
> -out:
> -	kfree(data);
> -	return error;
> -}
> -
> -int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode)
> -{
> -	struct gfs2_sbd *sdp = GFS2_SB(&dip->i_inode);
> -	struct posix_acl *acl;
> -	umode_t mode = inode->i_mode;
> -	int error = 0;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return 0;
> -	if (S_ISLNK(inode->i_mode))
> -		return 0;
> -
> -	acl = gfs2_get_acl(&dip->i_inode, ACL_TYPE_DEFAULT);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl) {
> -		mode &= ~current_umask();
> -		return gfs2_set_mode(inode, mode);
> -	}
> -
> -	if (S_ISDIR(inode->i_mode)) {
> -		error = gfs2_acl_set(inode, ACL_TYPE_DEFAULT, acl);
> -		if (error)
> -			goto out;
> -	}
> -
> -	error = __posix_acl_create(&acl, GFP_NOFS, &mode);
> -	if (error < 0)
> -		return error;
>  
> -	if (error == 0)
> -		goto munge;
> -
> -	error = gfs2_acl_set(inode, ACL_TYPE_ACCESS, acl);
> -	if (error)
> -		goto out;
> -munge:
> -	error = gfs2_set_mode(inode, mode);
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr)
> -{
> -	struct inode *inode = &ip->i_inode;
> -	struct posix_acl *acl;
> -	char *data;
> -	unsigned int len;
> -	int error;
> -
> -	acl = gfs2_get_acl(&ip->i_inode, ACL_TYPE_ACCESS);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (!acl)
> -		return gfs2_setattr_simple(inode, attr);
> -
> -	error = __posix_acl_chmod(&acl, GFP_NOFS, attr->ia_mode);
> -	if (error)
> -		return error;
> -
> -	len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> -	data = kmalloc(len, GFP_NOFS);
> -	error = -ENOMEM;
> -	if (data == NULL)
> -		goto out;
> -	posix_acl_to_xattr(&init_user_ns, acl, data, len);
> -	error = gfs2_xattr_acl_chmod(ip, attr, data);
> -	kfree(data);
> -	set_cached_acl(&ip->i_inode, ACL_TYPE_ACCESS, acl);
> -
> -out:
> -	posix_acl_release(acl);
> -	return error;
> -}
> -
> -static int gfs2_acl_type(const char *name)
> -{
> -	if (strcmp(name, GFS2_POSIX_ACL_ACCESS) == 0)
> -		return ACL_TYPE_ACCESS;
> -	if (strcmp(name, GFS2_POSIX_ACL_DEFAULT) == 0)
> -		return ACL_TYPE_DEFAULT;
> -	return -EINVAL;
> -}
> -
> -static int gfs2_xattr_system_get(struct dentry *dentry, const char *name,
> -				 void *buffer, size_t size, int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl;
> -	int type;
> -	int error;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -
> -	acl = gfs2_get_acl(inode, type);
> -	if (IS_ERR(acl))
> -		return PTR_ERR(acl);
> -	if (acl == NULL)
> -		return -ENODATA;
> -
> -	error = posix_acl_to_xattr(&init_user_ns, acl, buffer, size);
> -	posix_acl_release(acl);
> -
> -	return error;
> -}
> -
> -static int gfs2_xattr_system_set(struct dentry *dentry, const char *name,
> -				 const void *value, size_t size, int flags,
> -				 int xtype)
> -{
> -	struct inode *inode = dentry->d_inode;
> -	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct posix_acl *acl = NULL;
> -	int error = 0, type;
> -
> -	if (!sdp->sd_args.ar_posix_acl)
> -		return -EOPNOTSUPP;
> -
> -	type = gfs2_acl_type(name);
> -	if (type < 0)
> -		return type;
> -	if (flags & XATTR_CREATE)
> -		return -EINVAL;
> -	if (type == ACL_TYPE_DEFAULT && !S_ISDIR(inode->i_mode))
> -		return value ? -EACCES : 0;
> -	if (!uid_eq(current_fsuid(), inode->i_uid) && !capable(CAP_FOWNER))
> -		return -EPERM;
> -	if (S_ISLNK(inode->i_mode))
> -		return -EOPNOTSUPP;
> -
> -	if (!value)
> -		goto set_acl;
> -
> -	acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	if (!acl) {
> -		/*
> -		 * acl_set_file(3) may request that we set default ACLs with
> -		 * zero length -- defend (gracefully) against that here.
> -		 */
> -		goto out;
> -	}
> -	if (IS_ERR(acl)) {
> -		error = PTR_ERR(acl);
> -		goto out;
> -	}
> -
> -	error = posix_acl_valid(acl);
> -	if (error)
> -		goto out_release;
> -
> -	error = -EINVAL;
>  	if (acl->a_count > GFS2_ACL_MAX_ENTRIES)
> -		goto out_release;
> +		return -EINVAL;
>  
>  	if (type == ACL_TYPE_ACCESS) {
>  		umode_t mode = inode->i_mode;
> +
>  		error = posix_acl_equiv_mode(acl, &mode);
> +		if (error < 0)
> +			return error;
>  
> -		if (error <= 0) {
> -			posix_acl_release(acl);
> +		if (error == 0)
>  			acl = NULL;
>  
> -			if (error < 0)
> -				return error;
> -		}
> -
>  		error = gfs2_set_mode(inode, mode);
>  		if (error)
> -			goto out_release;
> +			return error;
>  	}
>  
> -set_acl:
> -	error = __gfs2_xattr_set(inode, name, value, size, 0, GFS2_EATYPE_SYS);
> -	if (!error) {
> -		if (acl)
> -			set_cached_acl(inode, type, acl);
> -		else
> -			forget_cached_acl(inode, type);
> +	if (acl) {
> +		len = posix_acl_to_xattr(&init_user_ns, acl, NULL, 0);
> +		if (len == 0)
> +			return 0;
> +		data = kmalloc(len, GFP_NOFS);
> +		if (data == NULL)
> +			return -ENOMEM;
> +		error = posix_acl_to_xattr(&init_user_ns, acl, data, len);
> +		if (error < 0)
> +			goto out;
> +	} else {
> +		data = NULL;
> +		len = 0;
>  	}
> -out_release:
> -	posix_acl_release(acl);
> +
> +	error = __gfs2_xattr_set(inode, name, data, len, 0, GFS2_EATYPE_SYS);
> +	if (error)
> +		goto out;
> +
> +	if (acl)
> +		set_cached_acl(inode, type, acl);
> +	else
> +		forget_cached_acl(inode, type);
>  out:
> +	kfree(data);
>  	return error;
>  }
> -
> -const struct xattr_handler gfs2_xattr_system_handler = {
> -	.prefix = XATTR_SYSTEM_PREFIX,
> -	.flags  = GFS2_EATYPE_SYS,
> -	.get    = gfs2_xattr_system_get,
> -	.set    = gfs2_xattr_system_set,
> -};
> -
> diff --git a/fs/gfs2/acl.h b/fs/gfs2/acl.h
> index 0da38dc..301260c 100644
> --- a/fs/gfs2/acl.h
> +++ b/fs/gfs2/acl.h
> @@ -17,8 +17,6 @@
>  #define GFS2_ACL_MAX_ENTRIES		25
>  
>  extern struct posix_acl *gfs2_get_acl(struct inode *inode, int type);
> -extern int gfs2_acl_create(struct gfs2_inode *dip, struct inode *inode);
> -extern int gfs2_acl_chmod(struct gfs2_inode *ip, struct iattr *attr);
> -extern const struct xattr_handler gfs2_xattr_system_handler;
> +extern int gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type);
>  
>  #endif /* __ACL_DOT_H__ */
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 7119504..92b8959 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -552,6 +552,7 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  			     unsigned int size, int excl, int *opened)
>  {
>  	const struct qstr *name = &dentry->d_name;
> +	struct posix_acl *default_acl, *acl;
>  	struct gfs2_holder ghs[2];
>  	struct inode *inode = NULL;
>  	struct gfs2_inode *dip = GFS2_I(dir), *ip;
> @@ -611,10 +612,14 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	if (!inode)
>  		goto fail_gunlock;
>  
> +	error = posix_acl_create(dir, &mode, &default_acl, &acl);
> +	if (error)
> +		goto fail_free_vfs_inode;
> +
>  	ip = GFS2_I(inode);
>  	error = gfs2_rs_alloc(ip);
>  	if (error)
> -		goto fail_free_inode;
> +		goto fail_free_acls;
>  
>  	inode->i_mode = mode;
>  	set_nlink(inode, S_ISDIR(mode) ? 2 : 1);
> @@ -682,7 +687,16 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry,
>  	gfs2_set_iop(inode);
>  	insert_inode_hash(inode);
>  
> -	error = gfs2_acl_create(dip, inode);
> +	if (default_acl) {
> +		error = gfs2_set_acl(inode, default_acl, ACL_TYPE_DEFAULT);
> +		posix_acl_release(default_acl);
> +	}
> +	if (acl) {
> +		if (!error)
> +			error = gfs2_set_acl(inode, acl, ACL_TYPE_ACCESS);
> +		posix_acl_release(acl);
> +	}
> +
>  	if (error)
>  		goto fail_gunlock3;
>  
> @@ -716,6 +730,12 @@ fail_free_inode:
>  	if (ip->i_gl)
>  		gfs2_glock_put(ip->i_gl);
>  	gfs2_rs_delete(ip, NULL);
> +fail_free_acls:
> +	if (default_acl)
> +		posix_acl_release(default_acl);
> +	if (acl)
> +		posix_acl_release(acl);
> +fail_free_vfs_inode:
>  	free_inode_nonrcu(inode);
>  	inode = NULL;
>  fail_gunlock:
> @@ -1678,10 +1698,11 @@ static int gfs2_setattr(struct dentry *dentry, struct iattr *attr)
>  		error = gfs2_setattr_size(inode, attr->ia_size);
>  	else if (attr->ia_valid & (ATTR_UID | ATTR_GID))
>  		error = setattr_chown(inode, attr);
> -	else if ((attr->ia_valid & ATTR_MODE) && IS_POSIXACL(inode))
> -		error = gfs2_acl_chmod(ip, attr);
> -	else
> +	else {
>  		error = gfs2_setattr_simple(inode, attr);
> +		if (!error && attr->ia_valid & ATTR_MODE)
> +			error = posix_acl_chmod(inode, inode->i_mode);
> +	}
>  
>  out:
>  	if (!error)
> @@ -1841,6 +1862,7 @@ const struct inode_operations gfs2_file_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  };
>  
>  const struct inode_operations gfs2_dir_iops = {
> @@ -1862,6 +1884,7 @@ const struct inode_operations gfs2_dir_iops = {
>  	.removexattr = gfs2_removexattr,
>  	.fiemap = gfs2_fiemap,
>  	.get_acl = gfs2_get_acl,
> +	.set_acl = gfs2_set_acl,
>  	.atomic_open = gfs2_atomic_open,
>  };
>  
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index 8c6a6f6..0b81f78 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -13,6 +13,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/xattr.h>
>  #include <linux/gfs2_ondisk.h>
> +#include <linux/posix_acl_xattr.h>
>  #include <asm/uaccess.h>
>  
>  #include "gfs2.h"
> @@ -1500,7 +1501,8 @@ static const struct xattr_handler gfs2_xattr_security_handler = {
>  const struct xattr_handler *gfs2_xattr_handlers[] = {
>  	&gfs2_xattr_user_handler,
>  	&gfs2_xattr_security_handler,
> -	&gfs2_xattr_system_handler,
> +	&posix_acl_access_xattr_handler,
> +	&posix_acl_default_xattr_handler,
>  	NULL,
>  };
>  

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Andreas Gruenbacher]

Christoph,

> +struct posix_acl *get_acl(struct inode *inode, int type)
> +{
> +	struct posix_acl *acl;
> +
> +	acl = get_cached_acl(inode, type);
> +	if (acl != ACL_NOT_CACHED)
> +		return acl;
> +
> +	if (!IS_POSIXACL(inode))
> +		return NULL;
> +
> +	/*
> +	 * A filesystem can force a ACL callback by just never filling the
> +	 * ACL cache. But normally you'd fill the cache either at inode
> +	 * instantiation time, or on the first ->get_acl call.
> +	 *
> +	 * 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);
> +		return ERR_PTR(-EAGAIN);

The function should return NULL here.

Andreas

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Andreas Gruenbacher]

Christoph,

> +int
> +posix_acl_chmod(struct inode *inode)
> +{
> +	struct posix_acl *acl;
> +	int ret = 0;
> +
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Andreas

[Cluster-devel] [PATCH 04/18] fs: add generic xattr_acl handlers

[Andreas Gruenbacher]

Christoph,

> +static int
> +posix_acl_xattr_set(struct dentry *dentry, const char *name,
> +		const void *value, size_t size, int flags, int type)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	struct posix_acl *acl = NULL;
> +	int ret;
> +
> +	if (!IS_POSIXACL(inode))
> +		return -EOPNOTSUPP;
> +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> +		return -EOPNOTSUPP;

Sama here, I would remove the S_ISLNK() check.

Andreas

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Andreas Gruenbacher]

Christoph,

gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
fs/gfs2/inode.c, from a long time ago, which should be removed
as well.

Andreas

[Cluster-devel] [PATCH 02/18] fs: add get_acl helper

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:06:09PM +0100, Andreas Gruenbacher wrote:
> > +	/*
> > +	 * A filesystem can force a ACL callback by just never filling the
> > +	 * ACL cache. But normally you'd fill the cache either at inode
> > +	 * instantiation time, or on the first ->get_acl call.
> > +	 *
> > +	 * 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);
> > +		return ERR_PTR(-EAGAIN);
> 
> The function should return NULL here.

Indeed.  EAGAIN is the convention check_acl() in fs/namei.c uses,
but it will return that automatically if we just return NULL here.

[Cluster-devel] [PATCH 05/18] fs: make posix_acl_chmod more useful

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:07:20PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> > +int
> > +posix_acl_chmod(struct inode *inode)
> > +{
> > +	struct posix_acl *acl;
> > +	int ret = 0;
> > +
> > +	if (S_ISLNK(inode->i_mode) || !inode->i_op->set_acl)
> > +		return -EOPNOTSUPP;
> 
> Symlinks never have get_acl callbacks, so I would remove the S_ISLNK() check here.

Yeah, will simplify it.  Same for the other places where we have both
checks.

[Cluster-devel] [PATCH 16/18] gfs2: use generic posix ACL infrastructure

[Christoph Hellwig]

On Thu, Dec 12, 2013 at 08:08:38PM +0100, Andreas Gruenbacher wrote:
> Christoph,
> 
> gfs2 has a left-over get_acl callback in gfs2_symlink_iops in
> fs/gfs2/inode.c, from a long time ago, which should be removed
> as well.

Ok, will fix.