[PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[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 

[PATCH 1/2] iomap: Allow forcing of waiting for running DIO in iomap_dio_rw()

[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

[PATCH 2/2] xfs: Use iomap_dio_rw_wait()

[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 7d0dc21c14ca..f35cea628bed 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,
-			   false);
-
 	/*
-	 * 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,
+			   unaligned_io);
 out:
 	xfs_iunlock(ip, iolock);
 
-- 
2.16.4

Re: [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[Dave Chinner]

On Wed, Oct 09, 2019 at 10:41:24PM +0200, Jan Kara wrote:
> 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.

Yeah, that'd be nice. :)

> 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?

I've just caught up on the ext4 iomap dio thread where this came up,
so I have some idea of what is going on now :)

My main issue is that I don't like the idea of a "force_wait"
parameter to iomap_dio_rw() that overrides what the kiocb says to
do inside iomap_dio_rw(). It just seems ... clunky.

I'd much prefer that the entire sync/async IO decision is done in
one spot, and the result of that is passed into iomap_dio_rw(). i.e.
the caller always determines the behaviour.

That would mean the callers need to do something like this by
default:

	ret = iomap_dio_rw(iocb, iter, ops, dops, is_sync_kiocb(iocb));

And filesystems like XFS will need to do:

	ret = iomap_dio_rw(iocb, iter, ops, dops,
			is_sync_kiocb(iocb) || unaligned);

and ext4 will calculate the parameter in whatever way it needs to.

In fact, it may be that a wrapper function is better for existing
callers:

static inline ssize_t iomap_dio_rw()
{
	return iomap_dio_rw_wait(iocb, iter, ops, dops, is_sync_kiocb(iocb));
}

And XFS/ext4 writes call iomap_dio_rw_wait() directly. That way we
don't need to change the read code at all...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[Christoph Hellwig]

On Thu, Oct 10, 2019 at 10:02:27AM +1100, Dave Chinner wrote:
> That would mean the callers need to do something like this by
> default:
> 
> 	ret = iomap_dio_rw(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> 
> And filesystems like XFS will need to do:
> 
> 	ret = iomap_dio_rw(iocb, iter, ops, dops,
> 			is_sync_kiocb(iocb) || unaligned);
> 
> and ext4 will calculate the parameter in whatever way it needs to.

I defintively like that.

> 
> In fact, it may be that a wrapper function is better for existing
> callers:
> 
> static inline ssize_t iomap_dio_rw()
> {
> 	return iomap_dio_rw_wait(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> }
> 
> And XFS/ext4 writes call iomap_dio_rw_wait() directly. That way we
> don't need to change the read code at all...

I have to say I really hated the way we were growing all these wrappers
in the old direct I/O code, so I've been asked Jan to not add the
wrapper in his old version.  But compared to the force_sync version it
at least makes a little more sense here.  I'm just not sure if
iomap_dio_rw_wait is the right name, but the __-prefix convention for
non-trivial differences also sucks.  I can't think of a better name,
though.

Re: [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[Darrick J. Wong]

On Thu, Oct 10, 2019 at 12:54:20AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 10, 2019 at 10:02:27AM +1100, Dave Chinner wrote:
> > That would mean the callers need to do something like this by
> > default:
> > 
> > 	ret = iomap_dio_rw(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> > 
> > And filesystems like XFS will need to do:
> > 
> > 	ret = iomap_dio_rw(iocb, iter, ops, dops,
> > 			is_sync_kiocb(iocb) || unaligned);
> > 
> > and ext4 will calculate the parameter in whatever way it needs to.
> 
> I defintively like that.
> 
> > 
> > In fact, it may be that a wrapper function is better for existing
> > callers:
> > 
> > static inline ssize_t iomap_dio_rw()
> > {
> > 	return iomap_dio_rw_wait(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> > }
> > 
> > And XFS/ext4 writes call iomap_dio_rw_wait() directly. That way we
> > don't need to change the read code at all...
> 
> I have to say I really hated the way we were growing all these wrappers
> in the old direct I/O code, so I've been asked Jan to not add the
> wrapper in his old version.  But compared to the force_sync version it
> at least makes a little more sense here.  I'm just not sure if
> iomap_dio_rw_wait is the right name, but the __-prefix convention for
> non-trivial differences also sucks.  I can't think of a better name,
> though.

<shrug> I'd just add the 'bool wait' parameter at the end of
iomap_dio_rw() and leave it that way.  If we ever develop more than one
caller that passes in "is_sync_kiocb(iocb)" (or more than two lucky
callers screwing it up I guess?) for that parameter then maybe we can
re-evaluate.

--D

Re: [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[Jan Kara]

On Thu 10-10-19 07:47:18, Darrick J. Wong wrote:
> On Thu, Oct 10, 2019 at 12:54:20AM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 10, 2019 at 10:02:27AM +1100, Dave Chinner wrote:
> > > That would mean the callers need to do something like this by
> > > default:
> > > 
> > > 	ret = iomap_dio_rw(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> > > 
> > > And filesystems like XFS will need to do:
> > > 
> > > 	ret = iomap_dio_rw(iocb, iter, ops, dops,
> > > 			is_sync_kiocb(iocb) || unaligned);
> > > 
> > > and ext4 will calculate the parameter in whatever way it needs to.
> > 
> > I defintively like that.
> > 
> > > 
> > > In fact, it may be that a wrapper function is better for existing
> > > callers:
> > > 
> > > static inline ssize_t iomap_dio_rw()
> > > {
> > > 	return iomap_dio_rw_wait(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> > > }
> > > 
> > > And XFS/ext4 writes call iomap_dio_rw_wait() directly. That way we
> > > don't need to change the read code at all...
> > 
> > I have to say I really hated the way we were growing all these wrappers
> > in the old direct I/O code, so I've been asked Jan to not add the
> > wrapper in his old version.  But compared to the force_sync version it
> > at least makes a little more sense here.  I'm just not sure if
> > iomap_dio_rw_wait is the right name, but the __-prefix convention for
> > non-trivial differences also sucks.  I can't think of a better name,
> > though.
> 
> <shrug> I'd just add the 'bool wait' parameter at the end of
> iomap_dio_rw() and leave it that way.  If we ever develop more than one
> caller that passes in "is_sync_kiocb(iocb)" (or more than two lucky
> callers screwing it up I guess?) for that parameter then maybe we can
> re-evaluate.

OK, fine by me. I guess this is the least controversial proposal so I'll
resend patches with this change tomorrow...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] iomap: Waiting for IO in iomap_dio_rw()

[Jan Kara]

On Thu 10-10-19 10:02:27, Dave Chinner wrote:
> On Wed, Oct 09, 2019 at 10:41:24PM +0200, Jan Kara wrote:
> > 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.
> 
> Yeah, that'd be nice. :)
> 
> > 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?
> 
> I've just caught up on the ext4 iomap dio thread where this came up,
> so I have some idea of what is going on now :)
> 
> My main issue is that I don't like the idea of a "force_wait"
> parameter to iomap_dio_rw() that overrides what the kiocb says to
> do inside iomap_dio_rw(). It just seems ... clunky.
> 
> I'd much prefer that the entire sync/async IO decision is done in
> one spot, and the result of that is passed into iomap_dio_rw(). i.e.
> the caller always determines the behaviour.
> 
> That would mean the callers need to do something like this by
> default:
> 
> 	ret = iomap_dio_rw(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> 
> And filesystems like XFS will need to do:
> 
> 	ret = iomap_dio_rw(iocb, iter, ops, dops,
> 			is_sync_kiocb(iocb) || unaligned);

Yeah, I've considered that as well. I just didn't like repeating
is_sync_kiocb(iocb) in all the callers when all the callers actually have
to have something like (is_sync_kiocb(iocb) || (some special conditions))
to be correct. And in fact it is not a definitive decision either as
iomap_dio_rw() can decide to override caller's wish and do the IO
synchronously anyway (when it gets -ENOTBLK from the filesystem). That's why
I came up with 'force_wait' argument, which isn't exactly beautiful either, I
agree.

> and ext4 will calculate the parameter in whatever way it needs to.
> 
> In fact, it may be that a wrapper function is better for existing
> callers:
> 
> static inline ssize_t iomap_dio_rw()
> {
> 	return iomap_dio_rw_wait(iocb, iter, ops, dops, is_sync_kiocb(iocb));
> }
> 
> And XFS/ext4 writes call iomap_dio_rw_wait() directly. That way we
> don't need to change the read code at all...

Yeah, this is similar to what I had in my previous version [1]. There I had
__iomap_dio_rw() with bool argument, iomap_dio_rw() passing is_sync_kiocb(iocb)
to __iomap_dio_rw() (i.e., fully backward compatible), and iomap_dio_rw_wait()
which executed IO synchronously. But Christoph didn't like the wrappers.

I can go with just one wrapper like you suggest if that's what people
prefer. I don't care much we just have to settle on something...

								Honza

[1] https://lore.kernel.org/linux-ext4/20191008151238.GK5078@quack2.suse.cz/
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR