enable iomap dio write completions from interrupt context

enable iomap dio write completions from interrupt context

[Christoph Hellwig]

Hi all,

Currently iomap defers all write completions to interrupt context.  This
was based on my assumption that no one cares about the latency of those
to simplify the code vs the old direct-io.c.  It turns out someone cared,
as Avi reported a lot of context switches with ScyllaDB, which at least
in older kernels with workqueue scheduling issues caused really high
tail latencies.

Fortunately allowing the direct completions is pretty easy with all the
other iomap changes we had since.

While doing this I've also found dead code which gets removed (patch 1)
and an incorrect assumption in zonefs that read completions are called
in user context, which it assumes for it's error handling.  Fix this by
always calling error completions from user context (patch 2).

Against the vfs/vfs-6.19.iomap branch.

Diffstat:
 Documentation/filesystems/iomap/operations.rst |    4 
 fs/backing-file.c                              |    6 -
 fs/iomap/direct-io.c                           |  149 +++++++++++--------------
 include/linux/fs.h                             |   43 +------
 io_uring/rw.c                                  |   16 --
 5 files changed, 81 insertions(+), 137 deletions(-)

[PATCH 1/5] fs, iomap: remove IOCB_DIO_CALLER_COMP

[Christoph Hellwig]

This was added by commit 099ada2c8726 ("io_uring/rw: add write support
for IOCB_DIO_CALLER_COMP") and disabled a little later by commit
838b35bb6a89 ("io_uring/rw: disable IOCB_DIO_CALLER_COMP") because it
didn't work.  Remove all the related code that sat unused for 2 years.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 .../filesystems/iomap/operations.rst          |  4 --
 fs/backing-file.c                             |  6 --
 fs/iomap/direct-io.c                          | 56 +------------------
 include/linux/fs.h                            | 43 +++-----------
 io_uring/rw.c                                 | 16 +-----
 5 files changed, 13 insertions(+), 112 deletions(-)

diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
index c88205132039..5558a44891bb 100644
--- a/Documentation/filesystems/iomap/operations.rst
+++ b/Documentation/filesystems/iomap/operations.rst
@@ -488,10 +488,6 @@ These ``struct kiocb`` flags are significant for direct I/O with iomap:
    Only meaningful for asynchronous I/O, and only if the entire I/O can
    be issued as a single ``struct bio``.
 
- * ``IOCB_DIO_CALLER_COMP``: Try to run I/O completion from the caller's
-   process context.
-   See ``linux/fs.h`` for more details.
-
 Filesystems should call ``iomap_dio_rw`` from ``->read_iter`` and
 ``->write_iter``, and set ``FMODE_CAN_ODIRECT`` in the ``->open``
 function for the file.
diff --git a/fs/backing-file.c b/fs/backing-file.c
index 15a7f8031084..2a86bb6fcd13 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -227,12 +227,6 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 	    !(file->f_mode & FMODE_CAN_ODIRECT))
 		return -EINVAL;
 
-	/*
-	 * Stacked filesystems don't support deferred completions, don't copy
-	 * this property in case it is set by the issuer.
-	 */
-	flags &= ~IOCB_DIO_CALLER_COMP;
-
 	old_cred = override_creds(ctx->cred);
 	if (is_sync_kiocb(iocb)) {
 		rwf_t rwf = iocb_to_rw_flags(flags);
diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 8b2f9fb89eb3..7659db85083a 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -16,8 +16,7 @@
  * Private flags for iomap_dio, must not overlap with the public ones in
  * iomap.h:
  */
-#define IOMAP_DIO_NO_INVALIDATE	(1U << 25)
-#define IOMAP_DIO_CALLER_COMP	(1U << 26)
+#define IOMAP_DIO_NO_INVALIDATE	(1U << 26)
 #define IOMAP_DIO_INLINE_COMP	(1U << 27)
 #define IOMAP_DIO_WRITE_THROUGH	(1U << 28)
 #define IOMAP_DIO_NEED_SYNC	(1U << 29)
@@ -140,11 +139,6 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
 }
 EXPORT_SYMBOL_GPL(iomap_dio_complete);
 
-static ssize_t iomap_dio_deferred_complete(void *data)
-{
-	return iomap_dio_complete(data);
-}
-
 static void iomap_dio_complete_work(struct work_struct *work)
 {
 	struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
@@ -182,29 +176,6 @@ static void iomap_dio_done(struct iomap_dio *dio)
 	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
 		WRITE_ONCE(iocb->private, NULL);
 		iomap_dio_complete_work(&dio->aio.work);
-	} else if (dio->flags & IOMAP_DIO_CALLER_COMP) {
-		/*
-		 * If this dio is flagged with IOMAP_DIO_CALLER_COMP, then
-		 * schedule our completion that way to avoid an async punt to a
-		 * workqueue.
-		 */
-		/* only polled IO cares about private cleared */
-		iocb->private = dio;
-		iocb->dio_complete = iomap_dio_deferred_complete;
-
-		/*
-		 * Invoke ->ki_complete() directly. We've assigned our
-		 * dio_complete callback handler, and since the issuer set
-		 * IOCB_DIO_CALLER_COMP, we know their ki_complete handler will
-		 * notice ->dio_complete being set and will defer calling that
-		 * handler until it can be done from a safe task context.
-		 *
-		 * Note that the 'res' being passed in here is not important
-		 * for this case. The actual completion value of the request
-		 * will be gotten from dio_complete when that is run by the
-		 * issuer.
-		 */
-		iocb->ki_complete(iocb, 0);
 	} else {
 		struct inode *inode = file_inode(iocb->ki_filp);
 
@@ -261,7 +232,6 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)
 			dio->flags |= IOMAP_DIO_INLINE_COMP;
 			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
 		}
-		dio->flags &= ~IOMAP_DIO_CALLER_COMP;
 		iomap_dio_done(dio);
 	}
 
@@ -380,19 +350,6 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 
 		if (!(bio_opf & REQ_FUA))
 			dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
-
-		/*
-		 * We can only do deferred completion for pure overwrites that
-		 * don't require additional I/O at completion time.
-		 *
-		 * This rules out writes that need zeroing or extent conversion,
-		 * extend the file size, or issue metadata I/O or cache flushes
-		 * during completion processing.
-		 */
-		if (need_zeroout || (pos >= i_size_read(inode)) ||
-		    ((dio->flags & IOMAP_DIO_NEED_SYNC) &&
-		     !(bio_opf & REQ_FUA)))
-			dio->flags &= ~IOMAP_DIO_CALLER_COMP;
 	} else {
 		bio_opf |= REQ_OP_READ;
 	}
@@ -413,7 +370,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 	 * ones we set for inline and deferred completions. If none of those
 	 * are available for this IO, clear the polled flag.
 	 */
-	if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP)))
+	if (!(dio->flags & IOMAP_DIO_INLINE_COMP))
 		dio->iocb->ki_flags &= ~IOCB_HIPRI;
 
 	if (need_zeroout) {
@@ -669,15 +626,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		iomi.flags |= IOMAP_WRITE;
 		dio->flags |= IOMAP_DIO_WRITE;
 
-		/*
-		 * Flag as supporting deferred completions, if the issuer
-		 * groks it. This can avoid a workqueue punt for writes.
-		 * We may later clear this flag if we need to do other IO
-		 * as part of this IO completion.
-		 */
-		if (iocb->ki_flags & IOCB_DIO_CALLER_COMP)
-			dio->flags |= IOMAP_DIO_CALLER_COMP;
-
 		if (dio_flags & IOMAP_DIO_OVERWRITE_ONLY) {
 			ret = -EAGAIN;
 			if (iomi.pos >= dio->i_size ||
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c895146c1444..e210d2d8af53 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -367,23 +367,9 @@ struct readahead_control;
 #define IOCB_NOIO		(1 << 20)
 /* can use bio alloc cache */
 #define IOCB_ALLOC_CACHE	(1 << 21)
-/*
- * IOCB_DIO_CALLER_COMP can be set by the iocb owner, to indicate that the
- * iocb completion can be passed back to the owner for execution from a safe
- * context rather than needing to be punted through a workqueue. If this
- * flag is set, the bio completion handling may set iocb->dio_complete to a
- * handler function and iocb->private to context information for that handler.
- * The issuer should call the handler with that context information from task
- * context to complete the processing of the iocb. Note that while this
- * provides a task context for the dio_complete() callback, it should only be
- * used on the completion side for non-IO generating completions. It's fine to
- * call blocking functions from this callback, but they should not wait for
- * unrelated IO (like cache flushing, new IO generation, etc).
- */
-#define IOCB_DIO_CALLER_COMP	(1 << 22)
 /* kiocb is a read or write operation submitted by fs/aio.c. */
-#define IOCB_AIO_RW		(1 << 23)
-#define IOCB_HAS_METADATA	(1 << 24)
+#define IOCB_AIO_RW		(1 << 22)
+#define IOCB_HAS_METADATA	(1 << 23)
 
 /* for use in trace events */
 #define TRACE_IOCB_STRINGS \
@@ -400,7 +386,6 @@ struct readahead_control;
 	{ IOCB_WAITQ,		"WAITQ" }, \
 	{ IOCB_NOIO,		"NOIO" }, \
 	{ IOCB_ALLOC_CACHE,	"ALLOC_CACHE" }, \
-	{ IOCB_DIO_CALLER_COMP,	"CALLER_COMP" }, \
 	{ IOCB_AIO_RW,		"AIO_RW" }, \
 	{ IOCB_HAS_METADATA,	"AIO_HAS_METADATA" }
 
@@ -412,23 +397,13 @@ struct kiocb {
 	int			ki_flags;
 	u16			ki_ioprio; /* See linux/ioprio.h */
 	u8			ki_write_stream;
-	union {
-		/*
-		 * Only used for async buffered reads, where it denotes the
-		 * page waitqueue associated with completing the read. Valid
-		 * IFF IOCB_WAITQ is set.
-		 */
-		struct wait_page_queue	*ki_waitq;
-		/*
-		 * Can be used for O_DIRECT IO, where the completion handling
-		 * is punted back to the issuer of the IO. May only be set
-		 * if IOCB_DIO_CALLER_COMP is set by the issuer, and the issuer
-		 * must then check for presence of this handler when ki_complete
-		 * is invoked. The data passed in to this handler must be
-		 * assigned to ->private when dio_complete is assigned.
-		 */
-		ssize_t (*dio_complete)(void *data);
-	};
+
+	/*
+	 * Only used for async buffered reads, where it denotes the page
+	 * waitqueue associated with completing the read.
+	 * Valid IFF IOCB_WAITQ is set.
+	 */
+	struct wait_page_queue	*ki_waitq;
 };
 
 static inline bool is_sync_kiocb(struct kiocb *kiocb)
diff --git a/io_uring/rw.c b/io_uring/rw.c
index 08882648d569..4d0ab8f50d14 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -277,7 +277,6 @@ static int __io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	} else {
 		rw->kiocb.ki_ioprio = get_current_ioprio();
 	}
-	rw->kiocb.dio_complete = NULL;
 	rw->kiocb.ki_flags = 0;
 	rw->kiocb.ki_write_stream = READ_ONCE(sqe->write_stream);
 
@@ -566,15 +565,6 @@ static inline int io_fixup_rw_res(struct io_kiocb *req, long res)
 
 void io_req_rw_complete(struct io_kiocb *req, io_tw_token_t tw)
 {
-	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
-	struct kiocb *kiocb = &rw->kiocb;
-
-	if ((kiocb->ki_flags & IOCB_DIO_CALLER_COMP) && kiocb->dio_complete) {
-		long res = kiocb->dio_complete(rw->kiocb.private);
-
-		io_req_set_res(req, io_fixup_rw_res(req, res), 0);
-	}
-
 	io_req_io_end(req);
 
 	if (req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING))
@@ -589,10 +579,8 @@ static void io_complete_rw(struct kiocb *kiocb, long res)
 	struct io_rw *rw = container_of(kiocb, struct io_rw, kiocb);
 	struct io_kiocb *req = cmd_to_io_kiocb(rw);
 
-	if (!kiocb->dio_complete || !(kiocb->ki_flags & IOCB_DIO_CALLER_COMP)) {
-		__io_complete_rw_common(req, res);
-		io_req_set_res(req, io_fixup_rw_res(req, res), 0);
-	}
+	__io_complete_rw_common(req, res);
+	io_req_set_res(req, io_fixup_rw_res(req, res), 0);
 	req->io_task_work.func = io_req_rw_complete;
 	__io_req_task_work_add(req, IOU_F_TWQ_LAZY_WAKE);
 }
-- 
2.47.3

[PATCH 2/5] iomap: always run error completions in user context

[Christoph Hellwig]

At least zonefs expects error completions to be able to sleep.  Because
error completions aren't performance critical, just defer them to workqueue
context unconditionally.

Fixes: 8dcc1a9d90c1 ("fs: New zonefs file system")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/direct-io.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 7659db85083a..765ab6dd6637 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -173,7 +173,18 @@ static void iomap_dio_done(struct iomap_dio *dio)
 
 		WRITE_ONCE(dio->submit.waiter, NULL);
 		blk_wake_io_task(waiter);
-	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
+		return;
+	}
+
+	/*
+	 * Always run error completions in user context.  These are not
+	 * performance critical and some code relies on taking sleeping locks
+	 * for error handling.
+	 */
+	if (dio->error)
+		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+
+	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
 		WRITE_ONCE(iocb->private, NULL);
 		iomap_dio_complete_work(&dio->aio.work);
 	} else {
-- 
2.47.3

[PATCH 3/5] iomap: rework REQ_FUA selection

[Christoph Hellwig]

The way how iomap_dio_can_use_fua and the caller is structured is
a bit confusing, as the main guarding condition is hidden in the
helper, and the secondary conditions are split between caller and
callee.

Refactor the code, so that there is a main IOMAP_DIO_WRITE_THROUGH
guard in iomap_dio_bio_iter, which is directly tied to clearing it
when not supported, and a helper that just checks if the I/O is a
pure overwrite.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/direct-io.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 765ab6dd6637..c4a883fa8ea5 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -288,20 +288,14 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
 }
 
 /*
- * Use a FUA write if we need datasync semantics and this is a pure data I/O
- * that doesn't require any metadata updates (including after I/O completion
- * such as unwritten extent conversion) and the underlying device either
- * doesn't have a volatile write cache or supports FUA.
- * This allows us to avoid cache flushes on I/O completion.
+ * Check if this mapping is a pure overwrite that does not need metadata updates
+ * at I/O completion time.
  */
-static inline bool iomap_dio_can_use_fua(const struct iomap *iomap,
-		struct iomap_dio *dio)
+static inline bool iomap_dio_is_overwrite(const struct iomap *iomap)
 {
-	if (iomap->flags & (IOMAP_F_SHARED | IOMAP_F_DIRTY))
+	if (iomap->type != IOMAP_MAPPED)
 		return false;
-	if (!(dio->flags & IOMAP_DIO_WRITE_THROUGH))
-		return false;
-	return !bdev_write_cache(iomap->bdev) || bdev_fua(iomap->bdev);
+	return !(iomap->flags & (IOMAP_F_NEW | IOMAP_F_SHARED));
 }
 
 static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
@@ -355,12 +349,22 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 
 		if (iomap->flags & IOMAP_F_NEW)
 			need_zeroout = true;
-		else if (iomap->type == IOMAP_MAPPED &&
-			 iomap_dio_can_use_fua(iomap, dio))
-			bio_opf |= REQ_FUA;
 
-		if (!(bio_opf & REQ_FUA))
-			dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
+		/*
+		 * Use a FUA write if we need datasync semantics and this is a
+		 * pure overwrite that doesn't require any metadata updates.
+		 *
+		 * This allows us to avoid cache flushes on I/O completion.
+		 */
+		if (dio->flags & IOMAP_DIO_WRITE_THROUGH) {
+			if (iomap_dio_is_overwrite(iomap) &&
+			    !(iomap->flags & IOMAP_F_DIRTY) &&
+			    (!bdev_write_cache(iomap->bdev) ||
+			     bdev_fua(iomap->bdev)))
+				bio_opf |= REQ_FUA;
+			else
+				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
+		}
 	} else {
 		bio_opf |= REQ_OP_READ;
 	}
