Add support for async buffered reads

Use the async page locking infrastructure, if IOCB_WAITQ is set in the
passed in iocb. The caller must expect an -EIOCBQUEUED return value, which means that IO is started but not done yet. This is similar to how
O_DIRECT signals the same operation. Once the callback is received by the caller for IO completion, the caller must retry the operation.

Signed-off-by Jens Axboe <[email protected]>

---
 mm/filemap.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

mm/filemap.c +8 -0

index c746541b1d49..a3b86c9acdc8 100644
@@ -1219,6 +1219,14 @@ static int __wait_on_page_locked_async(struct
 page *page,
 	return ret;
 }
 
+static int wait_on_page_locked_async(struct page *page,
+				     struct wait_page_queue *wait)
+{
+	if (!PageLocked(page))
+		return 0;
+	return __wait_on_page_locked_async(compound_head(page), wait,
 false);
+}
+
 /**

* put_and_wait_on_page_locked - Drop a reference and wait for it to
be unlocked
* @page: The page to wait for.

mm/filemap.c +0 -5

@@ -2058,17 +2066,25 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					index, last_index - index);
 		}
 		if (!PageUptodate(page)) {
-			if (iocb->ki_flags & IOCB_NOWAIT) {
-				put_page(page);
-				goto would_block;
-			}
-
 			/*
 			 * See comment in do_read_cache_page on why
 			 * wait_on_page_locked is used to avoid

unnecessarily

mm/filemap.c +7 -1

 			 * serialisations and why it's safe.
 			 */
-			error = wait_on_page_locked_killable(page);
+			if (iocb->ki_flags & IOCB_WAITQ) {
+				if (written) {
+					put_page(page);
+					goto out;
+				}
+				error = wait_on_page_locked_async(page,
+								iocb-

Quoted 1 line

private);

mm/filemap.c +6 -0

+			} else {
+				if (iocb->ki_flags & IOCB_NOWAIT) {
+					put_page(page);
+					goto would_block;
+				}
+				error =

wait_on_page_locked_killable(page);

mm/filemap.c +5 -1

+			}
 			if (unlikely(error))
 				goto readpage_error;
 			if (PageUptodate(page))

@@ -2156,7 +2172,10 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 
 page_not_up_to_date:
 		/* Get exclusive access to the page ... */
-		error = lock_page_killable(page);
+		if (iocb->ki_flags & IOCB_WAITQ)
+			error = lock_page_async(page, iocb->private);
+		else
+			error = lock_page_killable(page);
 		if (unlikely(error))
 			goto readpage_error;

Quoted 59 lines · Jens Axboe

Use the async page locking infrastructure, if IOCB_WAITQ is set in the
passed in iocb. The caller must expect an -EIOCBQUEUED return value, which means that IO is started but not done yet. This is similar to how
O_DIRECT signals the same operation. Once the callback is received by the caller for IO completion, the caller must retry the operation.

Signed-off-by Jens Axboe <[email protected]>

---
 mm/filemap.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

mm/filemap.c +8 -0

index c746541b1d49..a3b86c9acdc8 100644
@@ -1219,6 +1219,14 @@ static int __wait_on_page_locked_async(struct
 page *page,
 	return ret;
 }
 
+static int wait_on_page_locked_async(struct page *page,
+				     struct wait_page_queue *wait)
+{
+	if (!PageLocked(page))
+		return 0;
+	return __wait_on_page_locked_async(compound_head(page), wait,
 false);
+}
+
 /**

* put_and_wait_on_page_locked - Drop a reference and wait for it to
be unlocked
* @page: The page to wait for.

mm/filemap.c +0 -5

@@ -2058,17 +2066,25 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					index, last_index - index);
 		}
 		if (!PageUptodate(page)) {
-			if (iocb->ki_flags & IOCB_NOWAIT) {
-				put_page(page);
-				goto would_block;
-			}
-
 			/*
 			 * See comment in do_read_cache_page on why
 			 * wait_on_page_locked is used to avoid

unnecessarily

mm/filemap.c +7 -1

 			 * serialisations and why it's safe.
 			 */
