fs/ntfs3: Update inode->i_mapping->a_ops on compression state change

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

Posted by Konstantin Komarov 1 year ago

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;
 	}
 
 	inode_set_flags(inode, new_fl, S_IMMUTABLE | S_APPEND);
diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 5df6a0b5add9..81271196c557 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -3434,10 +3434,12 @@ int ni_set_compress(struct inode *inode, bool compr)
 	}
 
 	ni->std_fa = std->fa;
-	if (compr)
+	if (compr) {
+		std->fa &= ~FILE_ATTRIBUTE_SPARSE_FILE;
 		std->fa |= FILE_ATTRIBUTE_COMPRESSED;
-	else
+	} else {
 		std->fa &= ~FILE_ATTRIBUTE_COMPRESSED;
+	}
 
 	if (ni->std_fa != std->fa) {
 		ni->std_fa = std->fa;
-- 
2.34.1

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

Posted by Christoph Hellwig 1 year ago

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.

How is this synchronized against using the address_space ops?
Most of them can be called without i_rwsem held.

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

Posted by Dave Chinner 1 year ago

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

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

Posted by Konstantin Komarov 1 year ago

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.

v2:
Additionally, ensure that all dirty pages are flushed and concurrent access
to the page cache is blocked.

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  |  3 ++-
 fs/ntfs3/file.c    | 22 ++++++++++++++++++++--
 fs/ntfs3/frecord.c |  6 ++++--
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
index af94e3737470..e946f75eb540 100644
--- a/fs/ntfs3/attrib.c
+++ b/fs/ntfs3/attrib.c
@@ -2664,8 +2664,9 @@ int attr_set_compress(struct ntfs_inode *ni, bool compr)
 		attr->nres.run_off = cpu_to_le16(run_off);
 	}
 
-	/* Update data attribute flags. */
+	/* Update 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 4d9d84cc3c6f..9b6a3f8d2e7c 100644
--- a/fs/ntfs3/file.c
+++ b/fs/ntfs3/file.c
@@ -101,8 +101,26 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
 	/* Allowed to change compression for empty files and for directories only. */
 	if (!is_dedup(ni) && !is_encrypted(ni) &&
 	    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
-		/* Change compress state. */
-		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
+		int err = 0;
+		struct address_space *mapping = inode->i_mapping;
+
+		/* write out all data and wait. */
+		filemap_invalidate_lock(mapping);
+		err = filemap_write_and_wait(mapping);
+
+		if (err >= 0) {
+			/* Change compress state. */
+			bool compr = flags & FS_COMPR_FL;
+			err = ni_set_compress(inode, compr);
+
+			/* For files change a_ops too. */
+			if (!err)
+				mapping->a_ops = compr ? &ntfs_aops_cmpr :
+							 &ntfs_aops;
+		}
+
+		filemap_invalidate_unlock(mapping);
+
 		if (err)
 			return err;
 	}
diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
index 5df6a0b5add9..81271196c557 100644
--- a/fs/ntfs3/frecord.c
+++ b/fs/ntfs3/frecord.c
@@ -3434,10 +3434,12 @@ int ni_set_compress(struct inode *inode, bool compr)
 	}
 
 	ni->std_fa = std->fa;