-- 
2.47.3

[PATCH 4/5] iomap: support write completions from interrupt context

[Christoph Hellwig]

Completions for pure overwrites don't need to be deferred to a workqueue
as there is no work to be done, or at least no work that needs a user
context.  Set the IOMAP_DIO_INLINE_COMP by default for writes like we
already do for reads, and the clear it for all the cases that actually
do need a user context for completions to update the inode size or
record updates to the logical to physical mapping.

I've audited all users of the ->end_io callback, and they only require
user context for I/O that involves unwritten extents, COW, size
extensions, or error handling and all those are still run from workqueue
context.

This restores the behavior of the old pre-iomap direct I/O code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/direct-io.c | 55 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index c4a883fa8ea5..df313232f422 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -184,6 +184,21 @@ static void iomap_dio_done(struct iomap_dio *dio)
 	if (dio->error)
 		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
 
+	/*
+	 * Never invalidate pages from this context to avoid deadlocks with
+	 * buffered I/O completions when called from the ioend workqueue,
+	 * or avoid sleeping when called directly from ->bi_end_io.
+	 * Tough luck if you hit the tiny race with someone dirtying the range
+	 * right between this check and the actual completion.
+	 */
+	if ((dio->flags & IOMAP_DIO_WRITE) &&
+	    (dio->flags & IOMAP_DIO_INLINE_COMP)) {
+		if (dio->iocb->ki_filp->f_mapping->nrpages)
+			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+		else
+			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
+	}
+
 	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
 		WRITE_ONCE(iocb->private, NULL);
 		iomap_dio_complete_work(&dio->aio.work);
@@ -234,15 +249,9 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)
 		/*
 		 * Try to avoid another context switch for the completion given
 		 * that we are already called from the ioend completion
-		 * workqueue, but never invalidate pages from this thread to
-		 * avoid deadlocks with buffered I/O completions.  Tough luck if
-		 * you hit the tiny race with someone dirtying the range now
-		 * between this check and the actual completion.
+		 * workqueue.
 		 */
-		if (!dio->iocb->ki_filp->f_mapping->nrpages) {
-			dio->flags |= IOMAP_DIO_INLINE_COMP;
-			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
-		}
+		dio->flags |= IOMAP_DIO_INLINE_COMP;
 		iomap_dio_done(dio);
 	}
 
@@ -365,6 +374,16 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 			else
 				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
 		}
+
+		/*
+		 * We can only do inline completion for pure overwrites that
+		 * don't require additional I/O at completion time.
+		 *
+		 * This rules out writes that need zeroing or extent conversion,
+		 * or extend the file size.
+		 */
+		if (!iomap_dio_is_overwrite(iomap))
+			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
 	} else {
 		bio_opf |= REQ_OP_READ;
 	}
@@ -624,10 +643,13 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	if (dio_flags & IOMAP_DIO_FSBLOCK_ALIGNED)
 		dio->flags |= IOMAP_DIO_FSBLOCK_ALIGNED;
 
-	if (iov_iter_rw(iter) == READ) {
-		/* reads can always complete inline */
-		dio->flags |= IOMAP_DIO_INLINE_COMP;
+	/*
+	 * Try to complete inline if we can.  For reads this is always possible,
+	 * but for writes we'll end up clearing this more often than not.
+	 */
+	dio->flags |= IOMAP_DIO_INLINE_COMP;
 
+	if (iov_iter_rw(iter) == READ) {
 		if (iomi.pos >= dio->i_size)
 			goto out_free_dio;
 
@@ -669,6 +691,12 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 				dio->flags |= IOMAP_DIO_WRITE_THROUGH;
 		}
 
+		/*
+		 * Inode size updates must to happen from process context.
+		 */
+		if (iomi.pos + iomi.len > dio->i_size)
+			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+
 		/*
 		 * Try to invalidate cache pages for the range we are writing.
 		 * If this invalidation fails, let the caller fall back to
@@ -741,9 +769,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	 * If all the writes we issued were already written through to the
 	 * media, we don't need to flush the cache on IO completion. Clear the
 	 * sync flag for this case.
+	 *
+	 * Otherwise clear the inline completion flag if any sync work is
+	 * needed, as that needs to be performed from process context.
 	 */
 	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
 		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
+	else if (dio->flags & IOMAP_DIO_NEED_SYNC)
+		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
 
 	/*
 	 * We are about to drop our additional submission reference, which
-- 
2.47.3

[PATCH 5/5] iomap: invert the polarity of IOMAP_DIO_INLINE_COMP

[Christoph Hellwig]

Replace IOMAP_DIO_INLINE_COMP with a flag to indicate that the
completion should be offloaded.  This removes a tiny bit of boilerplate
code, but more importantly just makes the code easier to follow as this
new flag gets set most of the time and only cleared in one place, while
it was the inverse for the old version.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/iomap/direct-io.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index df313232f422..80ec3ff4e5dd 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -17,7 +17,7 @@
  * iomap.h:
  */
 #define IOMAP_DIO_NO_INVALIDATE	(1U << 26)
-#define IOMAP_DIO_INLINE_COMP	(1U << 27)
+#define IOMAP_DIO_COMP_WORK	(1U << 27)
 #define IOMAP_DIO_WRITE_THROUGH	(1U << 28)
 #define IOMAP_DIO_NEED_SYNC	(1U << 29)
 #define IOMAP_DIO_WRITE		(1U << 30)
@@ -182,7 +182,7 @@ static void iomap_dio_done(struct iomap_dio *dio)
 	 * for error handling.
 	 */
 	if (dio->error)
-		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+		dio->flags |= IOMAP_DIO_COMP_WORK;
 
 	/*
 	 * Never invalidate pages from this context to avoid deadlocks with
@@ -192,17 +192,14 @@ static void iomap_dio_done(struct iomap_dio *dio)
 	 * right between this check and the actual completion.
 	 */
 	if ((dio->flags & IOMAP_DIO_WRITE) &&
-	    (dio->flags & IOMAP_DIO_INLINE_COMP)) {
+	    !(dio->flags & IOMAP_DIO_COMP_WORK)) {
 		if (dio->iocb->ki_filp->f_mapping->nrpages)
-			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+			dio->flags |= IOMAP_DIO_COMP_WORK;
 		else
 			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
 	}
 
-	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
-		WRITE_ONCE(iocb->private, NULL);
-		iomap_dio_complete_work(&dio->aio.work);
-	} else {
+	if (dio->flags & IOMAP_DIO_COMP_WORK) {
 		struct inode *inode = file_inode(iocb->ki_filp);
 
 		/*
@@ -213,7 +210,11 @@ static void iomap_dio_done(struct iomap_dio *dio)
 		 */
 		INIT_WORK(&dio->aio.work, iomap_dio_complete_work);
 		queue_work(inode->i_sb->s_dio_done_wq, &dio->aio.work);
+		return;
 	}
+
+	WRITE_ONCE(iocb->private, NULL);
+	iomap_dio_complete_work(&dio->aio.work);
 }
 
 void iomap_dio_bio_end_io(struct bio *bio)
@@ -251,7 +252,7 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)
 		 * that we are already called from the ioend completion
 		 * workqueue.
 		 */
-		dio->flags |= IOMAP_DIO_INLINE_COMP;
+		dio->flags &= ~IOMAP_DIO_COMP_WORK;
 		iomap_dio_done(dio);
 	}
 
@@ -383,7 +384,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 		 * or extend the file size.
 		 */
 		if (!iomap_dio_is_overwrite(iomap))
-			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+			dio->flags |= IOMAP_DIO_COMP_WORK;
 	} else {
 		bio_opf |= REQ_OP_READ;
 	}
@@ -404,7 +405,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
 	 * ones we set for inline and deferred completions. If none of those
 	 * are available for this IO, clear the polled flag.
 	 */
-	if (!(dio->flags & IOMAP_DIO_INLINE_COMP))
+	if (dio->flags & IOMAP_DIO_COMP_WORK)
 		dio->iocb->ki_flags &= ~IOCB_HIPRI;
 
 	if (need_zeroout) {
@@ -643,12 +644,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	if (dio_flags & IOMAP_DIO_FSBLOCK_ALIGNED)
 		dio->flags |= IOMAP_DIO_FSBLOCK_ALIGNED;
 
-	/*
-	 * Try to complete inline if we can.  For reads this is always possible,
-	 * but for writes we'll end up clearing this more often than not.
-	 */
-	dio->flags |= IOMAP_DIO_INLINE_COMP;
-
 	if (iov_iter_rw(iter) == READ) {
 		if (iomi.pos >= dio->i_size)
 			goto out_free_dio;
@@ -695,7 +690,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 		 * Inode size updates must to happen from process context.
 		 */
 		if (iomi.pos + iomi.len > dio->i_size)
-			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+			dio->flags |= IOMAP_DIO_COMP_WORK;
 
 		/*
 		 * Try to invalidate cache pages for the range we are writing.
@@ -776,7 +771,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
 	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
 		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
 	else if (dio->flags & IOMAP_DIO_NEED_SYNC)
-		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+		dio->flags |= IOMAP_DIO_COMP_WORK;
 
 	/*
 	 * We are about to drop our additional submission reference, which
-- 
2.47.3

Re: enable iomap dio write completions from interrupt context

[Damien Le Moal]

On 11/12/25 16:21, Christoph Hellwig wrote:
> Hi all,
> 
> Currently iomap defers all write completions to interrupt context.  This
> was based on my assumption that no one cares about the latency of those
> to simplify the code vs the old direct-io.c.  It turns out someone cared,
> as Avi reported a lot of context switches with ScyllaDB, which at least
> in older kernels with workqueue scheduling issues caused really high
> tail latencies.
> 
> Fortunately allowing the direct completions is pretty easy with all the
> other iomap changes we had since.
> 
> While doing this I've also found dead code which gets removed (patch 1)
> and an incorrect assumption in zonefs that read completions are called
> in user context, which it assumes for it's error handling.  Fix this by
> always calling error completions from user context (patch 2).
> 
> Against the vfs/vfs-6.19.iomap branch.
> 
> Diffstat:
>  Documentation/filesystems/iomap/operations.rst |    4 
>  fs/backing-file.c                              |    6 -
>  fs/iomap/direct-io.c                           |  149 +++++++++++--------------
>  include/linux/fs.h                             |   43 +------
>  io_uring/rw.c                                  |   16 --
>  5 files changed, 81 insertions(+), 137 deletions(-)

Where is the zonefs change ? Missing a patch ?


-- 
Damien Le Moal
Western Digital Research

Re: enable iomap dio write completions from interrupt context

[Christoph Hellwig]

On Wed, Nov 12, 2025 at 05:43:05PM +0900, Damien Le Moal wrote:
> Where is the zonefs change ? Missing a patch ?

There's not zonefs change.  The zonefs fix is the core code now calling
the read ->end_io handler from workqueue context after an I/O error, and
thus now doing what zonefs incorrectly assumed before.

Re: enable iomap dio write completions from interrupt context

[Damien Le Moal]

On 11/12/25 17:44, Christoph Hellwig wrote:
> On Wed, Nov 12, 2025 at 05:43:05PM +0900, Damien Le Moal wrote:
>> Where is the zonefs change ? Missing a patch ?
> 
> There's not zonefs change.  The zonefs fix is the core code now calling
> the read ->end_io handler from workqueue context after an I/O error, and
> thus now doing what zonefs incorrectly assumed before.

OK. Thanks for the clarification. I was thinking the other way around :)


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/5] fs, iomap: remove IOCB_DIO_CALLER_COMP

[Jan Kara]

On Wed 12-11-25 08:21:25, Christoph Hellwig wrote:
> This was added by commit 099ada2c8726 ("io_uring/rw: add write support
> for IOCB_DIO_CALLER_COMP") and disabled a little later by commit
> 838b35bb6a89 ("io_uring/rw: disable IOCB_DIO_CALLER_COMP") because it
> didn't work.  Remove all the related code that sat unused for 2 years.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I was wondering once where that flag can get set but then got distracted
and forgot about it :). The patch looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  .../filesystems/iomap/operations.rst          |  4 --
>  fs/backing-file.c                             |  6 --
>  fs/iomap/direct-io.c                          | 56 +------------------
>  include/linux/fs.h                            | 43 +++-----------
>  io_uring/rw.c                                 | 16 +-----
>  5 files changed, 13 insertions(+), 112 deletions(-)
> 
> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> index c88205132039..5558a44891bb 100644
> --- a/Documentation/filesystems/iomap/operations.rst
> +++ b/Documentation/filesystems/iomap/operations.rst
> @@ -488,10 +488,6 @@ These ``struct kiocb`` flags are significant for direct I/O with iomap:
>     Only meaningful for asynchronous I/O, and only if the entire I/O can
>     be issued as a single ``struct bio``.
>  
> - * ``IOCB_DIO_CALLER_COMP``: Try to run I/O completion from the caller's
> -   process context.
> -   See ``linux/fs.h`` for more details.
> -
>  Filesystems should call ``iomap_dio_rw`` from ``->read_iter`` and
>  ``->write_iter``, and set ``FMODE_CAN_ODIRECT`` in the ``->open``
>  function for the file.
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index 15a7f8031084..2a86bb6fcd13 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -227,12 +227,6 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
>  	    !(file->f_mode & FMODE_CAN_ODIRECT))
>  		return -EINVAL;
>  
> -	/*
> -	 * Stacked filesystems don't support deferred completions, don't copy
> -	 * this property in case it is set by the issuer.
> -	 */
> -	flags &= ~IOCB_DIO_CALLER_COMP;
> -
>  	old_cred = override_creds(ctx->cred);
>  	if (is_sync_kiocb(iocb)) {
>  		rwf_t rwf = iocb_to_rw_flags(flags);
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 8b2f9fb89eb3..7659db85083a 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -16,8 +16,7 @@
>   * Private flags for iomap_dio, must not overlap with the public ones in
>   * iomap.h:
>   */
> -#define IOMAP_DIO_NO_INVALIDATE	(1U << 25)
> -#define IOMAP_DIO_CALLER_COMP	(1U << 26)
> +#define IOMAP_DIO_NO_INVALIDATE	(1U << 26)
>  #define IOMAP_DIO_INLINE_COMP	(1U << 27)
>  #define IOMAP_DIO_WRITE_THROUGH	(1U << 28)
>  #define IOMAP_DIO_NEED_SYNC	(1U << 29)
> @@ -140,11 +139,6 @@ ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  }
>  EXPORT_SYMBOL_GPL(iomap_dio_complete);
>  
> -static ssize_t iomap_dio_deferred_complete(void *data)
> -{
> -	return iomap_dio_complete(data);
> -}
> -
>  static void iomap_dio_complete_work(struct work_struct *work)
>  {
>  	struct iomap_dio *dio = container_of(work, struct iomap_dio, aio.work);
> @@ -182,29 +176,6 @@ static void iomap_dio_done(struct iomap_dio *dio)
>  	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
>  		WRITE_ONCE(iocb->private, NULL);
>  		iomap_dio_complete_work(&dio->aio.work);
> -	} else if (dio->flags & IOMAP_DIO_CALLER_COMP) {
> -		/*
> -		 * If this dio is flagged with IOMAP_DIO_CALLER_COMP, then
> -		 * schedule our completion that way to avoid an async punt to a
> -		 * workqueue.
> -		 */
> -		/* only polled IO cares about private cleared */
> -		iocb->private = dio;
> -		iocb->dio_complete = iomap_dio_deferred_complete;
> -
> -		/*
> -		 * Invoke ->ki_complete() directly. We've assigned our
> -		 * dio_complete callback handler, and since the issuer set
> -		 * IOCB_DIO_CALLER_COMP, we know their ki_complete handler will
> -		 * notice ->dio_complete being set and will defer calling that
> -		 * handler until it can be done from a safe task context.
> -		 *
> -		 * Note that the 'res' being passed in here is not important
> -		 * for this case. The actual completion value of the request
> -		 * will be gotten from dio_complete when that is run by the
> -		 * issuer.
> -		 */
> -		iocb->ki_complete(iocb, 0);
>  	} else {
>  		struct inode *inode = file_inode(iocb->ki_filp);
>  
> @@ -261,7 +232,6 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)
>  			dio->flags |= IOMAP_DIO_INLINE_COMP;
>  			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
>  		}
> -		dio->flags &= ~IOMAP_DIO_CALLER_COMP;
>  		iomap_dio_done(dio);
>  	}
>  
> @@ -380,19 +350,6 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  
>  		if (!(bio_opf & REQ_FUA))
>  			dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> -
> -		/*
> -		 * We can only do deferred completion for pure overwrites that
> -		 * don't require additional I/O at completion time.
> -		 *
> -		 * This rules out writes that need zeroing or extent conversion,
> -		 * extend the file size, or issue metadata I/O or cache flushes
> -		 * during completion processing.
> -		 */
> -		if (need_zeroout || (pos >= i_size_read(inode)) ||
> -		    ((dio->flags & IOMAP_DIO_NEED_SYNC) &&
> -		     !(bio_opf & REQ_FUA)))
> -			dio->flags &= ~IOMAP_DIO_CALLER_COMP;
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}
> @@ -413,7 +370,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  	 * ones we set for inline and deferred completions. If none of those
>  	 * are available for this IO, clear the polled flag.
>  	 */
> -	if (!(dio->flags & (IOMAP_DIO_INLINE_COMP|IOMAP_DIO_CALLER_COMP)))
> +	if (!(dio->flags & IOMAP_DIO_INLINE_COMP))
>  		dio->iocb->ki_flags &= ~IOCB_HIPRI;
>  
>  	if (need_zeroout) {
> @@ -669,15 +626,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		iomi.flags |= IOMAP_WRITE;
>  		dio->flags |= IOMAP_DIO_WRITE;
>  
> -		/*
> -		 * Flag as supporting deferred completions, if the issuer
> -		 * groks it. This can avoid a workqueue punt for writes.
> -		 * We may later clear this flag if we need to do other IO
> -		 * as part of this IO completion.
> -		 */
> -		if (iocb->ki_flags & IOCB_DIO_CALLER_COMP)
> -			dio->flags |= IOMAP_DIO_CALLER_COMP;
> -
>  		if (dio_flags & IOMAP_DIO_OVERWRITE_ONLY) {
>  			ret = -EAGAIN;
>  			if (iomi.pos >= dio->i_size ||
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index c895146c1444..e210d2d8af53 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -367,23 +367,9 @@ struct readahead_control;
>  #define IOCB_NOIO		(1 << 20)
>  /* can use bio alloc cache */
>  #define IOCB_ALLOC_CACHE	(1 << 21)
> -/*
> - * IOCB_DIO_CALLER_COMP can be set by the iocb owner, to indicate that the
> - * iocb completion can be passed back to the owner for execution from a safe
> - * context rather than needing to be punted through a workqueue. If this
> - * flag is set, the bio completion handling may set iocb->dio_complete to a
> - * handler function and iocb->private to context information for that handler.
> - * The issuer should call the handler with that context information from task
> - * context to complete the processing of the iocb. Note that while this
> - * provides a task context for the dio_complete() callback, it should only be
> - * used on the completion side for non-IO generating completions. It's fine to
> - * call blocking functions from this callback, but they should not wait for
> - * unrelated IO (like cache flushing, new IO generation, etc).
> - */
> -#define IOCB_DIO_CALLER_COMP	(1 << 22)
>  /* kiocb is a read or write operation submitted by fs/aio.c. */
> -#define IOCB_AIO_RW		(1 << 23)
> -#define IOCB_HAS_METADATA	(1 << 24)
> +#define IOCB_AIO_RW		(1 << 22)
> +#define IOCB_HAS_METADATA	(1 << 23)
>  
>  /* for use in trace events */
>  #define TRACE_IOCB_STRINGS \
> @@ -400,7 +386,6 @@ struct readahead_control;
>  	{ IOCB_WAITQ,		"WAITQ" }, \
>  	{ IOCB_NOIO,		"NOIO" }, \
>  	{ IOCB_ALLOC_CACHE,	"ALLOC_CACHE" }, \
> -	{ IOCB_DIO_CALLER_COMP,	"CALLER_COMP" }, \
>  	{ IOCB_AIO_RW,		"AIO_RW" }, \
>  	{ IOCB_HAS_METADATA,	"AIO_HAS_METADATA" }
>  
> @@ -412,23 +397,13 @@ struct kiocb {
>  	int			ki_flags;
>  	u16			ki_ioprio; /* See linux/ioprio.h */
>  	u8			ki_write_stream;
> -	union {
> -		/*
> -		 * Only used for async buffered reads, where it denotes the
> -		 * page waitqueue associated with completing the read. Valid
> -		 * IFF IOCB_WAITQ is set.
> -		 */
> -		struct wait_page_queue	*ki_waitq;
> -		/*
> -		 * Can be used for O_DIRECT IO, where the completion handling
> -		 * is punted back to the issuer of the IO. May only be set
> -		 * if IOCB_DIO_CALLER_COMP is set by the issuer, and the issuer
> -		 * must then check for presence of this handler when ki_complete
> -		 * is invoked. The data passed in to this handler must be
> -		 * assigned to ->private when dio_complete is assigned.
> -		 */
> -		ssize_t (*dio_complete)(void *data);
> -	};
> +
> +	/*
> +	 * Only used for async buffered reads, where it denotes the page
> +	 * waitqueue associated with completing the read.
> +	 * Valid IFF IOCB_WAITQ is set.
> +	 */
> +	struct wait_page_queue	*ki_waitq;
>  };
>  
>  static inline bool is_sync_kiocb(struct kiocb *kiocb)
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 08882648d569..4d0ab8f50d14 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -277,7 +277,6 @@ static int __io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  	} else {
>  		rw->kiocb.ki_ioprio = get_current_ioprio();
>  	}
> -	rw->kiocb.dio_complete = NULL;
>  	rw->kiocb.ki_flags = 0;
>  	rw->kiocb.ki_write_stream = READ_ONCE(sqe->write_stream);
>  
> @@ -566,15 +565,6 @@ static inline int io_fixup_rw_res(struct io_kiocb *req, long res)
>  
>  void io_req_rw_complete(struct io_kiocb *req, io_tw_token_t tw)
>  {
> -	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
> -	struct kiocb *kiocb = &rw->kiocb;
> -
> -	if ((kiocb->ki_flags & IOCB_DIO_CALLER_COMP) && kiocb->dio_complete) {
> -		long res = kiocb->dio_complete(rw->kiocb.private);
> -
> -		io_req_set_res(req, io_fixup_rw_res(req, res), 0);
> -	}
> -
>  	io_req_io_end(req);
>  
>  	if (req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING))
> @@ -589,10 +579,8 @@ static void io_complete_rw(struct kiocb *kiocb, long res)
>  	struct io_rw *rw = container_of(kiocb, struct io_rw, kiocb);
>  	struct io_kiocb *req = cmd_to_io_kiocb(rw);
>  
> -	if (!kiocb->dio_complete || !(kiocb->ki_flags & IOCB_DIO_CALLER_COMP)) {
> -		__io_complete_rw_common(req, res);
> -		io_req_set_res(req, io_fixup_rw_res(req, res), 0);
> -	}
> +	__io_complete_rw_common(req, res);
> +	io_req_set_res(req, io_fixup_rw_res(req, res), 0);
>  	req->io_task_work.func = io_req_rw_complete;
>  	__io_req_task_work_add(req, IOU_F_TWQ_LAZY_WAKE);
>  }
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/5] iomap: always run error completions in user context