-			error = wait_on_page_locked_killable(page);
+			if (iocb->ki_flags & IOCB_WAITQ) {
+				if (written) {
+					put_page(page);
+					goto out;
+				}
+				error = wait_on_page_locked_async(page,
+								iocb-

Quoted 1 line

Quoted 1 line

private);

If it is being used in 'generic_file_buffered_read()' as storage for a wait queue, then it is hard to consider this a 'private' field.

Perhaps either rename and add type checking, or else add a separate field altogether to struct kiocb?

Quoted 60 lines · Trond Myklebust

Quoted 59 lines · Jens Axboe

Use the async page locking infrastructure, if IOCB_WAITQ is set in the
passed in iocb. The caller must expect an -EIOCBQUEUED return value, which means that IO is started but not done yet. This is similar to how
O_DIRECT signals the same operation. Once the callback is received by the caller for IO completion, the caller must retry the operation.

Signed-off-by Jens Axboe <[email protected]>

---
 mm/filemap.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

mm/filemap.c +8 -0

index c746541b1d49..a3b86c9acdc8 100644
@@ -1219,6 +1219,14 @@ static int __wait_on_page_locked_async(struct
 page *page,
 	return ret;
 }
 
+static int wait_on_page_locked_async(struct page *page,
+				     struct wait_page_queue *wait)
+{
+	if (!PageLocked(page))
+		return 0;
+	return __wait_on_page_locked_async(compound_head(page), wait,
 false);
+}
+
 /**

* put_and_wait_on_page_locked - Drop a reference and wait for it to
be unlocked
* @page: The page to wait for.

mm/filemap.c +0 -5

@@ -2058,17 +2066,25 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					index, last_index - index);
 		}
 		if (!PageUptodate(page)) {
-			if (iocb->ki_flags & IOCB_NOWAIT) {
-				put_page(page);
-				goto would_block;
-			}
-
 			/*
 			 * See comment in do_read_cache_page on why
 			 * wait_on_page_locked is used to avoid

unnecessarily

mm/filemap.c +7 -1

 			 * serialisations and why it's safe.
 			 */
-			error = wait_on_page_locked_killable(page);
+			if (iocb->ki_flags & IOCB_WAITQ) {
+				if (written) {
+					put_page(page);
+					goto out;
+				}
+				error = wait_on_page_locked_async(page,
+								iocb-

Quoted 4 lines

Quoted 1 line

Quoted 1 line

private);

If it is being used in 'generic_file_buffered_read()' as storage for a wait queue, then it is hard to consider this a 'private' field.

private isn't the prettiest, and in fact this one in particular is a bit of a mess. It's not clear if it's caller or callee owned. It's generally not used, outside of the old usb gadget code, iomap O_DIRECT, and ocfs2. With FMODE_BUF_RASYNC, the fs obviously can't set it if it uses ->private for buffered IO.

Quoted 2 lines

Perhaps either rename and add type checking, or else add a separate field altogether to struct kiocb?

I'd hate to add a new field and increase the size of the kiocb... One alternative is to do:

	union {
		void *private;
		struct wait_page_queue *ki_waitq;
	};

and still use IOCB_WAITQ to say that ->ki_waitq is valid.

There's also 4 bytes of padding in the kiocb struct. And some fields are only used for O_DIRECT as well, eg ->ki_cookie which is just used for polled O_DIRECT. So we could also do:

	union {
		unsigned int ki_cookie;
		struct wait_page_queue *ki_waitq;
	};

and still not grow the kiocb. How about we go with this approach, and also add:

	if (kiocb->ki_flags & IOCB_HIPRI)
		return -EOPNOTSUPP;

to kiocb_wait_page_queue_init() to make sure that this combination isn't valid?

Quoted 120 lines · Jens Axboe

Quoted 74 lines · Trond Myklebust

Quoted 73 lines · Jens Axboe

Use the async page locking infrastructure, if IOCB_WAITQ is set in
the
passed in iocb. The caller must expect an -EIOCBQUEUED return value,
which means that IO is started but not done yet. This is similar to
how
O_DIRECT signals the same operation. Once the callback is received by
the caller for IO completion, the caller must retry the operation.

Signed-off-by Jens Axboe <[email protected]>

---
 mm/filemap.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

mm/filemap.c +0 -0

index c746541b1d49..a3b86c9acdc8 100644
@@ -1219,6 +1219,14 @@ static int

__wait_on_page_locked_async(struct
page *page,

mm/filemap.c +2 -0

 	return ret;
 }
 
+static int wait_on_page_locked_async(struct page *page,
+				     struct wait_page_queue

*wait)

mm/filemap.c +4 -0

+{
+	if (!PageLocked(page))
+		return 0;
+	return __wait_on_page_locked_async(compound_head(page),

wait,
false);

mm/filemap.c +2 -0

+}
+
 /**
  * put_and_wait_on_page_locked - Drop a reference and wait for

it to
be unlocked
* @page: The page to wait for.

mm/filemap.c +0 -5

@@ -2058,17 +2066,25 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					index, last_index -
 index);
 		}
 		if (!PageUptodate(page)) {
-			if (iocb->ki_flags & IOCB_NOWAIT) {
-				put_page(page);
-				goto would_block;
-			}
-
 			/*
 			 * See comment in do_read_cache_page on

why
* wait_on_page_locked is used to avoid
unnecessarily

mm/filemap.c +0 -1

 			 * serialisations and why it's safe.
 			 */
-			error =

wait_on_page_locked_killable(page);

mm/filemap.c +6 -0

+			if (iocb->ki_flags & IOCB_WAITQ) {
+				if (written) {
+					put_page(page);
+					goto out;
+				}
+				error =

wait_on_page_locked_async(page,

mm/filemap.c +1 -0

+								

iocb-

Quoted 5 lines

Quoted 1 line

Quoted 1 line

private);

If it is being used in 'generic_file_buffered_read()' as storage for a
wait queue, then it is hard to consider this a 'private' field.

private isn't the prettiest, and in fact this one in particular is a bit
of a mess. It's not clear if it's caller or callee owned. It's generally
not used, outside of the old usb gadget code, iomap O_DIRECT, and ocfs2.
With FMODE_BUF_RASYNC, the fs obviously can't set it if it uses ->private for buffered IO.

Quoted 3 lines

Perhaps either rename and add type checking, or else add a separate
field altogether to struct kiocb?

I'd hate to add a new field and increase the size of the kiocb... One
alternative is to do:

	union {
		void *private;
		struct wait_page_queue *ki_waitq;
	};

and still use IOCB_WAITQ to say that ->ki_waitq is valid.

There's also 4 bytes of padding in the kiocb struct. And some fields are
only used for O_DIRECT as well, eg ->ki_cookie which is just used for
polled O_DIRECT. So we could also do:

	union {
		unsigned int ki_cookie;
		struct wait_page_queue *ki_waitq;
	};

and still not grow the kiocb. How about we go with this approach, and
also add:

Quoted 6 lines

	if (kiocb->ki_flags & IOCB_HIPRI)
		return -EOPNOTSUPP;

to kiocb_wait_page_queue_init() to make sure that this combination isn't
valid?

Here's the incremental, which is spread over 3 patches. I think this one
makes sense, as polled IO doesn't support buffered IO. And because doing
an async callback for completion is not how polled IO operates anyway,
so even if buffered IO supported it, we'd not use the callback for polled IO anyway. kiocb_wait_page_queue_init() checks and backs this up.

include/linux/fs.h +4 -2

index 0ef5f5973b1c..f7b1eb765c6e 100644
@@ -317,7 +317,7 @@ enum rw_hint {
 #define IOCB_SYNC		(1 << 5)
 #define IOCB_WRITE		(1 << 6)
 #define IOCB_NOWAIT		(1 << 7)
-/* iocb->private holds wait_page_async struct */
+/* iocb->ki_waitq is valid */
 #define IOCB_WAITQ		(1 << 8)
 
 struct kiocb {

@@ -332,7 +332,10 @@ struct kiocb {
 	int			ki_flags;
 	u16			ki_hint;
 	u16			ki_ioprio; /* See linux/ioprio.h */
-	unsigned int		ki_cookie; /* for ->iopoll */
+	union {
+		unsigned int		ki_cookie; /* for ->iopoll */
+		struct wait_page_queue	*ki_waitq; /* for async

buffered IO */

include/linux/fs.h +1 -0

+	};
 
 	randomized_struct_fields_end
 };

include/linux/pagemap.h +4 -1

index def58de92053..8b65420410ee 100644
@@ -498,13 +498,16 @@ static inline int
 kiocb_wait_page_queue_init(struct kiocb *kiocb,
 					     wait_queue_func_t func,
 					     void *data)
 {
+	/* Can't support async wakeup with polled IO */
+	if (kiocb->ki_flags & IOCB_HIPRI)
+		return -EINVAL;
 	if (kiocb->ki_filp->f_mode & FMODE_BUF_RASYNC) {
 		wait->wait.func = func;
 		wait->wait.private = data;
 		wait->wait.flags = 0;
 		INIT_LIST_HEAD(&wait->wait.entry);
 		kiocb->ki_flags |= IOCB_WAITQ;
-		kiocb->private = wait;
+		kiocb->ki_waitq = wait;
 		return 0;
 	}

mm/filemap.c +1 -1

index a3b86c9acdc8..18022de7dc33 100644
@@ -2077,7 +2077,7 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					goto out;
 				}
 				error = wait_on_page_locked_async(page,
-								iocb-
 >private);
+								iocb-
 >ki_waitq);

mm/filemap.c +1 -1

 			} else {
 				if (iocb->ki_flags & IOCB_NOWAIT) {
 					put_page(page);

@@ -2173,7 +2173,7 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 page_not_up_to_date:
 		/* Get exclusive access to the page ... */
 		if (iocb->ki_flags & IOCB_WAITQ)
-			error = lock_page_async(page, iocb->private);
+			error = lock_page_async(page, iocb->ki_waitq);
 		else
 			error = lock_page_killable(page);

if (unlikely(error))

Quoted 201 lines · Jens Axboe

Quoted 120 lines · Jens Axboe

Quoted 74 lines · Trond Myklebust

Quoted 73 lines · Jens Axboe

Use the async page locking infrastructure, if IOCB_WAITQ is set in
the
passed in iocb. The caller must expect an -EIOCBQUEUED return value,
which means that IO is started but not done yet. This is similar to
how
O_DIRECT signals the same operation. Once the callback is received by
the caller for IO completion, the caller must retry the operation.

Signed-off-by Jens Axboe <[email protected]>

---
 mm/filemap.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

mm/filemap.c +0 -0

index c746541b1d49..a3b86c9acdc8 100644
@@ -1219,6 +1219,14 @@ static int

__wait_on_page_locked_async(struct
page *page,

mm/filemap.c +2 -0

 	return ret;
 }
 
+static int wait_on_page_locked_async(struct page *page,
+				     struct wait_page_queue

*wait)

mm/filemap.c +4 -0

+{
+	if (!PageLocked(page))
+		return 0;
+	return __wait_on_page_locked_async(compound_head(page),

wait,
false);

mm/filemap.c +2 -0

+}
+
 /**
  * put_and_wait_on_page_locked - Drop a reference and wait for

it to
be unlocked
* @page: The page to wait for.

mm/filemap.c +0 -5

@@ -2058,17 +2066,25 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					index, last_index -
 index);
 		}
 		if (!PageUptodate(page)) {
-			if (iocb->ki_flags & IOCB_NOWAIT) {
-				put_page(page);
-				goto would_block;
-			}
-
 			/*
 			 * See comment in do_read_cache_page on

why
* wait_on_page_locked is used to avoid
unnecessarily

mm/filemap.c +0 -1

 			 * serialisations and why it's safe.
 			 */
-			error =

wait_on_page_locked_killable(page);

mm/filemap.c +6 -0

+			if (iocb->ki_flags & IOCB_WAITQ) {
+				if (written) {
+					put_page(page);
+					goto out;
+				}
+				error =

wait_on_page_locked_async(page,

mm/filemap.c +1 -0

+								

iocb-

Quoted 5 lines

Quoted 1 line

Quoted 1 line

private);

If it is being used in 'generic_file_buffered_read()' as storage for a
wait queue, then it is hard to consider this a 'private' field.

private isn't the prettiest, and in fact this one in particular is a bit
of a mess. It's not clear if it's caller or callee owned. It's generally
not used, outside of the old usb gadget code, iomap O_DIRECT, and ocfs2.
With FMODE_BUF_RASYNC, the fs obviously can't set it if it uses ->private for buffered IO.

Quoted 3 lines

Perhaps either rename and add type checking, or else add a separate
field altogether to struct kiocb?

I'd hate to add a new field and increase the size of the kiocb... One
alternative is to do:

	union {
		void *private;
		struct wait_page_queue *ki_waitq;
	};

and still use IOCB_WAITQ to say that ->ki_waitq is valid.

There's also 4 bytes of padding in the kiocb struct. And some fields are
only used for O_DIRECT as well, eg ->ki_cookie which is just used for
polled O_DIRECT. So we could also do:

	union {
		unsigned int ki_cookie;
		struct wait_page_queue *ki_waitq;
	};

and still not grow the kiocb. How about we go with this approach, and
also add:

Quoted 6 lines

	if (kiocb->ki_flags & IOCB_HIPRI)
		return -EOPNOTSUPP;

to kiocb_wait_page_queue_init() to make sure that this combination isn't
valid?

Here's the incremental, which is spread over 3 patches. I think this one
makes sense, as polled IO doesn't support buffered IO. And because doing
an async callback for completion is not how polled IO operates anyway,
so even if buffered IO supported it, we'd not use the callback for polled IO anyway. kiocb_wait_page_queue_init() checks and backs this up.

include/linux/fs.h +4 -2

index 0ef5f5973b1c..f7b1eb765c6e 100644
@@ -317,7 +317,7 @@ enum rw_hint {
 #define IOCB_SYNC		(1 << 5)
 #define IOCB_WRITE		(1 << 6)
 #define IOCB_NOWAIT		(1 << 7)
-/* iocb->private holds wait_page_async struct */
+/* iocb->ki_waitq is valid */
 #define IOCB_WAITQ		(1 << 8)
 
 struct kiocb {

@@ -332,7 +332,10 @@ struct kiocb {
 	int			ki_flags;
 	u16			ki_hint;
 	u16			ki_ioprio; /* See linux/ioprio.h */
-	unsigned int		ki_cookie; /* for ->iopoll */
+	union {
+		unsigned int		ki_cookie; /* for ->iopoll */
+		struct wait_page_queue	*ki_waitq; /* for async

buffered IO */

include/linux/fs.h +1 -0

+	};
 
 	randomized_struct_fields_end
 };

include/linux/pagemap.h +4 -1

index def58de92053..8b65420410ee 100644
@@ -498,13 +498,16 @@ static inline int
 kiocb_wait_page_queue_init(struct kiocb *kiocb,
 					     wait_queue_func_t func,
 					     void *data)
 {
+	/* Can't support async wakeup with polled IO */
+	if (kiocb->ki_flags & IOCB_HIPRI)
+		return -EINVAL;
 	if (kiocb->ki_filp->f_mode & FMODE_BUF_RASYNC) {
 		wait->wait.func = func;
 		wait->wait.private = data;
 		wait->wait.flags = 0;
 		INIT_LIST_HEAD(&wait->wait.entry);
 		kiocb->ki_flags |= IOCB_WAITQ;
-		kiocb->private = wait;
+		kiocb->ki_waitq = wait;
 		return 0;
 	}

mm/filemap.c +1 -1

index a3b86c9acdc8..18022de7dc33 100644
@@ -2077,7 +2077,7 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 					goto out;
 				}
 				error = wait_on_page_locked_async(page,
-								iocb-
 > private);
+								iocb-
 > ki_waitq);

Quoted 12 lines

mm/filemap.c +1 -1

 			} else {
 				if (iocb->ki_flags & IOCB_NOWAIT) {
 					put_page(page);

@@ -2173,7 +2173,7 @@ static ssize_t
 generic_file_buffered_read(struct kiocb *iocb,
 page_not_up_to_date:
 		/* Get exclusive access to the page ... */
 		if (iocb->ki_flags & IOCB_WAITQ)
-			error = lock_page_async(page, iocb->private);
+			error = lock_page_async(page, iocb->ki_waitq);
 		else
 			error = lock_page_killable(page);

Quoted 1 line

if (unlikely(error))

Ack. That seems cleaner to me.

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

fs/io_uring.c +5 -0

+		task_work_add(tsk, &rw->task_work, true);
+	}
+	wake_up_process(tsk);
+	return 1;
+}

static int io_read(struct io_kiocb *req, bool force_nonblock) {

fs/io_uring.c +1 -0

 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2601,6 +2696,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 
+retry:
 		if (req->file->f_op->read_iter)
 			ret2 = call_read_iter(req->file, kiocb, &iter);
 		else

fs/io_uring.c +2 -0

+				goto retry;
+			kiocb->ki_flags &= ~IOCB_WAITQ;
 			return -EAGAIN;
 		}
 	}

Quoted 14 lines · Jens Axboe

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

...

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	init_task_work(&rw->task_work, io_async_buf_retry);
+	/* submit ref gets dropped, acquire a new one */
+	refcount_inc(&req->refs);
+	tsk = req->task;
+	ret = task_work_add(tsk, &rw->task_work, true);
+	if (unlikely(ret)) {
+		/* queue just for cancelation */
+		init_task_work(&rw->task_work, io_async_buf_cancel);
+		tsk = io_wq_get_task(req->ctx->io_wq);

IIRC, task will be put somewhere around io_free_req(). Then shouldn't here be some juggling with reassigning req->task with task_{get,put}()?

Quoted 5 lines

(quoted diff) +5 -0

+		task_work_add(tsk, &rw->task_work, true);
+	}
+	wake_up_process(tsk);
+	return 1;
+}

...

Quoted 7 lines

static int io_read(struct io_kiocb *req, bool force_nonblock) {

(quoted diff) +0 -0

 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2601,6 +2696,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 

Review static int io_read(struct io_kiocb *req, bool force_nonblock) · line 2718

Quoted 9 lines

(quoted diff) +2 -0

+retry:
 		if (req->file->f_op->read_iter)
 			ret2 = call_read_iter(req->file, kiocb, &iter);
 		else

@@ -2619,6 +2715,9 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
+			if (io_rw_should_retry(req))

It looks like a state machine with IOCB_WAITQ and gotos. Wouldn't it be cleaner to call call_read_iter()/loop_rw_iter() here directly instead of "goto retry" ?

BTW, can this async stuff return -EAGAIN ?

Quoted 16 lines · Pavel Begunkov

Quoted 14 lines · Jens Axboe

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

...

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	init_task_work(&rw->task_work, io_async_buf_retry);
+	/* submit ref gets dropped, acquire a new one */
+	refcount_inc(&req->refs);
+	tsk = req->task;
+	ret = task_work_add(tsk, &rw->task_work, true);
+	if (unlikely(ret)) {
+		/* queue just for cancelation */
+		init_task_work(&rw->task_work, io_async_buf_cancel);
+		tsk = io_wq_get_task(req->ctx->io_wq);

IIRC, task will be put somewhere around io_free_req(). Then shouldn't here be some juggling with reassigning req->task with task_{get,put}()?

Not sure I follow? Yes, we'll put this task again when the request is freed, but not sure what you mean with juggling?

Quoted 13 lines

Quoted 5 lines

(quoted diff) +5 -0

+		task_work_add(tsk, &rw->task_work, true);
+	}
+	wake_up_process(tsk);
+	return 1;
+}

...

Quoted 7 lines

static int io_read(struct io_kiocb *req, bool force_nonblock) {

fs/io_uring.c +0 -0

 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2601,6 +2696,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 

Quoted 12 lines

fs/io_uring.c static int io_read(struct io_kiocb *req, bool force_nonblock) · line 2718

Quoted 9 lines

fs/io_uring.c +2 -0

+retry:
 		if (req->file->f_op->read_iter)
 			ret2 = call_read_iter(req->file, kiocb, &iter);
 		else

@@ -2619,6 +2715,9 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
+			if (io_rw_should_retry(req))

It looks like a state machine with IOCB_WAITQ and gotos. Wouldn't it be cleaner to call call_read_iter()/loop_rw_iter() here directly instead of "goto retry" ?

We could, probably making that part a separate helper then. How about the below incremental?

Quoted 2 lines

BTW, can this async stuff return -EAGAIN ?

Probably? Prefer not to make any definitive calls on that being possible or not, as it's sure to disappoint. If it does and IOCB_WAITQ is already set, then we'll punt to a thread like before.

fs/io_uring.c +8 -5

index a5a4d9602915..669dccd81207 100644
@@ -2677,6 +2677,13 @@ static bool io_rw_should_retry(struct io_kiocb *req)
 	return false;
 }
 
+static int __io_read(struct io_kiocb *req, struct iov_iter *iter)
+{
+	if (req->file->f_op->read_iter)
+		return call_read_iter(req->file, &req->rw.kiocb, iter);
+	return loop_rw_iter(READ, req->file, &req->rw.kiocb, iter);
+}
+
 static int io_read(struct io_kiocb *req, bool force_nonblock)
 {
 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2710,11 +2717,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 
-retry:
-		if (req->file->f_op->read_iter)
-			ret2 = call_read_iter(req->file, kiocb, &iter);
-		else
-			ret2 = loop_rw_iter(READ, req->file, kiocb, &iter);
+		ret2 = __io_read(req, &iter);
 
 		/* Catch -EAGAIN return for forced non-blocking submission */
 		if (!force_nonblock || ret2 != -EAGAIN) {

fs/io_uring.c +1 -0

+			}
 			kiocb->ki_flags &= ~IOCB_WAITQ;
 			return -EAGAIN;
 		}

Quoted 31 lines · Jens Axboe

Quoted 16 lines · Pavel Begunkov

Quoted 14 lines · Jens Axboe

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

...

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	init_task_work(&rw->task_work, io_async_buf_retry);
+	/* submit ref gets dropped, acquire a new one */
+	refcount_inc(&req->refs);
+	tsk = req->task;
+	ret = task_work_add(tsk, &rw->task_work, true);
+	if (unlikely(ret)) {
+		/* queue just for cancelation */
+		init_task_work(&rw->task_work, io_async_buf_cancel);
+		tsk = io_wq_get_task(req->ctx->io_wq);

IIRC, task will be put somewhere around io_free_req(). Then shouldn't here be some juggling with reassigning req->task with task_{get,put}()?

Quoted 2 lines

Not sure I follow? Yes, we'll put this task again when the request is freed, but not sure what you mean with juggling?

I meant something like:

...
/* queue just for cancelation */ init_task_work(&rw->task_work, io_async_buf_cancel);
+ put_task_struct(req->task);
+ req->task = get_task_struct(io_wq_task);

but, thinking twice, if I got the whole idea right, it should be ok as is -- io-wq won't go away before the request anyway, and leaving req->task pinned down for a bit is not a problem.

Quoted 26 lines

Quoted 25 lines

Quoted 5 lines

(quoted diff) +5 -0

+		task_work_add(tsk, &rw->task_work, true);
+	}
+	wake_up_process(tsk);
+	return 1;
+}

...

Quoted 12 lines

static int io_read(struct io_kiocb *req, bool force_nonblock) {

fs/io_uring.c +1 -0

 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2601,6 +2696,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 
+retry:
 		if (req->file->f_op->read_iter)
 			ret2 = call_read_iter(req->file, kiocb, &iter);
 		else

fs/io_uring.c static int io_read(struct io_kiocb *req, bool force_nonblock) · line 2718

Quoted 4 lines

fs/io_uring.c +1 -0

@@ -2619,6 +2715,9 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
+			if (io_rw_should_retry(req))

It looks like a state machine with IOCB_WAITQ and gotos. Wouldn't it be cleaner to call call_read_iter()/loop_rw_iter() here directly instead of "goto retry" ?

Quoted 2 lines

We could, probably making that part a separate helper then. How about the below incremental?

IMHO, it was easy to get lost with such implicit state switching. Looks better now! See a small comment below.

Quoted 32 lines

fs/io_uring.c +8 -5

index a5a4d9602915..669dccd81207 100644
@@ -2677,6 +2677,13 @@ static bool io_rw_should_retry(struct io_kiocb *req)
 	return false;
 }
 
+static int __io_read(struct io_kiocb *req, struct iov_iter *iter)
+{
+	if (req->file->f_op->read_iter)
+		return call_read_iter(req->file, &req->rw.kiocb, iter);
+	return loop_rw_iter(READ, req->file, &req->rw.kiocb, iter);
+}
+
 static int io_read(struct io_kiocb *req, bool force_nonblock)
 {
 	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;

@@ -2710,11 +2717,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 	if (!ret) {
 		ssize_t ret2;
 
-retry:
-		if (req->file->f_op->read_iter)
-			ret2 = call_read_iter(req->file, kiocb, &iter);
-		else
-			ret2 = loop_rw_iter(READ, req->file, kiocb, &iter);
+		ret2 = __io_read(req, &iter);
 
 		/* Catch -EAGAIN return for forced non-blocking submission */
 		if (!force_nonblock || ret2 != -EAGAIN) {

fs/io_uring.c static int io_read(struct io_kiocb *req, bool force_nonblock) · line 2738

Quoted 9 lines

fs/io_uring.c +4 -2

@@ -2729,8 +2732,11 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
-			if (io_rw_should_retry(req))
-				goto retry;
+			if (io_rw_should_retry(req)) {
+				ret2 = __io_read(req, &iter);
+				if (ret2 != -EAGAIN)
+					goto out_free;

"goto out_free" returns ret=0, so someone should add a cqe

if (ret2 != -EAGAIN) {
	kiocb_done(kiocb, ret2);
	goto free_out;
}

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

fs/io_uring.c +4 -0

index e95481c552ff..dd532d2634c2 100644
@@ -498,6 +498,8 @@ struct io_async_rw {
 	struct iovec			*iov;
 	ssize_t				nr_segs;
 	ssize_t				size;
+	struct wait_page_queue		wpq;
+	struct callback_head		task_work;
 };
 
 struct io_async_ctx {

@@ -2568,6 +2570,99 @@ static int io_read_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+static void io_async_buf_cancel(struct callback_head *cb)
+{

fs/io_uring.c +7 -0

+	io_commit_cqring(ctx);
+	spin_unlock_irq(&ctx->completion_lock);
+
+	io_cqring_ev_posted(ctx);
+	req_set_fail_links(req);
+	io_double_put_req(req);
+}

Quoted 31 lines · Jens Axboe

If the file is flagged with FMODE_BUF_RASYNC, then we don't have to punt the buffered read to an io-wq worker. Instead we can rely on page unlocking callbacks to support retry based async IO. This is a lot more efficient than doing async thread offload.

The retry is done similarly to how we handle poll based retry. From the unlock callback, we simply queue the retry to a task_work based handler.

Signed-off-by Jens Axboe <[email protected]>

---
 fs/io_uring.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

fs/io_uring.c +2 -0

index e95481c552ff..dd532d2634c2 100644
@@ -498,6 +498,8 @@ struct io_async_rw {
 	struct iovec			*iov;
 	ssize_t				nr_segs;
 	ssize_t				size;
+	struct wait_page_queue		wpq;
+	struct callback_head		task_work;
 };
 
 struct io_async_ctx {

@@ -2568,6 +2570,99 @@ static int io_read_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 

fs/io_uring.c static int io_read_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe, · line 2584

Quoted 12 lines

fs/io_uring.c +12 -0

+static void io_async_buf_cancel(struct callback_head *cb)
+{
+	struct io_async_rw *rw;
+	struct io_ring_ctx *ctx;
+	struct io_kiocb *req;
+
+	rw = container_of(cb, struct io_async_rw, task_work);
+	req = rw->wpq.wait.private;
+	ctx = req->ctx;
+
+	spin_lock_irq(&ctx->completion_lock);
+	io_cqring_fill_event(req, -ECANCELED);

It seems like it should go through kiocb_done()/io_complete_rw_common(). My concern is missing io_put_kbuf().

We technically support this already through io_uring, but it's implemented with a thread backend to support cases where we would block. This isn't ideal.

After a few prep patches, the core of this patchset is adding support for async callbacks on page unlock. With this primitive, we can simply retry the IO operation. With io_uring, this works a lot like poll based retry for files that support it. If a page is currently locked and needed, -EIOCBQUEUED is returned with a callback armed. The callers callback is responsible for restarting the operation.

With this callback primitive, we can add support for generic_file_buffered_read(), which is what most file systems end up using for buffered reads. XFS/ext4/btrfs/bdev is wired up, but probably trivial to add more.

The file flags support for this by setting FMODE_BUF_RASYNC, similar to what we do for FMODE_NOWAIT. Open to suggestions here if this is the preferred method or not.

In terms of results, I wrote a small test app that randomly reads 4G of data in 4K chunks from a file hosted by ext4. The app uses a queue depth of 32. If you want to test yourself, you can just use buffered=1 with ioengine=io_uring with fio. No application changes are needed to use the more optimized buffered async read.

preadv for comparison:

	real    1m13.821s
	user    0m0.558s
	sys     0m11.125s
	CPU	~13%

Mainline:

	real    0m12.054s
	user    0m0.111s
	sys     0m5.659s
	CPU	~32% + ~50% == ~82%

This patchset:

	real    0m9.283s
	user    0m0.147s
	sys     0m4.619s
	CPU	~52%

The CPU numbers are just a rough estimate. For the mainline io_uring run, this includes the app itself and all the threads doing IO on its behalf (32% for the app, ~1.6% per worker and 32 of them). Context switch rate is much smaller with the patchset, since we only have the one task performing IO.

The goal here is efficiency. Async thread offload adds latency, and it also adds noticable overhead on items such as adding pages to the page cache. By allowing proper async buffered read support, we don't have X threads hammering on the same inode page cache, we have just the single app actually doing IO.

Been beating on this and it's solid for me, and I'm now pretty happy with how it all turned out. Not aware of any missing bits/pieces or code cleanups that need doing.

Series can also be found here:

https://git.kernel.dk/cgit/linux-block/log/?h=async-buffered.3

or pull from:

git://git.kernel.dk/linux-block async-buffered.3

 fs/block_dev.c            |   2 +-
 fs/btrfs/file.c           |   2 +-
 fs/ext4/file.c            |   2 +-
 fs/io_uring.c             |  99 ++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_file.c         |   2 +-
 include/linux/blk_types.h |   3 +-
 include/linux/fs.h        |   5 ++
 include/linux/pagemap.h   |  64 ++++++++++++++++++++++
 mm/filemap.c              | 111 ++++++++++++++++++++++++--------------
 9 files changed, 245 insertions(+), 45 deletions(-)

Changes since v2:
- Get rid of unnecessary wait_page_async struct, just use wait_page_async
- Add another prep handler, adding wake_page_match()
- Use wake_page_match() in both callers
Changes since v1:
- Fix an issue with inline page locking
- Fix a potential race with __wait_on_page_locked_async()
- Fix a hang related to not setting page_match, thus missing a wakeup

And this one is v3, obviously, not v2...

Quoted 88 lines · Jens Axboe

We technically support this already through io_uring, but it's implemented with a thread backend to support cases where we would block. This isn't ideal.

After a few prep patches, the core of this patchset is adding support for async callbacks on page unlock. With this primitive, we can simply retry the IO operation. With io_uring, this works a lot like poll based retry for files that support it. If a page is currently locked and needed, -EIOCBQUEUED is returned with a callback armed. The callers callback is responsible for restarting the operation.

With this callback primitive, we can add support for generic_file_buffered_read(), which is what most file systems end up using for buffered reads. XFS/ext4/btrfs/bdev is wired up, but probably trivial to add more.

The file flags support for this by setting FMODE_BUF_RASYNC, similar to what we do for FMODE_NOWAIT. Open to suggestions here if this is the preferred method or not.

In terms of results, I wrote a small test app that randomly reads 4G of data in 4K chunks from a file hosted by ext4. The app uses a queue depth of 32. If you want to test yourself, you can just use buffered=1 with ioengine=io_uring with fio. No application changes are needed to use the more optimized buffered async read.

preadv for comparison:

	real    1m13.821s
	user    0m0.558s
	sys     0m11.125s
	CPU	~13%

Mainline:

	real    0m12.054s
	user    0m0.111s
	sys     0m5.659s
	CPU	~32% + ~50% == ~82%

This patchset:

	real    0m9.283s
	user    0m0.147s
	sys     0m4.619s
	CPU	~52%

The CPU numbers are just a rough estimate. For the mainline io_uring run, this includes the app itself and all the threads doing IO on its behalf (32% for the app, ~1.6% per worker and 32 of them). Context switch rate is much smaller with the patchset, since we only have the one task performing IO.

The goal here is efficiency. Async thread offload adds latency, and it also adds noticable overhead on items such as adding pages to the page cache. By allowing proper async buffered read support, we don't have X threads hammering on the same inode page cache, we have just the single app actually doing IO.

Been beating on this and it's solid for me, and I'm now pretty happy with how it all turned out. Not aware of any missing bits/pieces or code cleanups that need doing.

Series can also be found here:

https://git.kernel.dk/cgit/linux-block/log/?h=async-buffered.3

or pull from:

git://git.kernel.dk/linux-block async-buffered.3

  fs/block_dev.c            |   2 +-
  fs/btrfs/file.c           |   2 +-
  fs/ext4/file.c            |   2 +-
  fs/io_uring.c             |  99 ++++++++++++++++++++++++++++++++++
  fs/xfs/xfs_file.c         |   2 +-
  include/linux/blk_types.h |   3 +-
  include/linux/fs.h        |   5 ++
  include/linux/pagemap.h   |  64 ++++++++++++++++++++++
  mm/filemap.c              | 111 ++++++++++++++++++++++++--------------
  9 files changed, 245 insertions(+), 45 deletions(-)

Changes since v2:
- Get rid of unnecessary wait_page_async struct, just use wait_page_async
- Add another prep handler, adding wake_page_match()
- Use wake_page_match() in both callers
Changes since v1:
- Fix an issue with inline page locking
- Fix a potential race with __wait_on_page_locked_async()
- Fix a hang related to not setting page_match, thus missing a wakeup

Yes! Jens & Team! Yes!

My code has never looked so beautiful, been so efficient and run so well since switching to io_uring/async awesome-ness.. Really, really is a game-changer in terms of software design, control, performance, expressiveness... so many levels. Really, really great work! Thank you!