| Message ID | 20230612053731.585947-1-hch@lst.de |
|---|---|
| State | New |
| Headers | show |
| Series | ext4: set FMODE_CAN_ODIRECT instead of a dummy direct_IO method | expand |
Christoph Hellwig <hch@lst.de> writes: > Since commit a2ad63daa88b ("VFS: add FMODE_CAN_ODIRECT file flag") file > systems can just set the FMODE_CAN_ODIRECT flag at open time instead of > wiring up a dummy direct_IO method to indicate support for direct I/O. > > Do that for ext4 so that noop_direct_IO can eventually be removed. Looks straight forward change to me. However I got some query... > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 02de439bf1f04e..b9c1cfa1864779 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -3573,7 +3571,6 @@ static const struct address_space_operations ext4_da_aops = { > .bmap = ext4_bmap, > .invalidate_folio = ext4_invalidate_folio, > .release_folio = ext4_release_folio, > - .direct_IO = noop_direct_IO, > .migrate_folio = buffer_migrate_folio, > .is_partially_uptodate = block_is_partially_uptodate, > .error_remove_page = generic_error_remove_page, > @@ -3582,7 +3579,6 @@ static const struct address_space_operations ext4_da_aops = { > > static const struct address_space_operations ext4_dax_aops = { > .writepages = ext4_dax_writepages, > - .direct_IO = noop_direct_IO, > .dirty_folio = noop_dirty_folio, > .bmap = ext4_bmap, > .swap_activate = ext4_iomap_swap_activate, why do we require .direct_IO function op for any of the dax_aops? IIUC, any inode if it supports DAX i.e. IS_DAX(inode), then it takes the separate path in file read/write iter path. so it should never do ->direct_IO on an inode which supports DAX right? Maybe I am missing something and I will have to check when does ->direct_IO gets called for DAX... ... Or is it due to CONFIG_FS_DAX? e.g. ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct inode *inode = file_inode(iocb->ki_filp); if (unlikely(ext4_forced_shutdown(EXT4_SB(inode->i_sb)))) return -EIO; #ifdef CONFIG_FS_DAX if (IS_DAX(inode)) return ext4_dax_write_iter(iocb, from); #endif if (iocb->ki_flags & IOCB_DIRECT) return ext4_dio_write_iter(iocb, from); else return ext4_buffered_write_iter(iocb, from); } -ritesh
On Tue, Jun 13, 2023 at 11:00:19AM +0530, Ritesh Harjani wrote: > why do we require .direct_IO function op for any of the dax_aops? > IIUC, any inode if it supports DAX i.e. IS_DAX(inode), then it takes the > separate path in file read/write iter path. > > so it should never do ->direct_IO on an inode which supports DAX right? do_dentry_open rejects opens with O_DIRECT if FMODE_CAN_ODIRECT is not set. So we either needs to set that manually or because there is a ->direct_IO if we want to keep supporting O_DIRECT opens for DAX files, which we've traditionally supported.
Christoph Hellwig <hch@lst.de> writes: > On Tue, Jun 13, 2023 at 11:00:19AM +0530, Ritesh Harjani wrote: >> why do we require .direct_IO function op for any of the dax_aops? >> IIUC, any inode if it supports DAX i.e. IS_DAX(inode), then it takes the >> separate path in file read/write iter path. >> >> so it should never do ->direct_IO on an inode which supports DAX right? > > do_dentry_open rejects opens with O_DIRECT if FMODE_CAN_ODIRECT is > not set. So we either needs to set that manually or because there is a > ->direct_IO if we want to keep supporting O_DIRECT opens for DAX > files, which we've traditionally supported. Ok, so for DAX it was mainly to support file opens with O_DIRECT semantics. Thanks -ritesh
diff --git a/fs/ext4/file.c b/fs/ext4/file.c index d101b3b0c7dad8..a1d8ffbf571274 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -900,7 +900,7 @@ static int ext4_file_open(struct inode *inode, struct file *filp) } filp->f_mode |= FMODE_NOWAIT | FMODE_BUF_RASYNC | - FMODE_DIO_PARALLEL_WRITE; + FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT; return dquot_file_open(inode, filp); } diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index 02de439bf1f04e..b9c1cfa1864779 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3539,7 +3539,6 @@ static const struct address_space_operations ext4_aops = { .bmap = ext4_bmap, .invalidate_folio = ext4_invalidate_folio, .release_folio = ext4_release_folio, - .direct_IO = noop_direct_IO, .migrate_folio = buffer_migrate_folio, .is_partially_uptodate = block_is_partially_uptodate, .error_remove_page = generic_error_remove_page, @@ -3556,7 +3555,6 @@ static const struct address_space_operations ext4_journalled_aops = { .bmap = ext4_bmap, .invalidate_folio = ext4_journalled_invalidate_folio, .release_folio = ext4_release_folio, - .direct_IO = noop_direct_IO, .migrate_folio = buffer_migrate_folio_norefs, .is_partially_uptodate = block_is_partially_uptodate, .error_remove_page = generic_error_remove_page, @@ -3573,7 +3571,6 @@ static const struct address_space_operations ext4_da_aops = { .bmap = ext4_bmap, .invalidate_folio = ext4_invalidate_folio, .release_folio = ext4_release_folio, - .direct_IO = noop_direct_IO, .migrate_folio = buffer_migrate_folio, .is_partially_uptodate = block_is_partially_uptodate, .error_remove_page = generic_error_remove_page, @@ -3582,7 +3579,6 @@ static const struct address_space_operations ext4_da_aops = { static const struct address_space_operations ext4_dax_aops = { .writepages = ext4_dax_writepages, - .direct_IO = noop_direct_IO, .dirty_folio = noop_dirty_folio, .bmap = ext4_bmap, .swap_activate = ext4_iomap_swap_activate,
Since commit a2ad63daa88b ("VFS: add FMODE_CAN_ODIRECT file flag") file systems can just set the FMODE_CAN_ODIRECT flag at open time instead of wiring up a dummy direct_IO method to indicate support for direct I/O. Do that for ext4 so that noop_direct_IO can eventually be removed. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/ext4/file.c | 2 +- fs/ext4/inode.c | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-)