[Jan Kara]

On Wed 12-11-25 08:21:26, Christoph Hellwig wrote:
> At least zonefs expects error completions to be able to sleep.  Because
> error completions aren't performance critical, just defer them to workqueue
> context unconditionally.
> 
> Fixes: 8dcc1a9d90c1 ("fs: New zonefs file system")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/iomap/direct-io.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index 7659db85083a..765ab6dd6637 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -173,7 +173,18 @@ static void iomap_dio_done(struct iomap_dio *dio)
>  
>  		WRITE_ONCE(dio->submit.waiter, NULL);
>  		blk_wake_io_task(waiter);
> -	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
> +		return;
> +	}
> +
> +	/*
> +	 * Always run error completions in user context.  These are not
> +	 * performance critical and some code relies on taking sleeping locks
> +	 * for error handling.
> +	 */
> +	if (dio->error)
> +		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +
> +	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
>  		WRITE_ONCE(iocb->private, NULL);
>  		iomap_dio_complete_work(&dio->aio.work);
>  	} else {
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/5] iomap: rework REQ_FUA selection

[Jan Kara]

On Wed 12-11-25 08:21:27, Christoph Hellwig wrote:
> The way how iomap_dio_can_use_fua and the caller is structured is
> a bit confusing, as the main guarding condition is hidden in the
> helper, and the secondary conditions are split between caller and
> callee.
> 
> Refactor the code, so that there is a main IOMAP_DIO_WRITE_THROUGH
> guard in iomap_dio_bio_iter, which is directly tied to clearing it
> when not supported, and a helper that just checks if the I/O is a
> pure overwrite.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

