Re: [PATCH] fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr

[Jan Kara]

On Tue 15-04-25 09:05:18, Lizhi Xu wrote:
> On Sun, 13 Apr 2025 22:50:54 -0700, Christoph Hellwig wrote:
> > On Fri, Apr 11, 2025 at 09:24:27AM +0800, Lizhi Xu wrote:
> > > The ntfs3 can use the page cache directly, so its address_space_operations
> > > need direct_IO.
> > 
> > I can't parse that sentence.  What are you trying to say with it?
> The comments [1] of generic_file_read_iter() clearly states "read_iter()
> for all filesystems that can use the page cache directly".
> 
> In the calltrace of this example, it is clear that direct_IO is not set.
> In [3], it is also clear that the lack of direct_IO in ntfs_aops_cmpr
> caused this problem.
> 
> In summary, direct_IO must be set in this issue.

I agree that you need to set .direct_IO in ntfs_aops_cmpr but since
compressed files do not *support* direct IO (at least I don't see any such
support in ntfs_direct_IO()) you either need to also handle these files in
ntfs_direct_IO() or you need to set special direct IO handler that will
just return 0 and thus fall back to buffered IO. So I don't think your
patch is correct as is.

									Honza

> [1]
>  * generic_file_read_iter - generic filesystem read routine
>  * @iocb:	kernel I/O control block
>  * @iter:	destination for the data read
>  *
>  * This is the "read_iter()" routine for all filesystems
>  * that can use the page cache directly.
> 
> [2]
> generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> 	size_t count = iov_iter_count(iter);
> 	ssize_t retval = 0;
> 
> 	if (!count)
> 		return 0; /* skip atime */
> 
> 	if (iocb->ki_flags & IOCB_DIRECT) {
> 		struct file *file = iocb->ki_filp;
> 		struct address_space *mapping = file->f_mapping;
> 		struct inode *inode = mapping->host;
> 
> 		retval = kiocb_write_and_wait(iocb, count);
> 		if (retval < 0)
> 			return retval;
> 		file_accessed(file);
> 
> 		retval = mapping->a_ops->direct_IO(iocb, iter); 
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b432163ebd15a0fb74051949cb61456d6c55ccbd
> diff --git a/fs/ntfs3/file.c b/fs/ntfs3/file.c
> index 4d9d84cc3c6f55..9b6a3f8d2e7c5c 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;
> 
> BR,
> Lizhi
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR