[PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[Hengyu Liang]

Commit cf1afec09e9f ("btrfs: validate properties before setting them")
made btrfs_set_prop() validate a property before setting it. For the
compression property, the validation fails with -EINVAL when the inode
has NODATASUM.

However, a file created on a filesystem mounted with nodatasum has
NODATASUM and still inherits the compress flag from its directory, and
btrfs_fileattr_set() sets the compression property whenever the new
flags include FS_COMPR_FL. As of now, chattr fails on such a file, even
when it only changes another flag.

The issue can be reproduced with:

    mkfs.btrfs -f /dev/sdb
    mount -o nodatasum /dev/sdb /mnt
    mkdir /mnt/dir
    chattr +c /mnt/dir
    touch /mnt/dir/file
    chattr +A /mnt/dir/file

Before commit cf1afec09e9f ("btrfs: validate properties before setting
them"), the last command succeeds.

After that commit, it fails with EINVAL.

This patch will make btrfs_fileattr_set() set the compression property
only when the inode can be compressed.

Fixes: cf1afec09e9f ("btrfs: validate properties before setting them")
Cc: stable@vger.kernel.org # 7.2+
Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
---
 fs/btrfs/ioctl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 52aab510aea0..347509043f2e 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -421,7 +421,12 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
 	if (IS_ERR(trans))
 		return PTR_ERR(trans);
 
-	if (comp) {
+	/*
+	 * An inode with NODATASUM can not be compressed but can still have the
+	 * compress flag, inherited from its parent directory. Setting the
+	 * property fails validation for such an inode, so leave it unset.
+	 */
+	if (comp && btrfs_inode_can_compress(inode)) {
 		ret = btrfs_set_prop(trans, inode, "btrfs.compression",
 				     comp, strlen(comp), 0);
 		if (ret)
-- 
2.53.0

Re: [PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[Qu Wenruo]

在 2026/10/10 21:24, Hengyu Liang 写道:
> Commit cf1afec09e9f ("btrfs: validate properties before setting them")
> made btrfs_set_prop() validate a property before setting it. For the
> compression property, the validation fails with -EINVAL when the inode
> has NODATASUM.
> 
> However, a file created on a filesystem mounted with nodatasum has
> NODATASUM and still inherits the compress flag from its directory, and
> btrfs_fileattr_set() sets the compression property whenever the new
> flags include FS_COMPR_FL. As of now, chattr fails on such a file, even
> when it only changes another flag.
> 
> The issue can be reproduced with:
> 
>      mkfs.btrfs -f /dev/sdb
>      mount -o nodatasum /dev/sdb /mnt
>      mkdir /mnt/dir
>      chattr +c /mnt/dir
>      touch /mnt/dir/file
>      chattr +A /mnt/dir/file
> 
> Before commit cf1afec09e9f ("btrfs: validate properties before setting
> them"), the last command succeeds.
> 
> After that commit, it fails with EINVAL.

Explain the reason why it fails.

> 
> This patch will make btrfs_fileattr_set() set the compression property
> only when the inode can be compressed.
> 
> Fixes: cf1afec09e9f ("btrfs: validate properties before setting them")
> Cc: stable@vger.kernel.org # 7.2+
> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
> ---
>   fs/btrfs/ioctl.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 52aab510aea0..347509043f2e 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -421,7 +421,12 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>   	if (IS_ERR(trans))
>   		return PTR_ERR(trans);
>   
> -	if (comp) {
> +	/*
> +	 * An inode with NODATASUM can not be compressed but can still have the
> +	 * compress flag, inherited from its parent directory. Setting the
> +	 * property fails validation for such an inode, so leave it unset.
> +	 */

This is very LLM-like, and very LLM's objective-oriented behavior.
Disclose it first.

Secondly it just slightly do nothing, which is not the expected behavior.

Furthermore, this introduced another layer of exceptions.

For regular mount options, -o nodatasum,compress, the later compress 
will override the nodatasum.

So when setting compress in the first place, the NODATASUM/NODATACOW 
flag should be cleared.

> +	if (comp && btrfs_inode_can_compress(inode)) {
>   		ret = btrfs_set_prop(trans, inode, "btrfs.compression",
>   				     comp, strlen(comp), 0);
>   		if (ret)

Re: [PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[Qu Wenruo]

在 2026/10/11 07:47, Qu Wenruo 写道:
> 
> 
> 在 2026/10/10 21:24, Hengyu Liang 写道:
>> Commit cf1afec09e9f ("btrfs: validate properties before setting them")
>> made btrfs_set_prop() validate a property before setting it. For the
>> compression property, the validation fails with -EINVAL when the inode
>> has NODATASUM.
>>
>> However, a file created on a filesystem mounted with nodatasum has
>> NODATASUM and still inherits the compress flag from its directory, and
>> btrfs_fileattr_set() sets the compression property whenever the new
>> flags include FS_COMPR_FL. As of now, chattr fails on such a file, even
>> when it only changes another flag.
>>
>> The issue can be reproduced with:
>>
>>      mkfs.btrfs -f /dev/sdb
>>      mount -o nodatasum /dev/sdb /mnt
>>      mkdir /mnt/dir
>>      chattr +c /mnt/dir
>>      touch /mnt/dir/file
>>      chattr +A /mnt/dir/file
>>
>> Before commit cf1afec09e9f ("btrfs: validate properties before setting
>> them"), the last command succeeds.
>>
>> After that commit, it fails with EINVAL.
> 
> Explain the reason why it fails.
> 
>>
>> This patch will make btrfs_fileattr_set() set the compression property
>> only when the inode can be compressed.
>>
>> Fixes: cf1afec09e9f ("btrfs: validate properties before setting them")
>> Cc: stable@vger.kernel.org # 7.2+
>> Signed-off-by: Hengyu Liang <hengyul@cs.unc.edu>
>> ---
>>   fs/btrfs/ioctl.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 52aab510aea0..347509043f2e 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -421,7 +421,12 @@ int btrfs_fileattr_set(struct mnt_idmap *idmap,
>>       if (IS_ERR(trans))
>>           return PTR_ERR(trans);
>> -    if (comp) {
>> +    /*
>> +     * An inode with NODATASUM can not be compressed but can still 
>> have the
>> +     * compress flag, inherited from its parent directory. Setting the
>> +     * property fails validation for such an inode, so leave it unset.
>> +     */
> 
> This is very LLM-like, and very LLM's objective-oriented behavior.
> Disclose it first.
> 
> Secondly it just slightly do nothing, which is not the expected behavior.

s/slightly/silently/

> 
> Furthermore, this introduced another layer of exceptions.
> 
> For regular mount options, -o nodatasum,compress, the later compress 
> will override the nodatasum.
> 
> So when setting compress in the first place, the NODATASUM/NODATACOW 
> flag should be cleared.
> 
>> +    if (comp && btrfs_inode_can_compress(inode)) {
>>           ret = btrfs_set_prop(trans, inode, "btrfs.compression",
>>                        comp, strlen(comp), 0);
>>           if (ret)
> 

Re: [PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[Hengyu Liang]

On 2026/10/11 07:47, Qu Wenruo wrote:
> This is very LLM-like, and very LLM's objective-oriented behavior.
> Disclose it first.

Yes, and sorry for not saying so in the patch. I found this while
comparing the behaviour of different kernel versions. I used an LLM to
analyse the difference, write the patch and the changelog and run the
tests in VMs.

> Explain the reason why it fails.

chattr reads the flags and writes the whole set back, so FS_COMPR_FL is
in the new flags. btrfs_fileattr_set() then calls btrfs_set_prop() for
btrfs.compression, and since cf1afec09e9f that runs
prop_compression_validate(), which returns -EINVAL because
btrfs_inode_can_compress() is false for a NODATASUM inode.

> So when setting compress in the first place, the NODATASUM/NODATACOW
> flag should be cleared.

OK, I will do it this way: a new inode that inherits the compress flag
does not get NODATASUM/NODATACOW from the mount options, and chattr +c
on an empty file clears NODATASUM.

This differs from the documentation in one place. btrfs(5) says that
all files created under nodatasum have no checksums and that
compression is disabled when nodatasum is enabled. With this change a
file created in a +c directory on a nodatasum mount has checksums and
is compressed. Is that what you intend?

In chattr there are two cases where I would not clear the flag:

- The file has NODATASUM and already has data. The existing extents
  have no checksums, so clearing NODATASUM would make them unreadable.
  chattr +c would return -EINVAL here, the same as setting the
  btrfs.compression property does since commit 0e852ab8974c ("btrfs:
  do not allow compression on nodatacow files").

- The file has +C. check_fsflags() already rejects chattr +c on it
  with -EINVAL and I would keep that.

One more question before v2. Files with both flags and data can already
exist on disk, and chattr +A on them fails since 7.2. Is it OK to leave
the compress flag and the property alone on these files when only other
flags change?

Thanks,
Hengyu

Re: [PATCH] btrfs: fix chattr failing on nodatasum files that have the compress flag

[Qu Wenruo]

在 2026/10/11 14:17, Hengyu Liang 写道:
> On 2026/10/11 07:47, Qu Wenruo wrote:
>> This is very LLM-like, and very LLM's objective-oriented behavior.
>> Disclose it first.
> 
> Yes, and sorry for not saying so in the patch. I found this while
> comparing the behaviour of different kernel versions. I used an LLM to
> analyse the difference, write the patch and the changelog and run the
> tests in VMs.
> 
>> Explain the reason why it fails.
> 
> chattr reads the flags and writes the whole set back, so FS_COMPR_FL is
> in the new flags. btrfs_fileattr_set() then calls btrfs_set_prop() for
> btrfs.compression, and since cf1afec09e9f that runs
> prop_compression_validate(), which returns -EINVAL because
> btrfs_inode_can_compress() is false for a NODATASUM inode.
> 
>> So when setting compress in the first place, the NODATASUM/NODATACOW
>> flag should be cleared.
> 
> OK, I will do it this way: a new inode that inherits the compress flag
> does not get NODATASUM/NODATACOW from the mount options, and chattr +c
> on an empty file clears NODATASUM.
> 
> This differs from the documentation in one place. btrfs(5) says that
> all files created under nodatasum have no checksums and that
> compression is disabled when nodatasum is enabled.

Prop has higher priority than mount option, and defrag has even higher 
priority than prop.
That's the long existing behavior.

> With this change a
> file created in a +c directory on a nodatasum mount has checksums and
> is compressed. Is that what you intend?
> 
> In chattr there are two cases where I would not clear the flag:
> 
> - The file has NODATASUM and already has data. The existing extents
>    have no checksums, so clearing NODATASUM would make them unreadable.
>    chattr +c would return -EINVAL here, the same as setting the
>    btrfs.compression property does since commit 0e852ab8974c ("btrfs:
>    do not allow compression on nodatacow files").

That's expected. Non-empty files can not change their 
NODATASUM/NODATACOW flags.

Thus in that case, adding compression to a nodatasum/nodatacow inode 
should fail.

> 
> - The file has +C. check_fsflags() already rejects chattr +c on it
>    with -EINVAL and I would keep that.
> 
> One more question before v2. Files with both flags and data can already
> exist on disk,

In that case NODATASUM/NODATACOW overrides the COMPRESS inode flags, and 
no compressed extent will be generated.

That's a much safer default behavior.

If we go the other way, we will trigger a log problems like odd csum items.

Although setting COMPRESS flag on NODATASUM/NODATACOW is a much bigger 
change, it should be addressed in a dedicated series, with a lot of 
extra corner cases to consider.

> and chattr +A on them fails since 7.2. Is it OK to leave
> the compress flag and the property alone on these files when only other
> flags change?

Yes, that's why unlike other properties, compression has both 
FS_NOCOMP_FL and FS_COMPR_FL.

If neither FS_NOCOMP_FL nor FS_COMPR_FL is set, COMPRESS flag should not 
be touched.

Obviously the code just before update_flags: label is not properly 
following that behavior.

If your objective is only to not return error, then that's what you 
should touch, skipping the btrfs_set_prop() completely when neither 
FS_NOCOMP_FL nor FS_COMPR_FL is set.

> 
> Thanks,
> Hengyu