[PATCH v2 1/2] staging: erofs: use xattr_prefix to wrap up

[PATCH v2 1/2] staging: erofs: use xattr_prefix to wrap up

[Gao Xiang]

Let's use xattr_prefix instead of open code.
No logic changes.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
change log v2:
 - remove the confusing line according to Dan Carpenter;

 drivers/staging/erofs/xattr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 1c9498e38f0e..7de46690d972 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -520,8 +520,7 @@ static int xattr_entrylist(struct xattr_iter *_it,
 	if (h == NULL || (h->list != NULL && !h->list(it->dentry)))
 		return 1;
 
-	/* Note that at least one of 'prefix' and 'name' should be non-NULL */
-	prefix = h->prefix != NULL ? h->prefix : h->name;
+	prefix = xattr_prefix(h);
 	prefix_len = strlen(prefix);
 
 	if (it->buffer == NULL) {
-- 
2.14.4

[PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Gao Xiang]

Let's add .get_acl() to read the file's acl from its xattrs
to make POSIX ACL usable.

Here is the on-disk detail,
fullname: system.posix_acl_access
struct erofs_xattr_entry:
        .e_name_len = 0
        .e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS (2)

fullname: system.posix_acl_default
struct erofs_xattr_entry:
	.e_name_len = 0
	.e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT (3)

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
change log v2:
 - add the missing SB_POSIXACL flag;
 - fix the on-disk detail, use .e_name_len == 0 and proper prefix value;
 - tested ok with xattr enabled erofs_mkfs;

 .../erofs/Documentation/filesystems/erofs.txt      |  2 ++
 drivers/staging/erofs/inode.c                      |  3 ++
 drivers/staging/erofs/namei.c                      |  1 +
 drivers/staging/erofs/super.c                      |  8 +++++
 drivers/staging/erofs/xattr.c                      | 37 ++++++++++++++++++++++
 drivers/staging/erofs/xattr.h                      |  6 ++++
 6 files changed, 57 insertions(+)

diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
index 803988d74c21..961ec4da7705 100644
--- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
+++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
@@ -36,6 +36,8 @@ Here is the main features of EROFS:
 
  - Support xattr inline and tail-end data inline for all files;
 
+ - Support POSIX.1e ACLs by using xattrs;
+
  - Support transparent file compression as an option:
    LZ4 algorithm with 4 KB fixed-output compression for high performance;
 
diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
index 4f04f7c38cf2..924b8dfc7a8f 100644
--- a/drivers/staging/erofs/inode.c
+++ b/drivers/staging/erofs/inode.c
@@ -287,6 +287,7 @@ const struct inode_operations erofs_generic_iops = {
 #ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
 #endif
+	.get_acl = erofs_get_acl,
 };
 
 const struct inode_operations erofs_symlink_iops = {
@@ -294,6 +295,7 @@ const struct inode_operations erofs_symlink_iops = {
 #ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
 #endif
+	.get_acl = erofs_get_acl,
 };
 
 const struct inode_operations erofs_fast_symlink_iops = {
@@ -301,5 +303,6 @@ const struct inode_operations erofs_fast_symlink_iops = {
 #ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
 #endif
+	.get_acl = erofs_get_acl,
 };
 
diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
index 7fed1f996ab0..b1752adc5934 100644
--- a/drivers/staging/erofs/namei.c
+++ b/drivers/staging/erofs/namei.c
@@ -238,5 +238,6 @@ const struct inode_operations erofs_dir_iops = {
 #ifdef CONFIG_EROFS_FS_XATTR
 	.listxattr = erofs_listxattr,
 #endif
+	.get_acl = erofs_get_acl,
 };
 
diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
index 176fca2af379..54cd7dac0a1f 100644
--- a/drivers/staging/erofs/super.c
+++ b/drivers/staging/erofs/super.c
@@ -398,6 +398,14 @@ static int erofs_read_super(struct super_block *sb,
 	if (!silent)
 		infoln("root inode @ nid %llu", ROOT_NID(sbi));
 
+#ifdef CONFIG_EROFS_FS_POSIX_ACL
+	/* Update the POSIXACL Flag */
+	if (test_opt(sbi, POSIX_ACL))
+		sb->s_flags |= SB_POSIXACL;
+	else
+		sb->s_flags &= ~SB_POSIXACL;
+#endif
+
 #ifdef CONFIG_EROFS_FS_ZIP
 	INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC);
 #endif
diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
index 7de46690d972..6759485ae862 100644
--- a/drivers/staging/erofs/xattr.c
+++ b/drivers/staging/erofs/xattr.c
@@ -643,3 +643,40 @@ ssize_t erofs_listxattr(struct dentry *dentry,
 	return shared_listxattr(&it);
 }
 
+#ifdef CONFIG_EROFS_FS_POSIX_ACL
+struct posix_acl *erofs_get_acl(struct inode *inode, int type)
+{
+	struct posix_acl *acl;
+	int ea_prefix, rc;
+	char *value = NULL;
+
+	switch(type) {
+	case ACL_TYPE_ACCESS:
+		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS;
+		break;
+	case ACL_TYPE_DEFAULT:
+		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT;
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	rc = erofs_getxattr(inode, ea_prefix, "", NULL, 0);
+	if (rc > 0) {
+		value = kvmalloc(rc, GFP_KERNEL);
+		if (!value)
+			return ERR_PTR(-ENOMEM);
+		rc = erofs_getxattr(inode, ea_prefix, "", value, rc);
+	}
+
+	if (rc == -ENOATTR)
+		acl = NULL;
+	else if (rc < 0)
+		acl = ERR_PTR(rc);
+	else
+		acl = posix_acl_from_xattr(&init_user_ns, value, rc);
+	kvfree(value);
+	return acl;
+}
+#endif
+
diff --git a/drivers/staging/erofs/xattr.h b/drivers/staging/erofs/xattr.h
index 634dae9aaa0b..35ba5ac2139a 100644
--- a/drivers/staging/erofs/xattr.h
+++ b/drivers/staging/erofs/xattr.h
@@ -87,5 +87,11 @@ static ssize_t __maybe_unused erofs_listxattr(struct dentry *dentry,
 }
 #endif
 
+#ifdef CONFIG_EROFS_FS_POSIX_ACL
+struct posix_acl *erofs_get_acl(struct inode *inode, int type);
+#else
+#define erofs_get_acl	(NULL)
+#endif
+
 #endif
 
-- 
2.14.4

Re: [PATCH v2 1/2] staging: erofs: use xattr_prefix to wrap up

[Chao Yu]

On 2019/1/26 0:10, Gao Xiang wrote:
> Let's use xattr_prefix instead of open code.
> No logic changes.
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Chao Yu]

On 2019/1/26 0:10, Gao Xiang wrote:
> Let's add .get_acl() to read the file's acl from its xattrs
> to make POSIX ACL usable.
> 
> Here is the on-disk detail,
> fullname: system.posix_acl_access
> struct erofs_xattr_entry:
>         .e_name_len = 0
>         .e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS (2)
> 
> fullname: system.posix_acl_default
> struct erofs_xattr_entry:
> 	.e_name_len = 0
> 	.e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT (3)
> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
> change log v2:
>  - add the missing SB_POSIXACL flag;
>  - fix the on-disk detail, use .e_name_len == 0 and proper prefix value;
>  - tested ok with xattr enabled erofs_mkfs;
> 
>  .../erofs/Documentation/filesystems/erofs.txt      |  2 ++
>  drivers/staging/erofs/inode.c                      |  3 ++
>  drivers/staging/erofs/namei.c                      |  1 +
>  drivers/staging/erofs/super.c                      |  8 +++++
>  drivers/staging/erofs/xattr.c                      | 37 ++++++++++++++++++++++
>  drivers/staging/erofs/xattr.h                      |  6 ++++
>  6 files changed, 57 insertions(+)
> 
> diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> index 803988d74c21..961ec4da7705 100644
> --- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> +++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> @@ -36,6 +36,8 @@ Here is the main features of EROFS:
>  
>   - Support xattr inline and tail-end data inline for all files;
>  
> + - Support POSIX.1e ACLs by using xattrs;
> +
>   - Support transparent file compression as an option:
>     LZ4 algorithm with 4 KB fixed-output compression for high performance;
>  
> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> index 4f04f7c38cf2..924b8dfc7a8f 100644
> --- a/drivers/staging/erofs/inode.c
> +++ b/drivers/staging/erofs/inode.c
> @@ -287,6 +287,7 @@ const struct inode_operations erofs_generic_iops = {
>  #ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
>  #endif
> +	.get_acl = erofs_get_acl,
>  };
>  
>  const struct inode_operations erofs_symlink_iops = {
> @@ -294,6 +295,7 @@ const struct inode_operations erofs_symlink_iops = {
>  #ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
>  #endif
> +	.get_acl = erofs_get_acl,
>  };
>  
>  const struct inode_operations erofs_fast_symlink_iops = {
> @@ -301,5 +303,6 @@ const struct inode_operations erofs_fast_symlink_iops = {
>  #ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
>  #endif
> +	.get_acl = erofs_get_acl,
>  };
>  
> diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
> index 7fed1f996ab0..b1752adc5934 100644
> --- a/drivers/staging/erofs/namei.c
> +++ b/drivers/staging/erofs/namei.c
> @@ -238,5 +238,6 @@ const struct inode_operations erofs_dir_iops = {
>  #ifdef CONFIG_EROFS_FS_XATTR
>  	.listxattr = erofs_listxattr,
>  #endif
> +	.get_acl = erofs_get_acl,
>  };
>  
> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> index 176fca2af379..54cd7dac0a1f 100644
> --- a/drivers/staging/erofs/super.c
> +++ b/drivers/staging/erofs/super.c
> @@ -398,6 +398,14 @@ static int erofs_read_super(struct super_block *sb,
>  	if (!silent)
>  		infoln("root inode @ nid %llu", ROOT_NID(sbi));
>  
> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> +	/* Update the POSIXACL Flag */
> +	if (test_opt(sbi, POSIX_ACL))
> +		sb->s_flags |= SB_POSIXACL;
> +	else
> +		sb->s_flags &= ~SB_POSIXACL;
> +#endif
> +
>  #ifdef CONFIG_EROFS_FS_ZIP
>  	INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC);
>  #endif
> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
> index 7de46690d972..6759485ae862 100644
> --- a/drivers/staging/erofs/xattr.c
> +++ b/drivers/staging/erofs/xattr.c
> @@ -643,3 +643,40 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>  	return shared_listxattr(&it);
>  }
>  
> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> +struct posix_acl *erofs_get_acl(struct inode *inode, int type)
> +{
> +	struct posix_acl *acl;
> +	int ea_prefix, rc;
> +	char *value = NULL;
> +
> +	switch(type) {
> +	case ACL_TYPE_ACCESS:
> +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS;
> +		break;
> +	case ACL_TYPE_DEFAULT:
> +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT;
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	rc = erofs_getxattr(inode, ea_prefix, "", NULL, 0);
> +	if (rc > 0) {
> +		value = kvmalloc(rc, GFP_KERNEL);

erofs_kmalloc() is enough?

Thanks,

> +		if (!value)
> +			return ERR_PTR(-ENOMEM);
> +		rc = erofs_getxattr(inode, ea_prefix, "", value, rc);
> +	}
> +
> +	if (rc == -ENOATTR)
> +		acl = NULL;
> +	else if (rc < 0)
> +		acl = ERR_PTR(rc);
> +	else
> +		acl = posix_acl_from_xattr(&init_user_ns, value, rc);
> +	kvfree(value);
> +	return acl;
> +}
> +#endif
> +
> diff --git a/drivers/staging/erofs/xattr.h b/drivers/staging/erofs/xattr.h
> index 634dae9aaa0b..35ba5ac2139a 100644
> --- a/drivers/staging/erofs/xattr.h
> +++ b/drivers/staging/erofs/xattr.h
> @@ -87,5 +87,11 @@ static ssize_t __maybe_unused erofs_listxattr(struct dentry *dentry,
>  }
>  #endif
>  
> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> +struct posix_acl *erofs_get_acl(struct inode *inode, int type);
> +#else
> +#define erofs_get_acl	(NULL)
> +#endif
> +
>  #endif
>  
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Gao Xiang]

Hi Chao,

On 2019/1/26 10:48, Chao Yu wrote:
> On 2019/1/26 0:10, Gao Xiang wrote:
>> Let's add .get_acl() to read the file's acl from its xattrs
>> to make POSIX ACL usable.
>>
>> Here is the on-disk detail,
>> fullname: system.posix_acl_access
>> struct erofs_xattr_entry:
>>         .e_name_len = 0
>>         .e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS (2)
>>
>> fullname: system.posix_acl_default
>> struct erofs_xattr_entry:
>> 	.e_name_len = 0
>> 	.e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT (3)
>>
>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>> ---
>> change log v2:
>>  - add the missing SB_POSIXACL flag;
>>  - fix the on-disk detail, use .e_name_len == 0 and proper prefix value;
>>  - tested ok with xattr enabled erofs_mkfs;
>>
>>  .../erofs/Documentation/filesystems/erofs.txt      |  2 ++
>>  drivers/staging/erofs/inode.c                      |  3 ++
>>  drivers/staging/erofs/namei.c                      |  1 +
>>  drivers/staging/erofs/super.c                      |  8 +++++
>>  drivers/staging/erofs/xattr.c                      | 37 ++++++++++++++++++++++
>>  drivers/staging/erofs/xattr.h                      |  6 ++++
>>  6 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> index 803988d74c21..961ec4da7705 100644
>> --- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> +++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
>> @@ -36,6 +36,8 @@ Here is the main features of EROFS:
>>  
>>   - Support xattr inline and tail-end data inline for all files;
>>  
>> + - Support POSIX.1e ACLs by using xattrs;
>> +
>>   - Support transparent file compression as an option:
>>     LZ4 algorithm with 4 KB fixed-output compression for high performance;
>>  
>> diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
>> index 4f04f7c38cf2..924b8dfc7a8f 100644
>> --- a/drivers/staging/erofs/inode.c
>> +++ b/drivers/staging/erofs/inode.c
>> @@ -287,6 +287,7 @@ const struct inode_operations erofs_generic_iops = {
>>  #ifdef CONFIG_EROFS_FS_XATTR
>>  	.listxattr = erofs_listxattr,
>>  #endif
>> +	.get_acl = erofs_get_acl,
>>  };
>>  
>>  const struct inode_operations erofs_symlink_iops = {
>> @@ -294,6 +295,7 @@ const struct inode_operations erofs_symlink_iops = {
>>  #ifdef CONFIG_EROFS_FS_XATTR
>>  	.listxattr = erofs_listxattr,
>>  #endif
>> +	.get_acl = erofs_get_acl,
>>  };
>>  
>>  const struct inode_operations erofs_fast_symlink_iops = {
>> @@ -301,5 +303,6 @@ const struct inode_operations erofs_fast_symlink_iops = {
>>  #ifdef CONFIG_EROFS_FS_XATTR
>>  	.listxattr = erofs_listxattr,
>>  #endif
>> +	.get_acl = erofs_get_acl,
>>  };
>>  
>> diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
>> index 7fed1f996ab0..b1752adc5934 100644
>> --- a/drivers/staging/erofs/namei.c
>> +++ b/drivers/staging/erofs/namei.c
>> @@ -238,5 +238,6 @@ const struct inode_operations erofs_dir_iops = {
>>  #ifdef CONFIG_EROFS_FS_XATTR
>>  	.listxattr = erofs_listxattr,
>>  #endif
>> +	.get_acl = erofs_get_acl,
>>  };
>>  
>> diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
>> index 176fca2af379..54cd7dac0a1f 100644
>> --- a/drivers/staging/erofs/super.c
>> +++ b/drivers/staging/erofs/super.c
>> @@ -398,6 +398,14 @@ static int erofs_read_super(struct super_block *sb,
>>  	if (!silent)
>>  		infoln("root inode @ nid %llu", ROOT_NID(sbi));
>>  
>> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
>> +	/* Update the POSIXACL Flag */
>> +	if (test_opt(sbi, POSIX_ACL))
>> +		sb->s_flags |= SB_POSIXACL;
>> +	else
>> +		sb->s_flags &= ~SB_POSIXACL;
>> +#endif
>> +
>>  #ifdef CONFIG_EROFS_FS_ZIP
>>  	INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC);
>>  #endif
>> diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
>> index 7de46690d972..6759485ae862 100644
>> --- a/drivers/staging/erofs/xattr.c
>> +++ b/drivers/staging/erofs/xattr.c
>> @@ -643,3 +643,40 @@ ssize_t erofs_listxattr(struct dentry *dentry,
>>  	return shared_listxattr(&it);
>>  }
>>  
>> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
>> +struct posix_acl *erofs_get_acl(struct inode *inode, int type)
>> +{
>> +	struct posix_acl *acl;
>> +	int ea_prefix, rc;
>> +	char *value = NULL;
>> +
>> +	switch(type) {
>> +	case ACL_TYPE_ACCESS:
>> +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS;
>> +		break;
>> +	case ACL_TYPE_DEFAULT:
>> +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT;
>> +		break;
>> +	default:
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	rc = erofs_getxattr(inode, ea_prefix, "", NULL, 0);
>> +	if (rc > 0) {
>> +		value = kvmalloc(rc, GFP_KERNEL);
> 
> erofs_kmalloc() is enough?

I think kmalloc is enough for this case, but I think kvmalloc is not
bad as well.

Actually I want to introduce erofs_kvmalloc() in the later patch and
use it in general except for the context which is unsuitable for vmalloc..

Let me send v3 if it is unnecessary...

Thanks,
Gao Xiang

> 
> Thanks,
> 
>> +		if (!value)
>> +			return ERR_PTR(-ENOMEM);
>> +		rc = erofs_getxattr(inode, ea_prefix, "", value, rc);
>> +	}
>> +
>> +	if (rc == -ENOATTR)
>> +		acl = NULL;
>> +	else if (rc < 0)
>> +		acl = ERR_PTR(rc);
>> +	else
>> +		acl = posix_acl_from_xattr(&init_user_ns, value, rc);
>> +	kvfree(value);
>> +	return acl;
>> +}
>> +#endif
>> +
>> diff --git a/drivers/staging/erofs/xattr.h b/drivers/staging/erofs/xattr.h
>> index 634dae9aaa0b..35ba5ac2139a 100644
>> --- a/drivers/staging/erofs/xattr.h
>> +++ b/drivers/staging/erofs/xattr.h
>> @@ -87,5 +87,11 @@ static ssize_t __maybe_unused erofs_listxattr(struct dentry *dentry,
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_EROFS_FS_POSIX_ACL
>> +struct posix_acl *erofs_get_acl(struct inode *inode, int type);
>> +#else
>> +#define erofs_get_acl	(NULL)
>> +#endif
>> +
>>  #endif
>>  
>>
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Dan Carpenter]

On Sat, Jan 26, 2019 at 10:48:53AM +0800, Chao Yu wrote:
> On 2019/1/26 0:10, Gao Xiang wrote:
> > Let's add .get_acl() to read the file's acl from its xattrs
> > to make POSIX ACL usable.
> > 
> > Here is the on-disk detail,
> > fullname: system.posix_acl_access
> > struct erofs_xattr_entry:
> >         .e_name_len = 0
> >         .e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS (2)
> > 
> > fullname: system.posix_acl_default
> > struct erofs_xattr_entry:
> > 	.e_name_len = 0
> > 	.e_name_index = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT (3)
> > 
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---
> > change log v2:
> >  - add the missing SB_POSIXACL flag;
> >  - fix the on-disk detail, use .e_name_len == 0 and proper prefix value;
> >  - tested ok with xattr enabled erofs_mkfs;
> 
> >  .../erofs/Documentation/filesystems/erofs.txt      |  2 ++
> >  drivers/staging/erofs/inode.c                      |  3 ++
> >  drivers/staging/erofs/namei.c                      |  1 +
> >  drivers/staging/erofs/super.c                      |  8 +++++
> >  drivers/staging/erofs/xattr.c                      | 37 ++++++++++++++++++++++
> >  drivers/staging/erofs/xattr.h                      |  6 ++++
> >  6 files changed, 57 insertions(+)
> > 
> > diff --git a/drivers/staging/erofs/Documentation/filesystems/erofs.txt b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> > index 803988d74c21..961ec4da7705 100644
> > --- a/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> > +++ b/drivers/staging/erofs/Documentation/filesystems/erofs.txt
> > @@ -36,6 +36,8 @@ Here is the main features of EROFS:
> >  
> >   - Support xattr inline and tail-end data inline for all files;
> >  
> > + - Support POSIX.1e ACLs by using xattrs;
> > +
> >   - Support transparent file compression as an option:
> >     LZ4 algorithm with 4 KB fixed-output compression for high performance;
> >  
> > diff --git a/drivers/staging/erofs/inode.c b/drivers/staging/erofs/inode.c
> > index 4f04f7c38cf2..924b8dfc7a8f 100644
> > --- a/drivers/staging/erofs/inode.c
> > +++ b/drivers/staging/erofs/inode.c
> > @@ -287,6 +287,7 @@ const struct inode_operations erofs_generic_iops = {
> >  #ifdef CONFIG_EROFS_FS_XATTR
> >  	.listxattr = erofs_listxattr,
> >  #endif
> > +	.get_acl = erofs_get_acl,
> >  };
> >  
> >  const struct inode_operations erofs_symlink_iops = {
> > @@ -294,6 +295,7 @@ const struct inode_operations erofs_symlink_iops = {
> >  #ifdef CONFIG_EROFS_FS_XATTR
> >  	.listxattr = erofs_listxattr,
> >  #endif
> > +	.get_acl = erofs_get_acl,
> >  };
> >  
> >  const struct inode_operations erofs_fast_symlink_iops = {
> > @@ -301,5 +303,6 @@ const struct inode_operations erofs_fast_symlink_iops = {
> >  #ifdef CONFIG_EROFS_FS_XATTR
> >  	.listxattr = erofs_listxattr,
> >  #endif
> > +	.get_acl = erofs_get_acl,
> >  };
> >  
> > diff --git a/drivers/staging/erofs/namei.c b/drivers/staging/erofs/namei.c
> > index 7fed1f996ab0..b1752adc5934 100644
> > --- a/drivers/staging/erofs/namei.c
> > +++ b/drivers/staging/erofs/namei.c
> > @@ -238,5 +238,6 @@ const struct inode_operations erofs_dir_iops = {
> >  #ifdef CONFIG_EROFS_FS_XATTR
> >  	.listxattr = erofs_listxattr,
> >  #endif
> > +	.get_acl = erofs_get_acl,
> >  };
> >  
> > diff --git a/drivers/staging/erofs/super.c b/drivers/staging/erofs/super.c
> > index 176fca2af379..54cd7dac0a1f 100644
> > --- a/drivers/staging/erofs/super.c
> > +++ b/drivers/staging/erofs/super.c
> > @@ -398,6 +398,14 @@ static int erofs_read_super(struct super_block *sb,
> >  	if (!silent)
> >  		infoln("root inode @ nid %llu", ROOT_NID(sbi));
> >  
> > +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> > +	/* Update the POSIXACL Flag */
> > +	if (test_opt(sbi, POSIX_ACL))
> > +		sb->s_flags |= SB_POSIXACL;
> > +	else
> > +		sb->s_flags &= ~SB_POSIXACL;
> > +#endif
> > +
> >  #ifdef CONFIG_EROFS_FS_ZIP
> >  	INIT_RADIX_TREE(&sbi->workstn_tree, GFP_ATOMIC);
> >  #endif
> > diff --git a/drivers/staging/erofs/xattr.c b/drivers/staging/erofs/xattr.c
> > index 7de46690d972..6759485ae862 100644
> > --- a/drivers/staging/erofs/xattr.c
> > +++ b/drivers/staging/erofs/xattr.c
> > @@ -643,3 +643,40 @@ ssize_t erofs_listxattr(struct dentry *dentry,
> >  	return shared_listxattr(&it);
> >  }
> >  
> > +#ifdef CONFIG_EROFS_FS_POSIX_ACL
> > +struct posix_acl *erofs_get_acl(struct inode *inode, int type)
> > +{
> > +	struct posix_acl *acl;
> > +	int ea_prefix, rc;
> > +	char *value = NULL;
> > +
> > +	switch(type) {
> > +	case ACL_TYPE_ACCESS:
> > +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_ACCESS;
> > +		break;
> > +	case ACL_TYPE_DEFAULT:
> > +		ea_prefix = EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT;
> > +		break;
> > +	default:
> > +		return ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	rc = erofs_getxattr(inode, ea_prefix, "", NULL, 0);
> > +	if (rc > 0) {
> > +		value = kvmalloc(rc, GFP_KERNEL);
> 
> erofs_kmalloc() is enough?
> 
> Thanks,
> 

Hopefully, regular kmalloc() is enough.

Do really need the erofs_kmalloc() function?  Regular kmalloc() has
fault injection already.  Have you tried to use it?

regards,
dan carpenter

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Gao Xiang]

Hi Dan,

On 2019/1/28 21:33, Dan Carpenter wrote:
> Hopefully, regular kmalloc() is enough.
> 
> Do really need the erofs_kmalloc() function?  Regular kmalloc() has
> fault injection already.  Have you tried to use it?

The fault injection subsystem was introduced in the initial upstreamed
EROFS version, which is taken from f2fs by Chao Yu.

Fault injection is important to test fs.. However, I have no more idea
about the future plan of this feature...Follow f2fs or use the general
fault injection...

There already exists erofs_kmalloc, thus I think it's both ok to use
erofs_kmalloc or kmalloc for this patch at least.

Chao, any idea about this?

Thanks,
Gao Xiang

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Dan Carpenter]

The point is, that people shouldn't recreate core code that already
exists.  At least try it out and have an explanation why the other code
doesn't work.

In other projects, the default is to keep code around once it has been
written but in staging the default choice is to delete the code unless
there is an explanation.

regards,
dan carpenter

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Gao Xiang]

On 2019/1/28 22:28, Dan Carpenter wrote:
> The point is, that people shouldn't recreate core code that already
> exists.  At least try it out and have an explanation why the other code
> doesn't work.
> 
> In other projects, the default is to keep code around once it has been
> written but in staging the default choice is to delete the code unless
> there is an explanation.

That is why I need Chao's idea about the future of EROFS fault injection.

As far as I know, the fault injection of f2fs also introduces after
the general fault injection, and the original reason is to test separated
partitions with different fault injection rate/type simultaneously, as follows:

commit 1ecc0c5c50ce8834f7e35b63be7480bf1aaa4155
Author: Chao Yu <yuchao0@huawei.com>
Date:   Fri Sep 23 21:30:09 2016 +0800

    f2fs: support configuring fault injection per superblock

    Previously, we only support global fault injection configuration, so that
    when we configure type/rate of fault injection through sysfs, mount
    option, it will influence all f2fs partition which is being used.

    It is not make sence, since it will be not convenient if developer want
    to test separated partitions with different fault injection rate/type
    simultaneously, also it's not possible to enable fault injection in one
    partition and disable fault injection in other one.

    >From now on, we move global configuration of fault injection in module
    into per-superblock, hence injection testing can be more flexible.

    Signed-off-by: Chao Yu <yuchao0@huawei.com>
    Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

For this patch, I could resend v4 to change erofs_kmalloc->kmalloc
if you like, it isn't an important stuff for this patch. However,
I'd like to get more ideas about the fault injection.

Thanks,
Gao Xiang

> 
> regards,
> dan carpenter
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Chao Yu]

Hi Dan and Xiang,

On 2019-1-28 21:48, Gao Xiang wrote:
> Hi Dan,
> 
> On 2019/1/28 21:33, Dan Carpenter wrote:
>> Hopefully, regular kmalloc() is enough.
>>
>> Do really need the erofs_kmalloc() function?  Regular kmalloc() has
>> fault injection already.  Have you tried to use it?

Yes, I think we'd better to use erofs_kmalloc(). :)

Actually, fault injection in erofs_kmalloc only affect erofs module, we can
expect that the range of fault can be limited in erofs code, rather than whole
kernel, so the test point can be aimed at more accurately.

> 
> The fault injection subsystem was introduced in the initial upstreamed
> EROFS version, which is taken from f2fs by Chao Yu.
> 
> Fault injection is important to test fs.. However, I have no more idea
> about the future plan of this feature...Follow f2fs or use the general
> fault injection...

I guess following f2fs is not bad.

Thanks,

> 
> There already exists erofs_kmalloc, thus I think it's both ok to use
> erofs_kmalloc or kmalloc for this patch at least.
> 
> Chao, any idea about this?
> 
> Thanks,
> Gao Xiang
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Dan Carpenter]

On Tue, Jan 29, 2019 at 12:41:55AM +0800, Chao Yu wrote:
> Hi Dan and Xiang,
> 
> On 2019-1-28 21:48, Gao Xiang wrote:
> > Hi Dan,
> > 
> > On 2019/1/28 21:33, Dan Carpenter wrote:
> >> Hopefully, regular kmalloc() is enough.
> >>
> >> Do really need the erofs_kmalloc() function?  Regular kmalloc() has
> >> fault injection already.  Have you tried to use it?
> 
> Yes, I think we'd better to use erofs_kmalloc(). :)
> 
> Actually, fault injection in erofs_kmalloc only affect erofs module, we can
> expect that the range of fault can be limited in erofs code, rather than whole
> kernel, so the test point can be aimed at more accurately.
> 

Are you serious?  The standard fault injection doesn't do that???

Please fix it instead of creating a duplicate better implementation
which only your filesystem can use.  I would have thought that obviously
any fault injection framework could at least be configured to test
specific code...

regards,
dan carpenter

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Gao Xiang]

Hi,

On 2019/1/29 2:30, Dan Carpenter wrote:
> Are you serious?  The standard fault injection doesn't do that???
> 
> Please fix it instead of creating a duplicate better implementation
> which only your filesystem can use.  I would have thought that obviously
> any fault injection framework could at least be configured to test
> specific code...
> 
> regards,
> dan carpenter

Since Chao is now on the short business trip, I will resend v4 and change
erofs_kmalloc into kmalloc temporarily to make sure ACL is usable first
in the staging tree.

As for the EROFS fault injection, let's wait Chao's reply...

Thanks,
Gao Xiang

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Chao Yu]

Sorry for the delay due to business travel.

On 2019/1/29 2:30, Dan Carpenter wrote:
> On Tue, Jan 29, 2019 at 12:41:55AM +0800, Chao Yu wrote:
>> Hi Dan and Xiang,
>>
>> On 2019-1-28 21:48, Gao Xiang wrote:
>>> Hi Dan,
>>>
>>> On 2019/1/28 21:33, Dan Carpenter wrote:
>>>> Hopefully, regular kmalloc() is enough.
>>>>
>>>> Do really need the erofs_kmalloc() function?  Regular kmalloc() has
>>>> fault injection already.  Have you tried to use it?
>>
>> Yes, I think we'd better to use erofs_kmalloc(). :)
>>
>> Actually, fault injection in erofs_kmalloc only affect erofs module, we can
>> expect that the range of fault can be limited in erofs code, rather than whole
>> kernel, so the test point can be aimed at more accurately.
>>
> 
> Are you serious?  The standard fault injection doesn't do that???

Oh, I just realized the common fault injection can inject into specified
module with function granularity, sorry.

> 
> Please fix it instead of creating a duplicate better implementation
> which only your filesystem can use.  I would have thought that obviously

I agreed that it will be good to make common fault injection better,
covering more cases, so that it can benefit all modules which need fault
injection functionality. But rather than injecting kmalloc, there will be
other injection demands from erofs/f2fs, like injecting in the middle of
their specified function, how could we do that? Could you give us advice?

Thanks,

> any fault injection framework could at least be configured to test
> specific code...
> 
> regards,
> dan carpenter
> 
> 
> .
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Chao Yu]

Hi Dan,

Any suggestion?

On 2019/2/3 10:52, Chao Yu wrote:
> Sorry for the delay due to business travel.
> 
> On 2019/1/29 2:30, Dan Carpenter wrote:
>> On Tue, Jan 29, 2019 at 12:41:55AM +0800, Chao Yu wrote:
>>> Hi Dan and Xiang,
>>>
>>> On 2019-1-28 21:48, Gao Xiang wrote:
>>>> Hi Dan,
>>>>
>>>> On 2019/1/28 21:33, Dan Carpenter wrote:
>>>>> Hopefully, regular kmalloc() is enough.
>>>>>
>>>>> Do really need the erofs_kmalloc() function?  Regular kmalloc() has
>>>>> fault injection already.  Have you tried to use it?
>>>
>>> Yes, I think we'd better to use erofs_kmalloc(). :)
>>>
>>> Actually, fault injection in erofs_kmalloc only affect erofs module, we can
>>> expect that the range of fault can be limited in erofs code, rather than whole
>>> kernel, so the test point can be aimed at more accurately.
>>>
>>
>> Are you serious?  The standard fault injection doesn't do that???
> 
> Oh, I just realized the common fault injection can inject into specified
> module with function granularity, sorry.
> 
>>
>> Please fix it instead of creating a duplicate better implementation
>> which only your filesystem can use.  I would have thought that obviously
> 
> I agreed that it will be good to make common fault injection better,
> covering more cases, so that it can benefit all modules which need fault
> injection functionality. But rather than injecting kmalloc, there will be
> other injection demands from erofs/f2fs, like injecting in the middle of
> their specified function, how could we do that? Could you give us advice?
> 
> Thanks,
> 
>> any fault injection framework could at least be configured to test
>> specific code...
>>
>> regards,
>> dan carpenter
>>
>>
>> .
>>
> 
> 
> .
> 

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Dan Carpenter]

On Fri, Feb 15, 2019 at 10:10:34AM +0800, Chao Yu wrote:
> Hi Dan,
> 
> Any suggestion?
> 

I won't NAK whatever you decide.  But my opinion is that you should
just use normal kernel memory allocators even though it means you have
to use two different fault injection frameworks.

Over time it would be good to improve and expand the standard kernel
error injection frameworks to cover more types.

regards,
dan carpenter

Re: [PATCH v2 2/2] staging: erofs: complete POSIX ACL support

[Chao Yu]

On 2019/2/15 15:36, Dan Carpenter wrote:
> On Fri, Feb 15, 2019 at 10:10:34AM +0800, Chao Yu wrote:
>> Hi Dan,
>>
>> Any suggestion?
>>
> 
> I won't NAK whatever you decide.  But my opinion is that you should
> just use normal kernel memory allocators even though it means you have
> to use two different fault injection frameworks.

Thanks for the suggestion.

There is only one injection type in erofs injection framework, I don't want
to remove all framework codes with kmalloc type, so I think we can do this
step by step:
a. use erofs_kamloc to keep all codes consistent.
b. add at least one erofs private injection type.
c. remove erofs_kmalloc and related type.

> 
> Over time it would be good to improve and expand the standard kernel
> error injection frameworks to cover more types.

Agreed.

Thanks,

> 
> regards,
> dan carpenter
> 
> 
>