One nit below but feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

> -static inline bool iomap_dio_can_use_fua(const struct iomap *iomap,
> -		struct iomap_dio *dio)
> +static inline bool iomap_dio_is_overwrite(const struct iomap *iomap)
>  {
> -	if (iomap->flags & (IOMAP_F_SHARED | IOMAP_F_DIRTY))
> +	if (iomap->type != IOMAP_MAPPED)
>  		return false;
> -	if (!(dio->flags & IOMAP_DIO_WRITE_THROUGH))
> -		return false;
> -	return !bdev_write_cache(iomap->bdev) || bdev_fua(iomap->bdev);
> +	return !(iomap->flags & (IOMAP_F_NEW | IOMAP_F_SHARED));
>  }

I'm a bit puzzled why did you leave IOMAP_F_DIRTY check in the caller.
Because that means we need metadata update to make the extent stable as the
comment before this function states...

								Honza

>  static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
> @@ -355,12 +349,22 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  
>  		if (iomap->flags & IOMAP_F_NEW)
>  			need_zeroout = true;
> -		else if (iomap->type == IOMAP_MAPPED &&
> -			 iomap_dio_can_use_fua(iomap, dio))
> -			bio_opf |= REQ_FUA;
>  
> -		if (!(bio_opf & REQ_FUA))
> -			dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> +		/*
> +		 * Use a FUA write if we need datasync semantics and this is a
> +		 * pure overwrite that doesn't require any metadata updates.
> +		 *
> +		 * This allows us to avoid cache flushes on I/O completion.
> +		 */
> +		if (dio->flags & IOMAP_DIO_WRITE_THROUGH) {
> +			if (iomap_dio_is_overwrite(iomap) &&
> +			    !(iomap->flags & IOMAP_F_DIRTY) &&
> +			    (!bdev_write_cache(iomap->bdev) ||
> +			     bdev_fua(iomap->bdev)))
> +				bio_opf |= REQ_FUA;
> +			else
> +				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> +		}
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Jan Kara]

On Wed 12-11-25 08:21:28, Christoph Hellwig wrote:
> Completions for pure overwrites don't need to be deferred to a workqueue
> as there is no work to be done, or at least no work that needs a user
> context.  Set the IOMAP_DIO_INLINE_COMP by default for writes like we
> already do for reads, and the clear it for all the cases that actually
> do need a user context for completions to update the inode size or
> record updates to the logical to physical mapping.
> 
> I've audited all users of the ->end_io callback, and they only require
> user context for I/O that involves unwritten extents, COW, size
> extensions, or error handling and all those are still run from workqueue
> context.
> 
> This restores the behavior of the old pre-iomap direct I/O code.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
...
> @@ -365,6 +374,16 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  			else
>  				dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
>  		}
> +
> +		/*
> +		 * We can only do inline completion for pure overwrites that
> +		 * don't require additional I/O at completion time.
> +		 *
> +		 * This rules out writes that need zeroing or extent conversion,
> +		 * or extend the file size.
> +		 */
> +		if (!iomap_dio_is_overwrite(iomap))
> +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}

OK, now I see why you wrote iomap_dio_is_overwrite() the way you did. You
still want to keep completions inline for overwrites of possibly
uncommitted extents. But I have to admit it all seems somewhat fragile and
difficult to follow. Can't we just check for IOMAP_DIO_UNWRITTEN |
IOMAP_DIO_COW | IOMAP_DIO_NEED_SYNC in flags (plus the i_size check) and be
done with it?

								Honza

> @@ -669,6 +691,12 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  				dio->flags |= IOMAP_DIO_WRITE_THROUGH;
>  		}
>  
> +		/*
> +		 * Inode size updates must to happen from process context.
> +		 */
> +		if (iomi.pos + iomi.len > dio->i_size)
> +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +
>  		/*
>  		 * Try to invalidate cache pages for the range we are writing.
>  		 * If this invalidation fails, let the caller fall back to
> @@ -741,9 +769,14 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	 * If all the writes we issued were already written through to the
>  	 * media, we don't need to flush the cache on IO completion. Clear the
>  	 * sync flag for this case.
> +	 *
> +	 * Otherwise clear the inline completion flag if any sync work is
> +	 * needed, as that needs to be performed from process context.
>  	 */
>  	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
>  		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
> +	else if (dio->flags & IOMAP_DIO_NEED_SYNC)
> +		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
>  
>  	/*
>  	 * We are about to drop our additional submission reference, which
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/5] fs, iomap: remove IOCB_DIO_CALLER_COMP

[Chaitanya Kulkarni]

On 11/11/25 23:21, Christoph Hellwig wrote:
> This was added by commit 099ada2c8726 ("io_uring/rw: add write support
> for IOCB_DIO_CALLER_COMP") and disabled a little later by commit
> 838b35bb6a89 ("io_uring/rw: disable IOCB_DIO_CALLER_COMP") because it
> didn't work.  Remove all the related code that sat unused for 2 years.
>
> Signed-off-by: Christoph Hellwig<hch@lst.de>
> ---

**Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 2/5] iomap: always run error completions in user context

[Chaitanya Kulkarni]

On 11/11/25 23:21, Christoph Hellwig wrote:
> At least zonefs expects error completions to be able to sleep.  Because
> error completions aren't performance critical, just defer them to workqueue
> context unconditionally.
>
> Fixes: 8dcc1a9d90c1 ("fs: New zonefs file system")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Christoph Hellwig]

On Wed, Nov 12, 2025 at 09:25:58PM +0100, Jan Kara wrote:
> > +
> > +		/*
> > +		 * We can only do inline completion for pure overwrites that
> > +		 * don't require additional I/O at completion time.
> > +		 *
> > +		 * This rules out writes that need zeroing or extent conversion,
> > +		 * or extend the file size.
> > +		 */
> > +		if (!iomap_dio_is_overwrite(iomap))
> > +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> >  	} else {
> >  		bio_opf |= REQ_OP_READ;
> >  	}
> 
> OK, now I see why you wrote iomap_dio_is_overwrite() the way you did. You
> still want to keep completions inline for overwrites of possibly
> uncommitted extents.

Yes.

> But I have to admit it all seems somewhat fragile and
> difficult to follow. Can't we just check for IOMAP_DIO_UNWRITTEN |
> IOMAP_DIO_COW | IOMAP_DIO_NEED_SYNC in flags (plus the i_size check) and be
> done with it?

You mean drop the common helper?  How would that be better and less
fragile?   Note that I care strongly, but I don't really see the point.

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Jan Kara]

