Re: [PATCH] fs/ntfs3: Update inode->i_mapping->a_ops on compression state change

[Dave Chinner <david@fromorbit.com>]

On Thu, Jan 23, 2025 at 04:53:35PM +0300, Konstantin Komarov wrote:
> Update inode->i_mapping->a_ops when the compression state changes to
> ensure correct address space operations.
> Clear ATTR_FLAG_SPARSED/FILE_ATTRIBUTE_SPARSE_FILE when enabling
> compression to prevent flag conflicts.
> 
> Fixes: 6b39bfaeec44 ("fs/ntfs3: Add support for the compression attribute")
> Reported-by: Kun Hu <huk23@m.fudan.edu.cn>, Jiaji Qin <jjtan24@m.fudan.edu.cn>
> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
> ---
>  fs/ntfs3/attrib.c  | 1 +
>  fs/ntfs3/file.c    | 2 ++
>  fs/ntfs3/frecord.c | 6 ++++--
>  3 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
> index af94e3737470..d2410ab6c7bf 100644
> --- a/fs/ntfs3/attrib.c
> +++ b/fs/ntfs3/attrib.c
> @@ -2666,6 +2666,7 @@ int attr_set_compress(struct ntfs_inode *ni, bool compr)
>  
>  	/* Update data attribute flags. */
>  	if (compr) {
> +		attr->flags &= ATTR_FLAG_SPARSED;
>  		attr->flags |= ATTR_FLAG_COMPRESSED;
>  		attr->nres.c_unit = NTFS_LZNT_CUNIT;
>  	} else {
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 3f96a11804c9..e8f452f47cd5 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -105,6 +105,8 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
>  		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
>  		if (err)
>  			return err;
> +		inode->i_mapping->a_ops =
> +			(flags & FS_COMPR_FL) ? &ntfs_aops_cmpr : &ntfs_aops;

It is not safe to change a_ops dynamically like this.

There can be other operations running concurrently using the
existing aops, and some functionality (e.g. the page fault paths)
have dependencies between aops methods and swapping the aops between
those operations can cause all sorts of problem (including crashing
the kernel).

This is the reason why we cannot turn functionality like FSDAX on
and off via fileattr_set() methods setting/clearing inode flags.
The inode has to transition out of cache and be re-instantiated to
change the inode aops methods safely...

-Dave.
-- 
Dave Chinner
david@fromorbit.com