[PATCH] btrfs-progs: tests/common: simplify check_prereq_btrfsacl

[PATCH] btrfs-progs: tests/common: simplify check_prereq_btrfsacl

[Anand Jain]

With the kernel patch
   [PATCH] btrfs: sysfs: display ACL support

We can now check if ACL is compiled without requiring a btrfs device.
However, to ensure backward compatibility, the old code remains in place.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
Depends on the patch-set:
   [PATCH 0/6] btrfs-progs: tests: fix no acl support

 tests/common | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/common b/tests/common
index afcbfc8f09ea..303282c3ecf8 100644
--- a/tests/common
+++ b/tests/common
@@ -578,6 +578,14 @@ setup_root_helper()
 # Check if btrfs is compiled with CONFIG_BTRFS_FS_POSIX_ACL, requires TEST_DEV.
 check_prereq_btrfsacl()
 {
+	if [ -f /sys/fs/btrfs/features/acl ]; then
+		if grep -q 0 /sys/fs/btrfs/features/acl; then
+			_not_run "acl is not compiled"
+		fi
+		return
+	fi
+
+	# It is an older kernel without acl sysfs interface.
 	run_check_mkfs_test_dev
 	run_check_mount_test_dev
 
-- 
2.31.1

[PATCH] btrfs: sysfs: display ACL support

[Anand Jain]

ACL support is dependent on the compile-time configuration option
CONFIG_BTRFS_FS_POSIX_ACL. Prior to mounting a btrfs filesystem, it is not
possible to determine whether ACL support has been compiled in. To address
this, add a sysfs interface, /sys/fs/btrfs/features/acl, and check for ACL
support in the system's btrfs.

  To determine ACL support:

  Return 0 indicates ACL is not supported:
    $ cat /sys/fs/btrfs/features/acl
    0

  Return 1 indicates ACL is supported:
    $ cat /sys/fs/btrfs/features/acl
    1

IMO, this is a better approach, so that we also know if kernel is older.

  On an older kernel
    $ ls /sys/fs/btrfs/features/acl
    ls: cannot access '/sys/fs/btrfs/features/acl': No such file or directory

    mount a btrfs filesystem
    $ cat /proc/self/mounts | grep btrfs | grep -q noacl
    $ echo $?
    0

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/sysfs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 25294e624851..25b311bb47ac 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -414,6 +414,21 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
 BTRFS_ATTR(static_feature, supported_sectorsizes,
 	   supported_sectorsizes_show);
 
+static ssize_t acl_show(struct kobject *kobj, struct kobj_attribute *a,
+			char *buf)
+{
+	ssize_t ret = 0;
+
+#ifdef CONFIG_BTRFS_FS_POSIX_ACL
+	ret += sysfs_emit_at(buf, ret, "%d\n", 1);
+#else
+	ret += sysfs_emit_at(buf, ret, "%d\n", 0);
+#endif
+
+	return ret;
+}
+BTRFS_ATTR(static_feature, acl, acl_show);
+
 /*
  * Features which only depend on kernel version.
  *
@@ -426,6 +441,7 @@ static struct attribute *btrfs_supported_static_feature_attrs[] = {
 	BTRFS_ATTR_PTR(static_feature, send_stream_version),
 	BTRFS_ATTR_PTR(static_feature, supported_rescue_options),
 	BTRFS_ATTR_PTR(static_feature, supported_sectorsizes),
+	BTRFS_ATTR_PTR(static_feature, acl),
 	NULL
 };
 
-- 
2.31.1

Re: [PATCH] btrfs: sysfs: display ACL support

[David Sterba]

On Tue, Jun 20, 2023 at 04:55:09PM +0800, Anand Jain wrote:
> ACL support is dependent on the compile-time configuration option
> CONFIG_BTRFS_FS_POSIX_ACL. Prior to mounting a btrfs filesystem, it is not
> possible to determine whether ACL support has been compiled in. To address
> this, add a sysfs interface, /sys/fs/btrfs/features/acl, and check for ACL
> support in the system's btrfs.

For completeness we could add it there but how many systems are there
that don't compile in ACLs? Typically this is for embedded environments
and it's probably global, not just for btrfs.

We can't drop CONFIG_BTRFS_FS_POSIX_ACL to make it unconditional as it
depends on the VFS interface but at least we could somehow use 
CONFIG_FS_POSIX_ACL directly.

Re: [PATCH] btrfs: sysfs: display ACL support

[Anand Jain]

On 6/20/23 18:25, David Sterba wrote:
> On Tue, Jun 20, 2023 at 04:55:09PM +0800, Anand Jain wrote:
>> ACL support is dependent on the compile-time configuration option
>> CONFIG_BTRFS_FS_POSIX_ACL. Prior to mounting a btrfs filesystem, it is not
>> possible to determine whether ACL support has been compiled in. To address
>> this, add a sysfs interface, /sys/fs/btrfs/features/acl, and check for ACL
>> support in the system's btrfs.
> 
> For completeness we could add it there but how many systems are there
> that don't compile in ACLs? Typically this is for embedded environments
> and it's probably global, not just for btrfs.
> 
> We can't drop CONFIG_BTRFS_FS_POSIX_ACL to make it unconditional as it
> depends on the VFS interface but at least we could somehow use
> CONFIG_FS_POSIX_ACL directly.


Directly using CONFIG_FS_POSIX_ACL is not recommended, as indicated
in fs/Kconfig file:

------
# Posix ACL utility routines
#
# Note: Posix ACLs can be implemented without these helpers.  Never use
# this symbol for ifdefs in core code.
#
config FS_POSIX_ACL
         def_bool n
------

There is no global switch for FS_POSIX_ACL, as it is selected by
individual modules (approximately 20) that enable ACL functionality
in it.

All file systems provide configurable ACLs, possibly for rare use cases.
IMO, providing a sysfs interface for better tool compatibility is a
good idea.

Re: [PATCH] btrfs: sysfs: display ACL support

[David Sterba]

On Tue, Jun 20, 2023 at 04:55:09PM +0800, Anand Jain wrote:
> ACL support is dependent on the compile-time configuration option
> CONFIG_BTRFS_FS_POSIX_ACL. Prior to mounting a btrfs filesystem, it is not
> possible to determine whether ACL support has been compiled in. To address
> this, add a sysfs interface, /sys/fs/btrfs/features/acl, and check for ACL
> support in the system's btrfs.
> 
>   To determine ACL support:
> 
>   Return 0 indicates ACL is not supported:
>     $ cat /sys/fs/btrfs/features/acl
>     0
> 
>   Return 1 indicates ACL is supported:
>     $ cat /sys/fs/btrfs/features/acl
>     1
> 
> IMO, this is a better approach, so that we also know if kernel is older.
> 
>   On an older kernel
>     $ ls /sys/fs/btrfs/features/acl
>     ls: cannot access '/sys/fs/btrfs/features/acl': No such file or directory
> 
>     mount a btrfs filesystem
>     $ cat /proc/self/mounts | grep btrfs | grep -q noacl
>     $ echo $?
>     0
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Added to misc-next, thanks.

> ---
>  fs/btrfs/sysfs.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 25294e624851..25b311bb47ac 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -414,6 +414,21 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>  BTRFS_ATTR(static_feature, supported_sectorsizes,
>  	   supported_sectorsizes_show);
>  
> +static ssize_t acl_show(struct kobject *kobj, struct kobj_attribute *a,
> +			char *buf)
> +{
> +	ssize_t ret = 0;

The simple callback can return directly sysfs_emit_at without the return
variable. Updated.

> +
> +#ifdef CONFIG_BTRFS_FS_POSIX_ACL
> +	ret += sysfs_emit_at(buf, ret, "%d\n", 1);
> +#else
> +	ret += sysfs_emit_at(buf, ret, "%d\n", 0);
> +#endif
> +
> +	return ret;
> +}
> +BTRFS_ATTR(static_feature, acl, acl_show);
> +
>  /*
>   * Features which only depend on kernel version.
>   *
> @@ -426,6 +441,7 @@ static struct attribute *btrfs_supported_static_feature_attrs[] = {
>  	BTRFS_ATTR_PTR(static_feature, send_stream_version),
>  	BTRFS_ATTR_PTR(static_feature, supported_rescue_options),
>  	BTRFS_ATTR_PTR(static_feature, supported_sectorsizes),
> +	BTRFS_ATTR_PTR(static_feature, acl),

Please keep the features sorted alphabetically, moved to the beginning
of the list.

>  	NULL
>  };

Re: [PATCH] btrfs: sysfs: display ACL support

[Anand Jain]

On 30/06/2023 00:41, David Sterba wrote:
> On Tue, Jun 20, 2023 at 04:55:09PM +0800, Anand Jain wrote:
>> ACL support is dependent on the compile-time configuration option
>> CONFIG_BTRFS_FS_POSIX_ACL. Prior to mounting a btrfs filesystem, it is not
>> possible to determine whether ACL support has been compiled in. To address
>> this, add a sysfs interface, /sys/fs/btrfs/features/acl, and check for ACL
>> support in the system's btrfs.
>>
>>    To determine ACL support:
>>
>>    Return 0 indicates ACL is not supported:
>>      $ cat /sys/fs/btrfs/features/acl
>>      0
>>
>>    Return 1 indicates ACL is supported:
>>      $ cat /sys/fs/btrfs/features/acl
>>      1
>>
>> IMO, this is a better approach, so that we also know if kernel is older.
>>
>>    On an older kernel
>>      $ ls /sys/fs/btrfs/features/acl
>>      ls: cannot access '/sys/fs/btrfs/features/acl': No such file or directory
>>
>>      mount a btrfs filesystem
>>      $ cat /proc/self/mounts | grep btrfs | grep -q noacl
>>      $ echo $?
>>      0
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Added to misc-next, thanks.
> 
>> ---
>>   fs/btrfs/sysfs.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
>> index 25294e624851..25b311bb47ac 100644
>> --- a/fs/btrfs/sysfs.c
>> +++ b/fs/btrfs/sysfs.c
>> @@ -414,6 +414,21 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
>>   BTRFS_ATTR(static_feature, supported_sectorsizes,
>>   	   supported_sectorsizes_show);
>>   
>> +static ssize_t acl_show(struct kobject *kobj, struct kobj_attribute *a,
>> +			char *buf)
>> +{
>> +	ssize_t ret = 0;
> 
> The simple callback can return directly sysfs_emit_at without the return
> variable. Updated.
> 

Changes in mics-next look good and much cooler.


>> +
>> +#ifdef CONFIG_BTRFS_FS_POSIX_ACL
>> +	ret += sysfs_emit_at(buf, ret, "%d\n", 1);
>> +#else
>> +	ret += sysfs_emit_at(buf, ret, "%d\n", 0);
>> +#endif
>> +
>> +	return ret;
>> +}
>> +BTRFS_ATTR(static_feature, acl, acl_show);
>> +
>>   /*
>>    * Features which only depend on kernel version.
>>    *
>> @@ -426,6 +441,7 @@ static struct attribute *btrfs_supported_static_feature_attrs[] = {
>>   	BTRFS_ATTR_PTR(static_feature, send_stream_version),
>>   	BTRFS_ATTR_PTR(static_feature, supported_rescue_options),
>>   	BTRFS_ATTR_PTR(static_feature, supported_sectorsizes),
>> +	BTRFS_ATTR_PTR(static_feature, acl),
> 
> Please keep the features sorted alphabetically, moved to the beginning
> of the list.

  Ah. Got it.

Thanks Anand

>>   	NULL
>>   };