On Thu 13-11-25 07:50:55, Christoph Hellwig wrote:
> On Wed, Nov 12, 2025 at 09:25:58PM +0100, Jan Kara wrote:
> > > +
> > > +		/*
> > > +		 * We can only do inline completion for pure overwrites that
> > > +		 * don't require additional I/O at completion time.
> > > +		 *
> > > +		 * This rules out writes that need zeroing or extent conversion,
> > > +		 * or extend the file size.
> > > +		 */
> > > +		if (!iomap_dio_is_overwrite(iomap))
> > > +			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> > >  	} else {
> > >  		bio_opf |= REQ_OP_READ;
> > >  	}
> > 
> > OK, now I see why you wrote iomap_dio_is_overwrite() the way you did. You
> > still want to keep completions inline for overwrites of possibly
> > uncommitted extents.
> 
> Yes.
> 
> > But I have to admit it all seems somewhat fragile and
> > difficult to follow. Can't we just check for IOMAP_DIO_UNWRITTEN |
> > IOMAP_DIO_COW | IOMAP_DIO_NEED_SYNC in flags (plus the i_size check) and be
> > done with it?
> 
> You mean drop the common helper?  How would that be better and less
> fragile?   Note that I care strongly, but I don't really see the point.

Sorry I was a bit terse. What I meant is that the two users of
iomap_dio_is_overwrite() actually care about different things and that
results in that function having a bit odd semantics IMHO. The first user
wants to figure out whether calling generic_write_sync() is needed upon io
completion to make data persistent (crash safe). The second user cares
whether we need to do metadata modifications upon io completion to make data
visible at all.

So it would be IMO more understandable if we had one helper like
iomap_dio_extent_stable() for the first case (which would be the same as
current iomap_dio_is_overwrite() + IOMAP_F_DIRTY) and for the second case
we already gather that information from extents in DIO flags and
filesystems do the non-trivial work in their end_io handlers based on these
flags so just checking IOMAP_DIO_UNWRITTEN | IOMAP_DIO_COW |
IOMAP_DIO_NEED_SYNC in iomap_dio_complete() should be a reliable trigger
for offloading to a workqueue (plus the check for dio->error). That way the
handling of IOMAP_DIO_INLINE_COMP happens only before we start IO (i_size
check) and in iomap_dio_complete() which seems easier to follow to me.

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

Re: enable iomap dio write completions from interrupt context

[Jan Kara]

On Wed 12-11-25 08:21:24, Christoph Hellwig wrote:
> While doing this I've also found dead code which gets removed (patch 1)
> and an incorrect assumption in zonefs that read completions are called
> in user context, which it assumes for it's error handling.  Fix this by
> always calling error completions from user context (patch 2).

Speaking of zonefs, I how is the unconditional locking of
zi->i_truncate_mutex in zonefs_file_write_dio_end_io() compatible with
inline completions?

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

Re: enable iomap dio write completions from interrupt context

[Christoph Hellwig]

On Thu, Nov 13, 2025 at 10:58:00AM +0100, Jan Kara wrote:
> On Wed 12-11-25 08:21:24, Christoph Hellwig wrote:
> > While doing this I've also found dead code which gets removed (patch 1)
> > and an incorrect assumption in zonefs that read completions are called
> > in user context, which it assumes for it's error handling.  Fix this by
> > always calling error completions from user context (patch 2).
> 
> Speaking of zonefs, I how is the unconditional locking of
> zi->i_truncate_mutex in zonefs_file_write_dio_end_io() compatible with
> inline completions?

It wouldn't, but zonefs doesn't use write inline completions because
it marks all I/O to sequential zones as unwritten.

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Christoph Hellwig]

On Thu, Nov 13, 2025 at 10:54:46AM +0100, Jan Kara wrote:
> > You mean drop the common helper?  How would that be better and less
> > fragile?   Note that I care strongly, but I don't really see the point.
> 
> Sorry I was a bit terse. What I meant is that the two users of
> iomap_dio_is_overwrite() actually care about different things and that
> results in that function having a bit odd semantics IMHO. The first user
> wants to figure out whether calling generic_write_sync() is needed upon io
> completion to make data persistent (crash safe).

Yes.

> The second user cares
> whether we need to do metadata modifications upon io completion to make data
> visible at all.

Not quite.  It cares if either generic_write_sync needs be called,
or we need a metadata modification, because both require the workqueue.

Re: enable iomap dio write completions from interrupt context

[Damien Le Moal]

On 11/13/25 19:05, Christoph Hellwig wrote:
> On Thu, Nov 13, 2025 at 10:58:00AM +0100, Jan Kara wrote:
>> On Wed 12-11-25 08:21:24, Christoph Hellwig wrote:
>>> While doing this I've also found dead code which gets removed (patch 1)
>>> and an incorrect assumption in zonefs that read completions are called
>>> in user context, which it assumes for it's error handling.  Fix this by
>>> always calling error completions from user context (patch 2).
>>
>> Speaking of zonefs, I how is the unconditional locking of
>> zi->i_truncate_mutex in zonefs_file_write_dio_end_io() compatible with
>> inline completions?
> 
> It wouldn't, but zonefs doesn't use write inline completions because
> it marks all I/O to sequential zones as unwritten.

Yes, append writes only. Probably need to add a comment to clarify that to make
sure there is no mistake about the completion context.

-- 
Damien Le Moal
Western Digital Research

Re: enable iomap dio write completions from interrupt context

[Jan Kara]

On Thu 13-11-25 11:05:27, Christoph Hellwig wrote:
> On Thu, Nov 13, 2025 at 10:58:00AM +0100, Jan Kara wrote:
> > On Wed 12-11-25 08:21:24, Christoph Hellwig wrote:
> > > While doing this I've also found dead code which gets removed (patch 1)
> > > and an incorrect assumption in zonefs that read completions are called
> > > in user context, which it assumes for it's error handling.  Fix this by
> > > always calling error completions from user context (patch 2).
> > 
> > Speaking of zonefs, I how is the unconditional locking of
> > zi->i_truncate_mutex in zonefs_file_write_dio_end_io() compatible with
> > inline completions?
> 
> It wouldn't, but zonefs doesn't use write inline completions because
> it marks all I/O to sequential zones as unwritten.

Ah, now I see that. Thanks for explanation.

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

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Jan Kara]

On Thu 13-11-25 11:06:30, Christoph Hellwig wrote:
> On Thu, Nov 13, 2025 at 10:54:46AM +0100, Jan Kara wrote:
> > > You mean drop the common helper?  How would that be better and less
> > > fragile?   Note that I care strongly, but I don't really see the point.
> > 
> > Sorry I was a bit terse. What I meant is that the two users of
> > iomap_dio_is_overwrite() actually care about different things and that
> > results in that function having a bit odd semantics IMHO. The first user
> > wants to figure out whether calling generic_write_sync() is needed upon io
> > completion to make data persistent (crash safe).
> 
> Yes.
> 
> > The second user cares
> > whether we need to do metadata modifications upon io completion to make data
> > visible at all.
> 
> Not quite.  It cares if either generic_write_sync needs be called,
> or we need a metadata modification, because both require the workqueue.

I agree but generic_write_sync() calling is handled by 

+       else if (dio->flags & IOMAP_DIO_NEED_SYNC)
+               dio->flags &= ~IOMAP_DIO_INLINE_COMP;

in your patch. So I assumed (maybe wrongly) that the second call to
iomap_dio_is_overwrite() in iomap_dio_bio_iter() is only about detecting a
need of metadata modification. And my argument is that the patch could use
IOMAP_DIO_UNWRITTEN | IOMAP_DIO_COW the same way as it uses
IOMAP_DIO_NEED_SYNC instead of calling iomap_dio_is_overwrite().

But if you don't like that I don't think it makes a huge difference and the
code is correct as is so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

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

Re: [PATCH 5/5] iomap: invert the polarity of IOMAP_DIO_INLINE_COMP

[Jan Kara]

