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

[Qu Wenruo <wqu@suse.com>]

在 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