-	if (compr)
+	if (compr) {
+		std->fa &= ~FILE_ATTRIBUTE_SPARSE_FILE;
 		std->fa |= FILE_ATTRIBUTE_COMPRESSED;
-	else
+	} else {
 		std->fa &= ~FILE_ATTRIBUTE_COMPRESSED;
+	}
 
 	if (ni->std_fa != std->fa) {
 		ni->std_fa = std->fa;
-- 
2.34.1

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

Posted by Dave Chinner 11 months, 3 weeks ago

On Fri, Jan 31, 2025 at 04:18:31PM +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.
> 
> v2:
> Additionally, ensure that all dirty pages are flushed and concurrent access
> to the page cache is blocked.
> 
> 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  |  3 ++-
>  fs/ntfs3/file.c    | 22 ++++++++++++++++++++--
>  fs/ntfs3/frecord.c |  6 ++++--
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
> index af94e3737470..e946f75eb540 100644
> --- a/fs/ntfs3/attrib.c
> +++ b/fs/ntfs3/attrib.c
> @@ -2664,8 +2664,9 @@ int attr_set_compress(struct ntfs_inode *ni, bool compr)
>  		attr->nres.run_off = cpu_to_le16(run_off);
>  	}
>  
> -	/* Update data attribute flags. */
> +	/* Update 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 4d9d84cc3c6f..9b6a3f8d2e7c 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -101,8 +101,26 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
>  	/* Allowed to change compression for empty files and for directories only. */
>  	if (!is_dedup(ni) && !is_encrypted(ni) &&
>  	    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> -		/* Change compress state. */
> -		int err = ni_set_compress(inode, flags & FS_COMPR_FL);
> +		int err = 0;
> +		struct address_space *mapping = inode->i_mapping;
> +
> +		/* write out all data and wait. */
> +		filemap_invalidate_lock(mapping);
> +		err = filemap_write_and_wait(mapping);
> +
> +		if (err >= 0) {
> +			/* Change compress state. */
> +			bool compr = flags & FS_COMPR_FL;
> +			err = ni_set_compress(inode, compr);
> +
> +			/* For files change a_ops too. */
> +			if (!err)
> +				mapping->a_ops = compr ? &ntfs_aops_cmpr :
> +							 &ntfs_aops;
> +		}
> +
> +		filemap_invalidate_unlock(mapping);

Holding the invalidate lock doesn't make it safe to change aops
methods. We've been down this road before - find some other way to
switch modes internally to the ntfs filesystem, because changing
aops pointers like this is *not safe* and not maintainable.

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

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

Posted by Kun Hu 11 months, 3 weeks ago


> 2025年1月31日 21:18，Konstantin Komarov <almaz.alexandrovich@paragon-software.com> 写道：
> 
> 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.
> 
> v2:
> Additionally, ensure that all dirty pages are flushed and concurrent access
> to the page cache is blocked.
> 
> 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  |  3 ++-
> fs/ntfs3/file.c    | 22 ++++++++++++++++++++--
> fs/ntfs3/frecord.c |  6 ++++--
> 3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs3/attrib.c b/fs/ntfs3/attrib.c
> index af94e3737470..e946f75eb540 100644
> --- a/fs/ntfs3/attrib.c
> +++ b/fs/ntfs3/attrib.c
> @@ -2664,8 +2664,9 @@ int attr_set_compress(struct ntfs_inode *ni, bool compr)
> attr->nres.run_off = cpu_to_le16(run_off);
> }
> 
> - /* Update data attribute flags. */
> + /* Update 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 4d9d84cc3c6f..9b6a3f8d2e7c 100644
> --- a/fs/ntfs3/file.c
> +++ b/fs/ntfs3/file.c
> @@ -101,8 +101,26 @@ int ntfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> /* Allowed to change compression for empty files and for directories only. */
> if (!is_dedup(ni) && !is_encrypted(ni) &&
>    (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> - /* Change compress state. */
> - int err = ni_set_compress(inode, flags & FS_COMPR_FL);
> + int err = 0;
> + struct address_space *mapping = inode->i_mapping;
> +
> + /* write out all data and wait. */
> + filemap_invalidate_lock(mapping);
> + err = filemap_write_and_wait(mapping);
> +
> + if (err >= 0) {
> + /* Change compress state. */
> + bool compr = flags & FS_COMPR_FL;
> + err = ni_set_compress(inode, compr);
> +
> + /* For files change a_ops too. */
> + if (!err)
> + mapping->a_ops = compr ? &ntfs_aops_cmpr :
> + &ntfs_aops;
> + }
> +
> + filemap_invalidate_unlock(mapping);
> +
> if (err)
> return err;
> }
> diff --git a/fs/ntfs3/frecord.c b/fs/ntfs3/frecord.c
> index 5df6a0b5add9..81271196c557 100644
> --- a/fs/ntfs3/frecord.c
> +++ b/fs/ntfs3/frecord.c
> @@ -3434,10 +3434,12 @@ int ni_set_compress(struct inode *inode, bool compr)
> }
> 
> ni->std_fa = std->fa;
> - if (compr)
> + if (compr) {
> + std->fa &= ~FILE_ATTRIBUTE_SPARSE_FILE;
> std->fa |= FILE_ATTRIBUTE_COMPRESSED;
> - else
> + } else {
> std->fa &= ~FILE_ATTRIBUTE_COMPRESSED;
> + }
> 
> if (ni->std_fa != std->fa) {
> ni->std_fa = std->fa;
> -- 
> 2.34.1
> 
> 

Hi Konstantin,

I wanted to follow up as I haven’t yet seen the fix you provided, titled “[v2] fs/ntfs3: Update inode->i_mapping->a_ops on compression state” in the kernel tree. Could you kindly confirm if this resolves the issue we’ve been discussing? Additionally, I would greatly appreciate it if you could share any updates regarding the resolution of this matter.

———
Thanks,
Kun Hu

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

Posted by Kun Hu 12 months ago

> 
> v2:
> Additionally, ensure that all dirty pages are flushed and concurrent access
> to the page cache is blocked.
> 

Hi Konstantin,

I wanted to follow up as I haven’t yet seen the fix you provided, titled “[v2] fs/ntfs3: Update inode->i_mapping->a_ops on compression state” in the kernel tree. Could you kindly confirm if this resolves the issue we’ve been discussing? Additionally, I would greatly appreciate it if you could share any updates regarding the resolution of this matter.

———
Thanks,
Kun Hu