On Wed 12-11-25 08:21:29, Christoph Hellwig wrote:
> Replace IOMAP_DIO_INLINE_COMP with a flag to indicate that the
> completion should be offloaded.  This removes a tiny bit of boilerplate
> code, but more importantly just makes the code easier to follow as this
> new flag gets set most of the time and only cleared in one place, while
> it was the inverse for the old version.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/iomap/direct-io.c | 33 ++++++++++++++-------------------
>  1 file changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index df313232f422..80ec3ff4e5dd 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -17,7 +17,7 @@
>   * iomap.h:
>   */
>  #define IOMAP_DIO_NO_INVALIDATE	(1U << 26)
> -#define IOMAP_DIO_INLINE_COMP	(1U << 27)
> +#define IOMAP_DIO_COMP_WORK	(1U << 27)
>  #define IOMAP_DIO_WRITE_THROUGH	(1U << 28)
>  #define IOMAP_DIO_NEED_SYNC	(1U << 29)
>  #define IOMAP_DIO_WRITE		(1U << 30)
> @@ -182,7 +182,7 @@ static void iomap_dio_done(struct iomap_dio *dio)
>  	 * for error handling.
>  	 */
>  	if (dio->error)
> -		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +		dio->flags |= IOMAP_DIO_COMP_WORK;
>  
>  	/*
>  	 * Never invalidate pages from this context to avoid deadlocks with
> @@ -192,17 +192,14 @@ static void iomap_dio_done(struct iomap_dio *dio)
>  	 * right between this check and the actual completion.
>  	 */
>  	if ((dio->flags & IOMAP_DIO_WRITE) &&
> -	    (dio->flags & IOMAP_DIO_INLINE_COMP)) {
> +	    !(dio->flags & IOMAP_DIO_COMP_WORK)) {
>  		if (dio->iocb->ki_filp->f_mapping->nrpages)
> -			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +			dio->flags |= IOMAP_DIO_COMP_WORK;
>  		else
>  			dio->flags |= IOMAP_DIO_NO_INVALIDATE;
>  	}
>  
> -	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
> -		WRITE_ONCE(iocb->private, NULL);
> -		iomap_dio_complete_work(&dio->aio.work);
> -	} else {
> +	if (dio->flags & IOMAP_DIO_COMP_WORK) {
>  		struct inode *inode = file_inode(iocb->ki_filp);
>  
>  		/*
> @@ -213,7 +210,11 @@ static void iomap_dio_done(struct iomap_dio *dio)
>  		 */
>  		INIT_WORK(&dio->aio.work, iomap_dio_complete_work);
>  		queue_work(inode->i_sb->s_dio_done_wq, &dio->aio.work);
> +		return;
>  	}
> +
> +	WRITE_ONCE(iocb->private, NULL);
> +	iomap_dio_complete_work(&dio->aio.work);
>  }
>  
>  void iomap_dio_bio_end_io(struct bio *bio)
> @@ -251,7 +252,7 @@ u32 iomap_finish_ioend_direct(struct iomap_ioend *ioend)
>  		 * that we are already called from the ioend completion
>  		 * workqueue.
>  		 */
> -		dio->flags |= IOMAP_DIO_INLINE_COMP;
> +		dio->flags &= ~IOMAP_DIO_COMP_WORK;
>  		iomap_dio_done(dio);
>  	}
>  
> @@ -383,7 +384,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  		 * or extend the file size.
>  		 */
>  		if (!iomap_dio_is_overwrite(iomap))
> -			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +			dio->flags |= IOMAP_DIO_COMP_WORK;
>  	} else {
>  		bio_opf |= REQ_OP_READ;
>  	}
> @@ -404,7 +405,7 @@ static int iomap_dio_bio_iter(struct iomap_iter *iter, struct iomap_dio *dio)
>  	 * ones we set for inline and deferred completions. If none of those
>  	 * are available for this IO, clear the polled flag.
>  	 */
> -	if (!(dio->flags & IOMAP_DIO_INLINE_COMP))
> +	if (dio->flags & IOMAP_DIO_COMP_WORK)
>  		dio->iocb->ki_flags &= ~IOCB_HIPRI;
>  
>  	if (need_zeroout) {
> @@ -643,12 +644,6 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	if (dio_flags & IOMAP_DIO_FSBLOCK_ALIGNED)
>  		dio->flags |= IOMAP_DIO_FSBLOCK_ALIGNED;
>  
> -	/*
> -	 * Try to complete inline if we can.  For reads this is always possible,
> -	 * but for writes we'll end up clearing this more often than not.
> -	 */
> -	dio->flags |= IOMAP_DIO_INLINE_COMP;
> -
>  	if (iov_iter_rw(iter) == READ) {
>  		if (iomi.pos >= dio->i_size)
>  			goto out_free_dio;
> @@ -695,7 +690,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  		 * Inode size updates must to happen from process context.
>  		 */
>  		if (iomi.pos + iomi.len > dio->i_size)
> -			dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +			dio->flags |= IOMAP_DIO_COMP_WORK;
>  
>  		/*
>  		 * Try to invalidate cache pages for the range we are writing.
> @@ -776,7 +771,7 @@ __iomap_dio_rw(struct kiocb *iocb, struct iov_iter *iter,
>  	if (dio->flags & IOMAP_DIO_WRITE_THROUGH)
>  		dio->flags &= ~IOMAP_DIO_NEED_SYNC;
>  	else if (dio->flags & IOMAP_DIO_NEED_SYNC)
> -		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> +		dio->flags |= IOMAP_DIO_COMP_WORK;
>  
>  	/*
>  	 * We are about to drop our additional submission reference, which
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 4/5] iomap: support write completions from interrupt context

[Christoph Hellwig]

On Thu, Nov 13, 2025 at 01:06:34PM +0100, Jan Kara wrote:
> On Thu 13-11-25 11:06:30, Christoph Hellwig wrote:
> > On Thu, Nov 13, 2025 at 10:54:46AM +0100, Jan Kara wrote:
> > > > You mean drop the common helper?  How would that be better and less
> > > > fragile?   Note that I care strongly, but I don't really see the point.
> > > 
> > > Sorry I was a bit terse. What I meant is that the two users of
> > > iomap_dio_is_overwrite() actually care about different things and that
> > > results in that function having a bit odd semantics IMHO. The first user
> > > wants to figure out whether calling generic_write_sync() is needed upon io
> > > completion to make data persistent (crash safe).
> > 
> > Yes.
> > 
> > > The second user cares
> > > whether we need to do metadata modifications upon io completion to make data
> > > visible at all.
> > 
> > Not quite.  It cares if either generic_write_sync needs be called,
> > or we need a metadata modification, because both require the workqueue.
> 
> I agree but generic_write_sync() calling is handled by 
> 
> +       else if (dio->flags & IOMAP_DIO_NEED_SYNC)
> +               dio->flags &= ~IOMAP_DIO_INLINE_COMP;
> 
> in your patch. So I assumed (maybe wrongly) that the second call to
> iomap_dio_is_overwrite() in iomap_dio_bio_iter() is only about detecting a
> need of metadata modification. And my argument is that the patch could use
> IOMAP_DIO_UNWRITTEN | IOMAP_DIO_COW the same way as it uses
> IOMAP_DIO_NEED_SYNC instead of calling iomap_dio_is_overwrite().
> 
> But if you don't like that I don't think it makes a huge difference and the
> code is correct as is so feel free to add:

I'll take a look if there is a way to clear thing up a bit.

[PATCH 2/5] iomap: always run error completions in user context

[Christoph Hellwig]

At least zonefs expects error completions to be able to sleep.  Because
error completions aren't performance critical, just defer them to workqueue
context unconditionally.

Fixes: 8dcc1a9d90c1 ("fs: New zonefs file system")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 fs/iomap/direct-io.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index 7659db85083a..765ab6dd6637 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -173,7 +173,18 @@ static void iomap_dio_done(struct iomap_dio *dio)
 
 		WRITE_ONCE(dio->submit.waiter, NULL);
 		blk_wake_io_task(waiter);
-	} else if (dio->flags & IOMAP_DIO_INLINE_COMP) {
+		return;
+	}
+
+	/*
+	 * Always run error completions in user context.  These are not
+	 * performance critical and some code relies on taking sleeping locks
+	 * for error handling.
+	 */
+	if (dio->error)
+		dio->flags &= ~IOMAP_DIO_INLINE_COMP;
+
+	if (dio->flags & IOMAP_DIO_INLINE_COMP) {
 		WRITE_ONCE(iocb->private, NULL);
 		iomap_dio_complete_work(&dio->aio.work);
 	} else {
-- 
2.47.3