* [PATCH 0/2 v2] iomap: Waiting for IO in iomap_dio_rw() @ 2019-10-11 14:14 Jan Kara 2019-10-11 14:14 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara 2019-10-11 14:14 ` [PATCH 2/2] xfs: Use iomap_dio_rw_wait() Jan Kara 0 siblings, 2 replies; 11+ messages in thread From: Jan Kara @ 2019-10-11 14:14 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, linux-xfs, darrick.wong, Dave Chinner, Matthew Bobrowski, Jan Kara Hello, here is new version of the series with small changes people suggested: Changes since v1: * The new function argument of iomap_dio_rw() does not get overridden by is_sync_kiocb() the caller is responsible for this. --- Original motivation: when doing the ext4 conversion of direct IO code to iomap, we found it very difficult to handle inode extension with what iomap code currently provides. Ext4 wants to do inode extension as sync IO (so that the whole duration of IO is protected by inode->i_rwsem), also we need to truncate blocks beyond end of file in case of error or short write. Now in ->end_io handler we don't have the information how long originally the write was (to judge whether we may have allocated more blocks than we actually used) and in ->write_iter we don't know whether / how much of the IO actually succeeded in case of AIO. Thinking about it for some time I think iomap code makes it unnecessarily complex for the filesystem in case it decides it doesn't want to perform AIO and wants to fall back to good old synchronous IO. In such case it is much easier for the filesystem if it just gets normal error return from iomap_dio_rw() and not just -EIOCBQUEUED. The first patch in the series adds argument to iomap_dio_rw() to wait for IO completion (internally iomap_dio_rw() already supports this!) and the second patch converts XFS waiting for unaligned DIO write to this new API. What do people think? Honza Previous versions: Link: http://lore.kernel.org/r/20191009202736.19227-1-jack@suse.cz ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-11 14:14 [PATCH 0/2 v2] iomap: Waiting for IO in iomap_dio_rw() Jan Kara @ 2019-10-11 14:14 ` Jan Kara 2019-10-11 15:28 ` Darrick J. Wong 2019-10-11 14:14 ` [PATCH 2/2] xfs: Use iomap_dio_rw_wait() Jan Kara 1 sibling, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-11 14:14 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, linux-xfs, darrick.wong, Dave Chinner, Matthew Bobrowski, Jan Kara Filesystems do not support doing IO as asynchronous in some cases. For example in case of unaligned writes or in case file size needs to be extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO in such cases, add argument to iomap_dio_rw() which makes the function wait for IO completion. This also results in executing iomap_dio_complete() inline in iomap_dio_rw() providing its return value to the caller as for ordinary sync IO. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/gfs2/file.c | 6 ++++-- fs/iomap/direct-io.c | 7 +++++-- fs/xfs/xfs_file.c | 5 +++-- include/linux/iomap.h | 3 ++- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 997b326247e2..f0caee2b7c00 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -732,7 +732,8 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to) if (ret) goto out_uninit; - ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL, + is_sync_kiocb(iocb)); gfs2_glock_dq(&gh); out_uninit: @@ -767,7 +768,8 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from) if (offset + len > i_size_read(&ip->i_inode)) goto out; - ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, + is_sync_kiocb(iocb)); out: gfs2_glock_dq(&gh); diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 1fc28c2da279..1bfa0993f705 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -392,7 +392,8 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, */ ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops) + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool wait_for_completion) { struct address_space *mapping = iocb->ki_filp->f_mapping; struct inode *inode = file_inode(iocb->ki_filp); @@ -400,7 +401,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, loff_t pos = iocb->ki_pos, start = pos; loff_t end = iocb->ki_pos + count - 1, ret = 0; unsigned int flags = IOMAP_DIRECT; - bool wait_for_completion = is_sync_kiocb(iocb); struct blk_plug plug; struct iomap_dio *dio; @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (!count) return 0; + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) + return -EINVAL; + dio = kmalloc(sizeof(*dio), GFP_KERNEL); if (!dio) return -ENOMEM; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 1ffb179f35d2..0739ba72a82e 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -188,7 +188,7 @@ xfs_file_dio_aio_read( file_accessed(iocb->ki_filp); xfs_ilock(ip, XFS_IOLOCK_SHARED); - ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL, is_sync_kiocb(iocb)); xfs_iunlock(ip, XFS_IOLOCK_SHARED); return ret; @@ -547,7 +547,8 @@ xfs_file_dio_aio_write( } trace_xfs_file_direct_write(ip, count, iocb->ki_pos); - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops); + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, + is_sync_kiocb(iocb)); /* * If unaligned, this is the only IO in-flight. If it has not yet diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 7aa5d6117936..76b14cb729dc 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -195,7 +195,8 @@ struct iomap_dio_ops { }; ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops); + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool wait_for_completion); int iomap_dio_iopoll(struct kiocb *kiocb, bool spin); #ifdef CONFIG_SWAP -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-11 14:14 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara @ 2019-10-11 15:28 ` Darrick J. Wong 2019-10-11 16:31 ` Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Darrick J. Wong @ 2019-10-11 15:28 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Christoph Hellwig, linux-xfs, Dave Chinner, Matthew Bobrowski On Fri, Oct 11, 2019 at 04:14:31PM +0200, Jan Kara wrote: > Filesystems do not support doing IO as asynchronous in some cases. For > example in case of unaligned writes or in case file size needs to be > extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO > in such cases, add argument to iomap_dio_rw() which makes the function > wait for IO completion. This also results in executing > iomap_dio_complete() inline in iomap_dio_rw() providing its return value > to the caller as for ordinary sync IO. > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/gfs2/file.c | 6 ++++-- > fs/iomap/direct-io.c | 7 +++++-- > fs/xfs/xfs_file.c | 5 +++-- > include/linux/iomap.h | 3 ++- > 4 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c > index 997b326247e2..f0caee2b7c00 100644 > --- a/fs/gfs2/file.c > +++ b/fs/gfs2/file.c > @@ -732,7 +732,8 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to) > if (ret) > goto out_uninit; > > - ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL); > + ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL, > + is_sync_kiocb(iocb)); > > gfs2_glock_dq(&gh); > out_uninit: > @@ -767,7 +768,8 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from) > if (offset + len > i_size_read(&ip->i_inode)) > goto out; > > - ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL); > + ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, > + is_sync_kiocb(iocb)); > > out: > gfs2_glock_dq(&gh); > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index 1fc28c2da279..1bfa0993f705 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -392,7 +392,8 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, > */ > ssize_t > iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > - const struct iomap_ops *ops, const struct iomap_dio_ops *dops) > + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, > + bool wait_for_completion) > { > struct address_space *mapping = iocb->ki_filp->f_mapping; > struct inode *inode = file_inode(iocb->ki_filp); > @@ -400,7 +401,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > loff_t pos = iocb->ki_pos, start = pos; > loff_t end = iocb->ki_pos + count - 1, ret = 0; > unsigned int flags = IOMAP_DIRECT; > - bool wait_for_completion = is_sync_kiocb(iocb); > struct blk_plug plug; > struct iomap_dio *dio; > > @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > if (!count) > return 0; > > + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) > + return -EINVAL; So far in iomap we've been returning EIO when someone internal screws up, which (AFAICT) is the case here. Other than that, the rest of the changes look ok. --D > + > dio = kmalloc(sizeof(*dio), GFP_KERNEL); > if (!dio) > return -ENOMEM; > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 1ffb179f35d2..0739ba72a82e 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -188,7 +188,7 @@ xfs_file_dio_aio_read( > file_accessed(iocb->ki_filp); > > xfs_ilock(ip, XFS_IOLOCK_SHARED); > - ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL); > + ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL, is_sync_kiocb(iocb)); > xfs_iunlock(ip, XFS_IOLOCK_SHARED); > > return ret; > @@ -547,7 +547,8 @@ xfs_file_dio_aio_write( > } > > trace_xfs_file_direct_write(ip, count, iocb->ki_pos); > - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops); > + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, > + is_sync_kiocb(iocb)); > > /* > * If unaligned, this is the only IO in-flight. If it has not yet > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index 7aa5d6117936..76b14cb729dc 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -195,7 +195,8 @@ struct iomap_dio_ops { > }; > > ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > - const struct iomap_ops *ops, const struct iomap_dio_ops *dops); > + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, > + bool wait_for_completion); > int iomap_dio_iopoll(struct kiocb *kiocb, bool spin); > > #ifdef CONFIG_SWAP > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-11 15:28 ` Darrick J. Wong @ 2019-10-11 16:31 ` Jan Kara 2019-10-11 18:04 ` Darrick J. Wong 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-11 16:31 UTC (permalink / raw) To: Darrick J. Wong Cc: Jan Kara, linux-fsdevel, Christoph Hellwig, linux-xfs, Dave Chinner, Matthew Bobrowski On Fri 11-10-19 08:28:21, Darrick J. Wong wrote: > On Fri, Oct 11, 2019 at 04:14:31PM +0200, Jan Kara wrote: > > Filesystems do not support doing IO as asynchronous in some cases. For > > example in case of unaligned writes or in case file size needs to be > > extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO > > in such cases, add argument to iomap_dio_rw() which makes the function > > wait for IO completion. This also results in executing > > iomap_dio_complete() inline in iomap_dio_rw() providing its return value > > to the caller as for ordinary sync IO. > > > > Signed-off-by: Jan Kara <jack@suse.cz> ... > > @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > > if (!count) > > return 0; > > > > + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) > > + return -EINVAL; > > So far in iomap we've been returning EIO when someone internal screws > up, which (AFAICT) is the case here. Yes. Should I resend with -EIO or will you tweak that on commit? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-11 16:31 ` Jan Kara @ 2019-10-11 18:04 ` Darrick J. Wong 2019-10-11 21:13 ` Darrick J. Wong 0 siblings, 1 reply; 11+ messages in thread From: Darrick J. Wong @ 2019-10-11 18:04 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Christoph Hellwig, linux-xfs, Dave Chinner, Matthew Bobrowski On Fri, Oct 11, 2019 at 06:31:27PM +0200, Jan Kara wrote: > On Fri 11-10-19 08:28:21, Darrick J. Wong wrote: > > On Fri, Oct 11, 2019 at 04:14:31PM +0200, Jan Kara wrote: > > > Filesystems do not support doing IO as asynchronous in some cases. For > > > example in case of unaligned writes or in case file size needs to be > > > extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO > > > in such cases, add argument to iomap_dio_rw() which makes the function > > > wait for IO completion. This also results in executing > > > iomap_dio_complete() inline in iomap_dio_rw() providing its return value > > > to the caller as for ordinary sync IO. > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > ... > > > > @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > > > if (!count) > > > return 0; > > > > > > + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) > > > + return -EINVAL; > > > > So far in iomap we've been returning EIO when someone internal screws > > up, which (AFAICT) is the case here. > > Yes. Should I resend with -EIO or will you tweak that on commit? Yes, please. :) --D > Honza > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-11 18:04 ` Darrick J. Wong @ 2019-10-11 21:13 ` Darrick J. Wong 0 siblings, 0 replies; 11+ messages in thread From: Darrick J. Wong @ 2019-10-11 21:13 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Christoph Hellwig, linux-xfs, Dave Chinner, Matthew Bobrowski On Fri, Oct 11, 2019 at 11:04:12AM -0700, Darrick J. Wong wrote: > On Fri, Oct 11, 2019 at 06:31:27PM +0200, Jan Kara wrote: > > On Fri 11-10-19 08:28:21, Darrick J. Wong wrote: > > > On Fri, Oct 11, 2019 at 04:14:31PM +0200, Jan Kara wrote: > > > > Filesystems do not support doing IO as asynchronous in some cases. For > > > > example in case of unaligned writes or in case file size needs to be > > > > extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO > > > > in such cases, add argument to iomap_dio_rw() which makes the function > > > > wait for IO completion. This also results in executing > > > > iomap_dio_complete() inline in iomap_dio_rw() providing its return value > > > > to the caller as for ordinary sync IO. > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > ... > > > > > > @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, > > > > if (!count) > > > > return 0; > > > > > > > > + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) > > > > + return -EINVAL; > > > > > > So far in iomap we've been returning EIO when someone internal screws > > > up, which (AFAICT) is the case here. > > > > Yes. Should I resend with -EIO or will you tweak that on commit? > > Yes, please. :) "Yes, resend the patch with -EIO, please." :/ --D > > --D > > > Honza > > -- > > Jan Kara <jack@suse.com> > > SUSE Labs, CR ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] xfs: Use iomap_dio_rw_wait() 2019-10-11 14:14 [PATCH 0/2 v2] iomap: Waiting for IO in iomap_dio_rw() Jan Kara 2019-10-11 14:14 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara @ 2019-10-11 14:14 ` Jan Kara 2019-10-11 15:28 ` Darrick J. Wong 1 sibling, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-11 14:14 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, linux-xfs, darrick.wong, Dave Chinner, Matthew Bobrowski, Jan Kara Use iomap_dio_rw() to wait for unaligned direct IO instead of opencoding the wait. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/xfs/xfs_file.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 0739ba72a82e..c0620135a279 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -547,16 +547,12 @@ xfs_file_dio_aio_write( } trace_xfs_file_direct_write(ip, count, iocb->ki_pos); - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, - is_sync_kiocb(iocb)); - /* - * If unaligned, this is the only IO in-flight. If it has not yet - * completed, wait on it before we release the iolock to prevent - * subsequent overlapping IO. + * If unaligned, this is the only IO in-flight. Wait on it before we + * release the iolock to prevent subsequent overlapping IO. */ - if (ret == -EIOCBQUEUED && unaligned_io) - inode_dio_wait(inode); + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, + is_sync_kiocb(iocb) || unaligned_io); out: xfs_iunlock(ip, iolock); -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: Use iomap_dio_rw_wait() 2019-10-11 14:14 ` [PATCH 2/2] xfs: Use iomap_dio_rw_wait() Jan Kara @ 2019-10-11 15:28 ` Darrick J. Wong 0 siblings, 0 replies; 11+ messages in thread From: Darrick J. Wong @ 2019-10-11 15:28 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Christoph Hellwig, linux-xfs, Dave Chinner, Matthew Bobrowski On Fri, Oct 11, 2019 at 04:14:32PM +0200, Jan Kara wrote: > Use iomap_dio_rw() to wait for unaligned direct IO instead of opencoding > the wait. > > Signed-off-by: Jan Kara <jack@suse.cz> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_file.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c > index 0739ba72a82e..c0620135a279 100644 > --- a/fs/xfs/xfs_file.c > +++ b/fs/xfs/xfs_file.c > @@ -547,16 +547,12 @@ xfs_file_dio_aio_write( > } > > trace_xfs_file_direct_write(ip, count, iocb->ki_pos); > - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, > - is_sync_kiocb(iocb)); > - > /* > - * If unaligned, this is the only IO in-flight. If it has not yet > - * completed, wait on it before we release the iolock to prevent > - * subsequent overlapping IO. > + * If unaligned, this is the only IO in-flight. Wait on it before we > + * release the iolock to prevent subsequent overlapping IO. > */ > - if (ret == -EIOCBQUEUED && unaligned_io) > - inode_dio_wait(inode); > + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, > + is_sync_kiocb(iocb) || unaligned_io); > out: > xfs_iunlock(ip, iolock); > > -- > 2.16.4 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2 v3] iomap: Waiting for IO in iomap_dio_rw() @ 2019-10-14 8:26 Jan Kara 2019-10-14 8:26 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-14 8:26 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, darrick.wong, linux-xfs, Matthew Bobrowski, Dave Chinner, Jan Kara Hello, here is new version of the series with the small change requested by Darrick. Changes since v2: * Changed iomap_dio_rw() to return -EIO in case caller doesn't ask it to wait for sync kiocb. Changes since v1: * The new function argument of iomap_dio_rw() does not get overridden by is_sync_kiocb() the caller is responsible for this. --- Original motivation: when doing the ext4 conversion of direct IO code to iomap, we found it very difficult to handle inode extension with what iomap code currently provides. Ext4 wants to do inode extension as sync IO (so that the whole duration of IO is protected by inode->i_rwsem), also we need to truncate blocks beyond end of file in case of error or short write. Now in ->end_io handler we don't have the information how long originally the write was (to judge whether we may have allocated more blocks than we actually used) and in ->write_iter we don't know whether / how much of the IO actually succeeded in case of AIO. Thinking about it for some time I think iomap code makes it unnecessarily complex for the filesystem in case it decides it doesn't want to perform AIO and wants to fall back to good old synchronous IO. In such case it is much easier for the filesystem if it just gets normal error return from iomap_dio_rw() and not just -EIOCBQUEUED. The first patch in the series adds argument to iomap_dio_rw() to wait for IO completion (internally iomap_dio_rw() already supports this!) and the second patch converts XFS waiting for unaligned DIO write to this new API. Honza Previous versions: Link: http://lore.kernel.org/r/20191009202736.19227-1-jack@suse.cz Link: http://lore.kernel.org/r/20191011125520.11697-1-jack@suse.cz ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-14 8:26 [PATCH 0/2 v3] iomap: Waiting for IO in iomap_dio_rw() Jan Kara @ 2019-10-14 8:26 ` Jan Kara 2019-10-14 8:40 ` Christoph Hellwig 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-14 8:26 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, darrick.wong, linux-xfs, Matthew Bobrowski, Dave Chinner, Jan Kara Filesystems do not support doing IO as asynchronous in some cases. For example in case of unaligned writes or in case file size needs to be extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO in such cases, add argument to iomap_dio_rw() which makes the function wait for IO completion. This also results in executing iomap_dio_complete() inline in iomap_dio_rw() providing its return value to the caller as for ordinary sync IO. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/gfs2/file.c | 6 ++++-- fs/iomap/direct-io.c | 7 +++++-- fs/xfs/xfs_file.c | 5 +++-- include/linux/iomap.h | 3 ++- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 997b326247e2..f0caee2b7c00 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -732,7 +732,8 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to) if (ret) goto out_uninit; - ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL, + is_sync_kiocb(iocb)); gfs2_glock_dq(&gh); out_uninit: @@ -767,7 +768,8 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from) if (offset + len > i_size_read(&ip->i_inode)) goto out; - ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, + is_sync_kiocb(iocb)); out: gfs2_glock_dq(&gh); diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 1fc28c2da279..da124cee1783 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -392,7 +392,8 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, */ ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops) + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool wait_for_completion) { struct address_space *mapping = iocb->ki_filp->f_mapping; struct inode *inode = file_inode(iocb->ki_filp); @@ -400,7 +401,6 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, loff_t pos = iocb->ki_pos, start = pos; loff_t end = iocb->ki_pos + count - 1, ret = 0; unsigned int flags = IOMAP_DIRECT; - bool wait_for_completion = is_sync_kiocb(iocb); struct blk_plug plug; struct iomap_dio *dio; @@ -409,6 +409,9 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, if (!count) return 0; + if (WARN_ON(is_sync_kiocb(iocb) && !wait_for_completion)) + return -EIO; + dio = kmalloc(sizeof(*dio), GFP_KERNEL); if (!dio) return -ENOMEM; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 1ffb179f35d2..0739ba72a82e 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -188,7 +188,7 @@ xfs_file_dio_aio_read( file_accessed(iocb->ki_filp); xfs_ilock(ip, XFS_IOLOCK_SHARED); - ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL, is_sync_kiocb(iocb)); xfs_iunlock(ip, XFS_IOLOCK_SHARED); return ret; @@ -547,7 +547,8 @@ xfs_file_dio_aio_write( } trace_xfs_file_direct_write(ip, count, iocb->ki_pos); - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops); + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, + is_sync_kiocb(iocb)); /* * If unaligned, this is the only IO in-flight. If it has not yet diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 7aa5d6117936..76b14cb729dc 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -195,7 +195,8 @@ struct iomap_dio_ops { }; ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops); + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool wait_for_completion); int iomap_dio_iopoll(struct kiocb *kiocb, bool spin); #ifdef CONFIG_SWAP -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-14 8:26 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara @ 2019-10-14 8:40 ` Christoph Hellwig 0 siblings, 0 replies; 11+ messages in thread From: Christoph Hellwig @ 2019-10-14 8:40 UTC (permalink / raw) To: Jan Kara Cc: linux-fsdevel, Christoph Hellwig, darrick.wong, linux-xfs, Matthew Bobrowski, Dave Chinner Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw() @ 2019-10-09 20:41 Jan Kara 2019-10-09 20:41 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara 0 siblings, 1 reply; 11+ messages in thread From: Jan Kara @ 2019-10-09 20:41 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, darrick.wong, linux-xfs, Matthew Bobrowski, Jan Kara Hello, when doing the ext4 conversion of direct IO code to iomap, we found it very difficult to handle inode extension with what iomap code currently provides. Ext4 wants to do inode extension as sync IO (so that the whole duration of IO is protected by inode->i_rwsem), also we need to truncate blocks beyond end of file in case of error or short write. Now in ->end_io handler we don't have the information how long originally the write was (to judge whether we may have allocated more blocks than we actually used) and in ->write_iter we don't know whether / how much of the IO actually succeeded in case of AIO. Thinking about it for some time I think iomap code makes it unnecessarily complex for the filesystem in case it decides it doesn't want to perform AIO and wants to fall back to good old synchronous IO. In such case it is much easier for the filesystem if it just gets normal error return from iomap_dio_rw() and not just -EIOCBQUEUED. The first patch in the series adds argument to iomap_dio_rw() to wait for IO completion (internally iomap_dio_rw() already supports this!) and the second patch converts XFS waiting for unaligned DIO write to this new API. What do people think? Honza ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw() 2019-10-09 20:41 [PATCH 0/2] iomap: Waiting for IO " Jan Kara @ 2019-10-09 20:41 ` Jan Kara 0 siblings, 0 replies; 11+ messages in thread From: Jan Kara @ 2019-10-09 20:41 UTC (permalink / raw) To: linux-fsdevel Cc: Christoph Hellwig, darrick.wong, linux-xfs, Matthew Bobrowski, Jan Kara Filesystems do not support doing IO as asynchronous in some cases. For example in case of unaligned writes or in case file size needs to be extended (e.g. for ext4). Instead of forcing filesystem to wait for AIO in such cases, add argument to iomap_dio_rw() which makes the function wait for IO completion. This also results in executing iomap_dio_complete() inline in iomap_dio_rw() providing its return value to the caller as for ordinary sync IO. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/gfs2/file.c | 4 ++-- fs/iomap/direct-io.c | 5 +++-- fs/xfs/xfs_file.c | 5 +++-- include/linux/iomap.h | 3 ++- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c index 997b326247e2..4650a7920ad8 100644 --- a/fs/gfs2/file.c +++ b/fs/gfs2/file.c @@ -732,7 +732,7 @@ static ssize_t gfs2_file_direct_read(struct kiocb *iocb, struct iov_iter *to) if (ret) goto out_uninit; - ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &gfs2_iomap_ops, NULL, false); gfs2_glock_dq(&gh); out_uninit: @@ -767,7 +767,7 @@ static ssize_t gfs2_file_direct_write(struct kiocb *iocb, struct iov_iter *from) if (offset + len > i_size_read(&ip->i_inode)) goto out; - ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, from, &gfs2_iomap_ops, NULL, false); out: gfs2_glock_dq(&gh); diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 1fc28c2da279..9969cc135aeb 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -392,7 +392,8 @@ iomap_dio_actor(struct inode *inode, loff_t pos, loff_t length, */ ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops) + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool force_wait) { struct address_space *mapping = iocb->ki_filp->f_mapping; struct inode *inode = file_inode(iocb->ki_filp); @@ -400,7 +401,7 @@ iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, loff_t pos = iocb->ki_pos, start = pos; loff_t end = iocb->ki_pos + count - 1, ret = 0; unsigned int flags = IOMAP_DIRECT; - bool wait_for_completion = is_sync_kiocb(iocb); + bool wait_for_completion = is_sync_kiocb(iocb) || force_wait; struct blk_plug plug; struct iomap_dio *dio; diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 1ffb179f35d2..7d0dc21c14ca 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -188,7 +188,7 @@ xfs_file_dio_aio_read( file_accessed(iocb->ki_filp); xfs_ilock(ip, XFS_IOLOCK_SHARED); - ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL); + ret = iomap_dio_rw(iocb, to, &xfs_iomap_ops, NULL, false); xfs_iunlock(ip, XFS_IOLOCK_SHARED); return ret; @@ -547,7 +547,8 @@ xfs_file_dio_aio_write( } trace_xfs_file_direct_write(ip, count, iocb->ki_pos); - ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops); + ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, &xfs_dio_write_ops, + false); /* * If unaligned, this is the only IO in-flight. If it has not yet diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 7aa5d6117936..fd245f4e8249 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -195,7 +195,8 @@ struct iomap_dio_ops { }; ssize_t iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter, - const struct iomap_ops *ops, const struct iomap_dio_ops *dops); + const struct iomap_ops *ops, const struct iomap_dio_ops *dops, + bool force_wait); int iomap_dio_iopoll(struct kiocb *kiocb, bool spin); #ifdef CONFIG_SWAP -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-10-14 8:41 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-11 14:14 [PATCH 0/2 v2] iomap: Waiting for IO in iomap_dio_rw() Jan Kara 2019-10-11 14:14 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara 2019-10-11 15:28 ` Darrick J. Wong 2019-10-11 16:31 ` Jan Kara 2019-10-11 18:04 ` Darrick J. Wong 2019-10-11 21:13 ` Darrick J. Wong 2019-10-11 14:14 ` [PATCH 2/2] xfs: Use iomap_dio_rw_wait() Jan Kara 2019-10-11 15:28 ` Darrick J. Wong -- strict thread matches above, loose matches on Subject: below -- 2019-10-14 8:26 [PATCH 0/2 v3] iomap: Waiting for IO in iomap_dio_rw() Jan Kara 2019-10-14 8:26 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara 2019-10-14 8:40 ` Christoph Hellwig 2019-10-09 20:41 [PATCH 0/2] iomap: Waiting for IO " Jan Kara 2019-10-09 20:41 ` [PATCH 1/2] iomap: Allow forcing of waiting for running DIO " Jan Kara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).