| Message ID | 20250411012428.3473333-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/ntfs3: Add missing direct_IO in ntfs_aops_cmpr | expand |
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?
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. [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
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
diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c index 3e2957a1e360..50524f573d3a 100644 --- a/fs/ntfs3/inode.c +++ b/fs/ntfs3/inode.c @@ -2068,5 +2068,6 @@ const struct address_space_operations ntfs_aops_cmpr = { .read_folio = ntfs_read_folio, .readahead = ntfs_readahead, .dirty_folio = block_dirty_folio, + .direct_IO = ntfs_direct_IO, }; // clang-format on
The ntfs3 can use the page cache directly, so its address_space_operations need direct_IO. Fixes: b432163ebd15 ("fs/ntfs3: Update inode->i_mapping->a_ops on compression state") Reported-by: syzbot+e36cc3297bd3afd25e19@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=e36cc3297bd3afd25e19 Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> --- fs/ntfs3/inode.c | 1 + 1 file changed, 1 insertion(+)