Re: [PATCH 6/8] nowait aio: ext4
On Tue 09-05-17 07:22:17, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues> > Return EAGAIN if any of the following checks fail for direct I/O: > + i_rwsem is lockable > + Writing beyond end of file (will trigger allocation) > + Blocks are not allocated at the write location > > Signed-off-by: Goldwyn Rodrigues The patch looks good. You can add: Reviewed-by: Jan Kara Honza > --- > fs/ext4/file.c | 20 > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c > index cefa9835f275..2efdc6d4d3e8 100644 > --- a/fs/ext4/file.c > +++ b/fs/ext4/file.c > @@ -216,7 +216,13 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter > *from) > return ext4_dax_write_iter(iocb, from); > #endif > > - inode_lock(inode); > + if (iocb->ki_flags & IOCB_NOWAIT) { > + if (!inode_trylock(inode)) > + return -EAGAIN; > + } else { > + inode_lock(inode); > + } > + > ret = ext4_write_checks(iocb, from); > if (ret <= 0) > goto out; > @@ -235,9 +241,15 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter > *from) > > iocb->private = > /* Check whether we do a DIO overwrite or not */ > - if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio && > - ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) > - overwrite = 1; > + if (o_direct && !unaligned_aio) { > + if (ext4_overwrite_io(inode, iocb->ki_pos, > iov_iter_count(from))) { > + if (ext4_should_dioread_nolock(inode)) > + overwrite = 1; > + } else if (iocb->ki_flags & IOCB_NOWAIT) { > + ret = -EAGAIN; > + goto out; > + } > + } > > ret = __generic_file_write_iter(iocb, from); > inode_unlock(inode); > -- > 2.12.0 > -- Jan Kara SUSE Labs, CR
[PATCH 6/8] nowait aio: ext4
From: Goldwyn RodriguesReturn EAGAIN if any of the following checks fail for direct I/O: + i_rwsem is lockable + Writing beyond end of file (will trigger allocation) + Blocks are not allocated at the write location Signed-off-by: Goldwyn Rodrigues --- fs/ext4/file.c | 20 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index cefa9835f275..2efdc6d4d3e8 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -216,7 +216,13 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) return ext4_dax_write_iter(iocb, from); #endif - inode_lock(inode); + if (iocb->ki_flags & IOCB_NOWAIT) { + if (!inode_trylock(inode)) + return -EAGAIN; + } else { + inode_lock(inode); + } + ret = ext4_write_checks(iocb, from); if (ret <= 0) goto out; @@ -235,9 +241,15 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) iocb->private = /* Check whether we do a DIO overwrite or not */ - if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio && - ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) - overwrite = 1; + if (o_direct && !unaligned_aio) { + if (ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) { + if (ext4_should_dioread_nolock(inode)) + overwrite = 1; + } else if (iocb->ki_flags & IOCB_NOWAIT) { + ret = -EAGAIN; + goto out; + } + } ret = __generic_file_write_iter(iocb, from); inode_unlock(inode); -- 2.12.0
Re: [PATCH 6/8] nowait aio: ext4
On Mon 10-04-17 07:39:43, Christoph Hellwig wrote: > On Mon, Apr 10, 2017 at 02:37:50PM +0200, Jan Kara wrote: > > I don't understand here. Do you want that all filesystems support NOWAIT > > direct IO? > > No. Per-file_system_type is way to coarse grained. All feature flag > needs to be per-file_operation at least for cases like ext4 with our > without extents (or journal) XFS v4 vs v5, different NFS versions, etc. Ah, I see your point now. Thanks for patience. I think we could make this work by making generic_file_write/read_iter() refuse NOWAIT IO with EOPNOTSUPP and then only modify those few filesystems that implement their own iter helpers and will not initially support NOWAIT IO. Sounds easy enough. Honza -- Jan KaraSUSE Labs, CR
Re: [PATCH 6/8] nowait aio: ext4
On Mon, Apr 10, 2017 at 02:37:50PM +0200, Jan Kara wrote: > I don't understand here. Do you want that all filesystems support NOWAIT > direct IO? No. Per-file_system_type is way to coarse grained. All feature flag needs to be per-file_operation at least for cases like ext4 with our without extents (or journal) XFS v4 vs v5, different NFS versions, etc. For RWF_* each file operation simply declares if the feature is supported not by rejecting unknown ones. FIEMAP does the same as do a few other interfaces.
Re: [PATCH 6/8] nowait aio: ext4
On Tue, Apr 04, 2017 at 01:41:09PM -0500, Goldwyn Rodrigues wrote: > I am working on incorporating RWF_* flags. However, I am not sure how > RWF_* flags would get rid of FS_NOWAIT/FS_NOWAIT_IO. Since most of > "blocking" information is with the filesystem, it is a per-filesystem > flag to block out (EOPNOTSUPP) the filesystems which do not support it. You need to check the flag in the actual read/write methods as the support for features on Linux is not a per-file_system_type thing.
Re: [PATCH 6/8] nowait aio: ext4
On 04/04/2017 03:41 AM, Christoph Hellwig wrote: > On Tue, Apr 04, 2017 at 09:58:53AM +0200, Jan Kara wrote: >> FS_NOWAIT looks a bit too generic given these are filesystem feature flags. >> Can we call it FS_NOWAIT_IO? > > It's way to generic as it's a feature of the particular file_operations > instance. But once we switch to using RWF_* we can just the existing > per-op feature checks for thos and the per-fs flag should just go away. > I am working on incorporating RWF_* flags. However, I am not sure how RWF_* flags would get rid of FS_NOWAIT/FS_NOWAIT_IO. Since most of "blocking" information is with the filesystem, it is a per-filesystem flag to block out (EOPNOTSUPP) the filesystems which do not support it. -- Goldwyn
Re: [PATCH 6/8] nowait aio: ext4
On Tue, Apr 04, 2017 at 09:58:53AM +0200, Jan Kara wrote: > FS_NOWAIT looks a bit too generic given these are filesystem feature flags. > Can we call it FS_NOWAIT_IO? It's way to generic as it's a feature of the particular file_operations instance. But once we switch to using RWF_* we can just the existing per-op feature checks for thos and the per-fs flag should just go away.
Re: [PATCH 6/8] nowait aio: ext4
On Mon 03-04-17 13:53:05, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues> > Return EAGAIN if any of the following checks fail for direct I/O: > + i_rwsem is lockable > + Writing beyond end of file (will trigger allocation) > + Blocks are not allocated at the write location Patches seem to be missing your Signed-off-by tag... > @@ -235,9 +237,21 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter > *from) > > iocb->private = > /* Check whether we do a DIO overwrite or not */ > - if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio && > - ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) > - overwrite = 1; > + if (o_direct && !unaligned_aio) { > + struct ext4_map_blocks map; > + if (ext4_blocks_mapped(inode, iocb->ki_pos, > + iov_iter_count(from), )) { > + /* To exclude unwritten extents, we need to check > + * m_flags. > + */ > + if (ext4_should_dioread_nolock(inode) && > + (map.m_flags & EXT4_MAP_MAPPED)) > + overwrite = 1; > + } else if (iocb->ki_flags & IOCB_NOWAIT) { > + ret = -EAGAIN; > + goto out; > + } > + } Actually, overwriting unwritten extents is relatively complex in ext4 as well. In particular we need to start a transaction and split out the written part of the extent. So I don't think we can easily support this without blocking. As a result I'd keep the condition for IOCB_NOWAIT the same as for overwrite IO. > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -117,7 +117,7 @@ static struct file_system_type ext2_fs_type = { > .name = "ext2", > .mount = ext4_mount, > .kill_sb= kill_block_super, > - .fs_flags = FS_REQUIRES_DEV, > + .fs_flags = FS_REQUIRES_DEV | FS_NOWAIT, FS_NOWAIT looks a bit too generic given these are filesystem feature flags. Can we call it FS_NOWAIT_IO? Honza -- Jan Kara SUSE Labs, CR
[PATCH 6/8] nowait aio: ext4
From: Goldwyn RodriguesReturn EAGAIN if any of the following checks fail for direct I/O: + i_rwsem is lockable + Writing beyond end of file (will trigger allocation) + Blocks are not allocated at the write location --- fs/ext4/file.c | 48 +++- fs/ext4/super.c | 6 +++--- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 8210c1f..e223b9f 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -127,27 +127,22 @@ ext4_unaligned_aio(struct inode *inode, struct iov_iter *from, loff_t pos) return 0; } -/* Is IO overwriting allocated and initialized blocks? */ -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len) +/* Are IO blocks allocated */ +static bool ext4_blocks_mapped(struct inode *inode, loff_t pos, loff_t len, + struct ext4_map_blocks *map) { - struct ext4_map_blocks map; unsigned int blkbits = inode->i_blkbits; int err, blklen; if (pos + len > i_size_read(inode)) return false; - map.m_lblk = pos >> blkbits; - map.m_len = EXT4_MAX_BLOCKS(len, pos, blkbits); - blklen = map.m_len; + map->m_lblk = pos >> blkbits; + map->m_len = EXT4_MAX_BLOCKS(len, pos, blkbits); + blklen = map->m_len; - err = ext4_map_blocks(NULL, inode, , 0); - /* -* 'err==len' means that all of the blocks have been preallocated, -* regardless of whether they have been initialized or not. To exclude -* unwritten extents, we need to check m_flags. -*/ - return err == blklen && (map.m_flags & EXT4_MAP_MAPPED); + err = ext4_map_blocks(NULL, inode, map, 0); + return err == blklen; } static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) @@ -204,6 +199,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct inode *inode = file_inode(iocb->ki_filp); int o_direct = iocb->ki_flags & IOCB_DIRECT; + int nowait = iocb->ki_flags & IOCB_NOWAIT; int unaligned_aio = 0; int overwrite = 0; ssize_t ret; @@ -216,7 +212,13 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) return ext4_dax_write_iter(iocb, from); #endif - inode_lock(inode); + if (o_direct && nowait) { + if (!inode_trylock(inode)) + return -EAGAIN; + } else { + inode_lock(inode); + } + ret = ext4_write_checks(iocb, from); if (ret <= 0) goto out; @@ -235,9 +237,21 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) iocb->private = /* Check whether we do a DIO overwrite or not */ - if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio && - ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) - overwrite = 1; + if (o_direct && !unaligned_aio) { + struct ext4_map_blocks map; + if (ext4_blocks_mapped(inode, iocb->ki_pos, + iov_iter_count(from), )) { + /* To exclude unwritten extents, we need to check +* m_flags. +*/ + if (ext4_should_dioread_nolock(inode) && + (map.m_flags & EXT4_MAP_MAPPED)) + overwrite = 1; + } else if (iocb->ki_flags & IOCB_NOWAIT) { + ret = -EAGAIN; + goto out; + } + } ret = __generic_file_write_iter(iocb, from); inode_unlock(inode); diff --git a/fs/ext4/super.c b/fs/ext4/super.c index a9448db..e1d424a 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -117,7 +117,7 @@ static struct file_system_type ext2_fs_type = { .name = "ext2", .mount = ext4_mount, .kill_sb= kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .fs_flags = FS_REQUIRES_DEV | FS_NOWAIT, }; MODULE_ALIAS_FS("ext2"); MODULE_ALIAS("ext2"); @@ -132,7 +132,7 @@ static struct file_system_type ext3_fs_type = { .name = "ext3", .mount = ext4_mount, .kill_sb= kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .fs_flags = FS_REQUIRES_DEV | FS_NOWAIT, }; MODULE_ALIAS_FS("ext3"); MODULE_ALIAS("ext3"); @@ -5623,7 +5623,7 @@ static struct file_system_type ext4_fs_type = { .name = "ext4", .mount = ext4_mount, .kill_sb= kill_block_super, - .fs_flags = FS_REQUIRES_DEV, + .fs_flags = FS_REQUIRES_DEV | FS_NOWAIT, }; MODULE_ALIAS_FS("ext4"); -- 2.10.2
[PATCH 6/8] nowait aio: ext4
From: Goldwyn RodriguesReturn EAGAIN if any of the following checks fail for direct I/O: + i_rwsem is lockable + Writing beyond end of file (will trigger allocation) + Blocks are not allocated at the write location Signed-off-by: Goldwyn Rodrigues --- fs/ext4/file.c | 48 +++- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 8210c1f..e223b9f 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -127,27 +127,22 @@ ext4_unaligned_aio(struct inode *inode, struct iov_iter *from, loff_t pos) return 0; } -/* Is IO overwriting allocated and initialized blocks? */ -static bool ext4_overwrite_io(struct inode *inode, loff_t pos, loff_t len) +/* Are IO blocks allocated */ +static bool ext4_blocks_mapped(struct inode *inode, loff_t pos, loff_t len, + struct ext4_map_blocks *map) { - struct ext4_map_blocks map; unsigned int blkbits = inode->i_blkbits; int err, blklen; if (pos + len > i_size_read(inode)) return false; - map.m_lblk = pos >> blkbits; - map.m_len = EXT4_MAX_BLOCKS(len, pos, blkbits); - blklen = map.m_len; + map->m_lblk = pos >> blkbits; + map->m_len = EXT4_MAX_BLOCKS(len, pos, blkbits); + blklen = map->m_len; - err = ext4_map_blocks(NULL, inode, , 0); - /* -* 'err==len' means that all of the blocks have been preallocated, -* regardless of whether they have been initialized or not. To exclude -* unwritten extents, we need to check m_flags. -*/ - return err == blklen && (map.m_flags & EXT4_MAP_MAPPED); + err = ext4_map_blocks(NULL, inode, map, 0); + return err == blklen; } static ssize_t ext4_write_checks(struct kiocb *iocb, struct iov_iter *from) @@ -204,6 +199,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) { struct inode *inode = file_inode(iocb->ki_filp); int o_direct = iocb->ki_flags & IOCB_DIRECT; + int nowait = iocb->ki_flags & IOCB_NOWAIT; int unaligned_aio = 0; int overwrite = 0; ssize_t ret; @@ -216,7 +212,13 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) return ext4_dax_write_iter(iocb, from); #endif - inode_lock(inode); + if (o_direct && nowait) { + if (!inode_trylock(inode)) + return -EAGAIN; + } else { + inode_lock(inode); + } + ret = ext4_write_checks(iocb, from); if (ret <= 0) goto out; @@ -235,9 +237,21 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from) iocb->private = /* Check whether we do a DIO overwrite or not */ - if (o_direct && ext4_should_dioread_nolock(inode) && !unaligned_aio && - ext4_overwrite_io(inode, iocb->ki_pos, iov_iter_count(from))) - overwrite = 1; + if (o_direct && !unaligned_aio) { + struct ext4_map_blocks map; + if (ext4_blocks_mapped(inode, iocb->ki_pos, + iov_iter_count(from), )) { + /* To exclude unwritten extents, we need to check +* m_flags. +*/ + if (ext4_should_dioread_nolock(inode) && + (map.m_flags & EXT4_MAP_MAPPED)) + overwrite = 1; + } else if (iocb->ki_flags & IOCB_NOWAIT) { + ret = -EAGAIN; + goto out; + } + } ret = __generic_file_write_iter(iocb, from); inode_unlock(inode); -- 2.10.2