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
