Mark the plug with nowait == true, which will cause requests to avoid blocking on request allocation. If they do, we catch them and reissue them from a task_work based handler.

Normally we can catch -EAGAIN directly, but the hard case is for split requests. As an example, the application issues a 512KB request. The block core will split this into 128KB if that's the max size for the device. The first request issues just fine, but we run into -EAGAIN for some latter splits for the same request. As the bio is split, we don't get to see the -EAGAIN until one of the actual reads complete, and hence we cannot handle it inline as part of submission.

This does potentially cause re-reads of parts of the range, as the whole request is reissued. There's currently no better way to handle this.

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

---
 fs/io_uring.c | 148 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 24 deletions(-)

fs/io_uring.c +0 -0

index 2e257c5a1866..40413fb9d07b 100644

fs/io_uring.c +4 -0

+static void io_rw_resubmit(struct callback_head *cb)
+{
+	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
+	struct io_ring_ctx *ctx = req->ctx;

fs/io_uring.c +5 -0

+}
+#endif
+
+static bool io_rw_reissue(struct io_kiocb *req, long res)
+{

fs/io_uring.c +6 -0

+	if (!ret)
+		return true;
+#endif
+	return false;
+}
+

Quoted 25 lines · Jens Axboe

Mark the plug with nowait == true, which will cause requests to avoid blocking on request allocation. If they do, we catch them and reissue them from a task_work based handler.

Normally we can catch -EAGAIN directly, but the hard case is for split requests. As an example, the application issues a 512KB request. The block core will split this into 128KB if that's the max size for the device. The first request issues just fine, but we run into -EAGAIN for some latter splits for the same request. As the bio is split, we don't get to see the -EAGAIN until one of the actual reads complete, and hence we cannot handle it inline as part of submission.

This does potentially cause re-reads of parts of the range, as the whole request is reissued. There's currently no better way to handle this.

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

---
 fs/io_uring.c | 148 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 24 deletions(-)

fs/io_uring.c +0 -0

index 2e257c5a1866..40413fb9d07b 100644

fs/io_uring.c static int io_file_get(struct io_submit_state *state, struct io_kiocb *req, · line 902

Quoted 3 lines

fs/io_uring.c +0 -0

@@ -900,6 +900,13 @@ static int io_file_get(struct io_submit_state *state, struct io_kiocb *req,
 static void __io_queue_sqe(struct io_kiocb *req,
 			   const struct io_uring_sqe *sqe);
 

...> +

Quoted 9 lines

fs/io_uring.c +9 -0

+static void io_rw_resubmit(struct callback_head *cb)
+{
+	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
+	struct io_ring_ctx *ctx = req->ctx;
+	int err;
+
+	__set_current_state(TASK_RUNNING);
+
+	err = io_sq_thread_acquire_mm(ctx, req);

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2067

Quoted 5 lines

fs/io_uring.c +5 -0

+
+	if (io_resubmit_prep(req, err)) {
+		refcount_inc(&req->refs);
+		io_queue_async_work(req);
+	}

Hmm, I have similar stuff but for iopoll. On top removing grab_env* for linked reqs and some extra. I think I'll rebase on top of this.

Quoted 8 lines

fs/io_uring.c +8 -0

+static bool io_rw_reissue(struct io_kiocb *req, long res)
+{
+#ifdef CONFIG_BLOCK
+	struct task_struct *tsk;
+	int ret;
+
+	if ((res != -EAGAIN && res != -EOPNOTSUPP) || io_wq_current_is_worker())
+		return false;

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2082

Quoted 4 lines

fs/io_uring.c +4 -0

+
+	tsk = req->task;
+	init_task_work(&req->task_work, io_rw_resubmit);
+	ret = task_work_add(tsk, &req->task_work, true);

I don't like that the request becomes un-discoverable for cancellation awhile sitting in the task_work list. Poll stuff at least have hash_node for that.

Quoted 39 lines · Pavel Begunkov

Quoted 25 lines · Jens Axboe

Mark the plug with nowait == true, which will cause requests to avoid blocking on request allocation. If they do, we catch them and reissue them from a task_work based handler.

Normally we can catch -EAGAIN directly, but the hard case is for split requests. As an example, the application issues a 512KB request. The block core will split this into 128KB if that's the max size for the device. The first request issues just fine, but we run into -EAGAIN for some latter splits for the same request. As the bio is split, we don't get to see the -EAGAIN until one of the actual reads complete, and hence we cannot handle it inline as part of submission.

This does potentially cause re-reads of parts of the range, as the whole request is reissued. There's currently no better way to handle this.

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

---
 fs/io_uring.c | 148 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 24 deletions(-)

fs/io_uring.c +0 -0

index 2e257c5a1866..40413fb9d07b 100644

fs/io_uring.c static int io_file_get(struct io_submit_state *state, struct io_kiocb *req, · line 902

Quoted 3 lines

fs/io_uring.c +0 -0

@@ -900,6 +900,13 @@ static int io_file_get(struct io_submit_state *state, struct io_kiocb *req,
 static void __io_queue_sqe(struct io_kiocb *req,
 			   const struct io_uring_sqe *sqe);
 

...> +

Quoted 9 lines

fs/io_uring.c +9 -0

+static void io_rw_resubmit(struct callback_head *cb)
+{
+	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
+	struct io_ring_ctx *ctx = req->ctx;
+	int err;
+
+	__set_current_state(TASK_RUNNING);
+
+	err = io_sq_thread_acquire_mm(ctx, req);

Quoted 8 lines

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2067

Quoted 5 lines

fs/io_uring.c +5 -0

+
+	if (io_resubmit_prep(req, err)) {
+		refcount_inc(&req->refs);
+		io_queue_async_work(req);
+	}

Hmm, I have similar stuff but for iopoll. On top removing grab_env* for linked reqs and some extra. I think I'll rebase on top of this.

Yes, there's certainly overlap there. I consider this series basically wrapped up, so feel free to just base on top of it.

Quoted 8 lines

Quoted 8 lines

fs/io_uring.c +8 -0

+static bool io_rw_reissue(struct io_kiocb *req, long res)
+{
+#ifdef CONFIG_BLOCK
+	struct task_struct *tsk;
+	int ret;
+
+	if ((res != -EAGAIN && res != -EOPNOTSUPP) || io_wq_current_is_worker())
+		return false;

Quoted 8 lines

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2082

Quoted 4 lines

fs/io_uring.c +4 -0

+
+	tsk = req->task;
+	init_task_work(&req->task_work, io_rw_resubmit);
+	ret = task_work_add(tsk, &req->task_work, true);

I don't like that the request becomes un-discoverable for cancellation awhile sitting in the task_work list. Poll stuff at least have hash_node for that.

Async buffered IO was never cancelable, so it doesn't really matter. It's tied to the task, so we know it'll get executed - either run, or canceled if the task is going away. This is really not that different from having the work discoverable through io-wq queueing before, since the latter could never be canceled anyway as it sits there uninterruptibly waiting for IO completion.

Quoted 69 lines · Jens Axboe

Quoted 39 lines · Pavel Begunkov

Quoted 25 lines · Jens Axboe

Mark the plug with nowait == true, which will cause requests to avoid blocking on request allocation. If they do, we catch them and reissue them from a task_work based handler.

Normally we can catch -EAGAIN directly, but the hard case is for split requests. As an example, the application issues a 512KB request. The block core will split this into 128KB if that's the max size for the device. The first request issues just fine, but we run into -EAGAIN for some latter splits for the same request. As the bio is split, we don't get to see the -EAGAIN until one of the actual reads complete, and hence we cannot handle it inline as part of submission.

This does potentially cause re-reads of parts of the range, as the whole request is reissued. There's currently no better way to handle this.

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

---
 fs/io_uring.c | 148 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 124 insertions(+), 24 deletions(-)

fs/io_uring.c +0 -0

index 2e257c5a1866..40413fb9d07b 100644

fs/io_uring.c static int io_file_get(struct io_submit_state *state, struct io_kiocb *req, · line 902

Quoted 3 lines

fs/io_uring.c +0 -0

@@ -900,6 +900,13 @@ static int io_file_get(struct io_submit_state *state, struct io_kiocb *req,
 static void __io_queue_sqe(struct io_kiocb *req,
 			   const struct io_uring_sqe *sqe);
 

...> +

Quoted 9 lines

fs/io_uring.c +9 -0

+static void io_rw_resubmit(struct callback_head *cb)
+{
+	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
+	struct io_ring_ctx *ctx = req->ctx;
+	int err;
+
+	__set_current_state(TASK_RUNNING);
+
+	err = io_sq_thread_acquire_mm(ctx, req);

Quoted 8 lines

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2067

Quoted 5 lines

fs/io_uring.c +5 -0

+
+	if (io_resubmit_prep(req, err)) {
+		refcount_inc(&req->refs);
+		io_queue_async_work(req);
+	}

Hmm, I have similar stuff but for iopoll. On top removing grab_env* for linked reqs and some extra. I think I'll rebase on top of this.

Yes, there's certainly overlap there. I consider this series basically wrapped up, so feel free to just base on top of it.

Quoted 8 lines

Quoted 8 lines

fs/io_uring.c +8 -0

+static bool io_rw_reissue(struct io_kiocb *req, long res)
+{
+#ifdef CONFIG_BLOCK
+	struct task_struct *tsk;
+	int ret;
+
+	if ((res != -EAGAIN && res != -EOPNOTSUPP) || io_wq_current_is_worker())
+		return false;

Quoted 8 lines

fs/io_uring.c static void io_complete_rw_common(struct kiocb *kiocb, long res) · line 2082

Quoted 4 lines

fs/io_uring.c +4 -0

+
+	tsk = req->task;
+	init_task_work(&req->task_work, io_rw_resubmit);
+	ret = task_work_add(tsk, &req->task_work, true);

I don't like that the request becomes un-discoverable for cancellation awhile sitting in the task_work list. Poll stuff at least have hash_node for that.

Quoted 6 lines

Async buffered IO was never cancelable, so it doesn't really matter. It's tied to the task, so we know it'll get executed - either run, or canceled if the task is going away. This is really not that different from having the work discoverable through io-wq queueing before, since the latter could never be canceled anyway as it sits there uninterruptibly waiting for IO completion.

Makes sense. I was thinking about using this task-requeue for all kinds of requests. Though, instead of speculating it'd be better for me to embody ideas into patches and see.

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Acked-by Johannes Weiner <[email protected]>

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

---
 mm/filemap.c | 2 --
 1 file changed, 2 deletions(-)

mm/filemap.c +0 -0

index f0ae9a6308cb..3378d4fca883 100644

Quoted 2 lines · Jens Axboe

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Doesn't think break preadv2(RWF_NOWAIT) semantics for on buffered reads? i.e. this can now block on memory allocation for the page cache, which is something RWF_NOWAIT IO should not do....

Quoted 7 lines · Dave Chinner

Quoted 2 lines · Jens Axboe

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Doesn't think break preadv2(RWF_NOWAIT) semantics for on buffered reads? i.e. this can now block on memory allocation for the page cache, which is something RWF_NOWAIT IO should not do....

Yes. This eventually ends up in page_cache_readahead_unbounded() which gets its gfp flags from readahead_gfp_mask(mapping).

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

Quoted 9 lines · Matthew Wilcox

Quoted 7 lines · Dave Chinner

Quoted 2 lines · Jens Axboe

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Doesn't think break preadv2(RWF_NOWAIT) semantics for on buffered reads? i.e. this can now block on memory allocation for the page cache, which is something RWF_NOWAIT IO should not do....

Quoted 7 lines

Yes. This eventually ends up in page_cache_readahead_unbounded() which gets its gfp flags from readahead_gfp_mask(mapping).

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

Quoted 6 lines · Jens Axboe

Quoted 4 lines · Matthew Wilcox

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

Quoted 4 lines

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Jens Axboe

Quoted 4 lines · Matthew Wilcox

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

Quoted 3 lines

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

I think we can still sleep though, right? I was thinking more like this:

http://git.infradead.org/users/willy/linux.git/shortlog/refs/heads/memalloc

Quoted 17 lines · Jens Axboe

Quoted 15 lines · Jens Axboe

Quoted 10 lines · Matthew Wilcox

Quoted 4 lines

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

Quoted 4 lines

I think we can still sleep though, right? I was thinking more like this:

http://git.infradead.org/users/willy/linux.git/shortlog/refs/heads/memalloc

Yeah, that's probably better. How do we want to handle this? I've already got the other bits queued up. I can either add them to the series, or pull a branch that'll go into Linus as well.

Am Mi., 24. Juni 2020 um 18:48 Uhr schrieb Jens Axboe <[email protected]>:

Quoted 24 lines · Matthew Wilcox

Quoted 21 lines · Jens Axboe

Quoted 7 lines · Jens Axboe

Quoted 6 lines · Matthew Wilcox

Quoted 4 lines

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

Quoted 8 lines

Quoted 4 lines

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

I think we can still sleep though, right? I was thinking more like this:

http://git.infradead.org/users/willy/linux.git/shortlog/refs/heads/memalloc

Quoted 3 lines

Yeah, that's probably better. How do we want to handle this? I've already got the other bits queued up. I can either add them to the series, or pull a branch that'll go into Linus as well.

Also note my conflicting patch that introduces a IOCB_NOIO flag for fixing a gfs2 regression:

https://lore.kernel.org/linux-fsdevel/[email protected]/

Quoted 17 lines · Matthew Wilcox

Quoted 15 lines · Jens Axboe

Quoted 10 lines · Jens Axboe

Quoted 4 lines · Matthew Wilcox

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

Quoted 4 lines

I think we can still sleep though, right? I was thinking more like this:

http://git.infradead.org/users/willy/linux.git/shortlog/refs/heads/memalloc

Yeah, that's probably better. How do we want to handle this? I've already got the other bits queued up. I can either add them to the series, or pull a branch that'll go into Linus as well.

Quoted 18 lines · Jens Axboe

Quoted 17 lines · Matthew Wilcox

Quoted 15 lines · Jens Axboe

Quoted 10 lines · Jens Axboe

Quoted 4 lines · Matthew Wilcox

I'd be quite happy to add a gfp_t to struct readahead_control. The other thing I've been looking into for other reasons is adding a memalloc_nowait_{save,restore}, which would avoid passing down the gfp_t.

That was my first thought, having the memalloc_foo_save/restore for this. I don't think adding a gfp_t to readahead_control is going to be super useful, seems like the kind of thing that should be non-blocking by default.

We're already doing memalloc_nofs_save/restore in page_cache_readahead_unbounded(), so I think all we need is to just do a noio dance in generic_file_buffered_read() and that should be enough.

Quoted 8 lines

Quoted 4 lines

I think we can still sleep though, right? I was thinking more like this:

http://git.infradead.org/users/willy/linux.git/shortlog/refs/heads/memalloc

Yeah, that's probably better. How do we want to handle this? I've already got the other bits queued up. I can either add them to the series, or pull a branch that'll go into Linus as well.

Jens, Willy,

Now that this patch has been merged and IOCB_NOWAIT semantics ifor buffered reads are broken in Linus' tree, what's the plan to get this regression fixed before 5.9 releases?

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Acked-by Johannes Weiner <[email protected]>

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

Quoted 5 lines · Jens Axboe

The read-ahead shouldn't block, so allow it to be done even if IOCB_NOWAIT is set in the kiocb.

Acked-by Johannes Weiner <[email protected]>

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

BTW, Jens, in case nobody had mentioned it, the Reply-To field for the patches in this patchset is screwed up:

| Reply-To: [email protected], [email protected], [email protected],
|         [email protected], [email protected],
| 	        [email protected]

Normally waiting for a page to become unlocked, or locking the page, requires waiting for IO to complete. Add support for lock_page_async() and wait_on_page_locked_async(), which are callback based instead. This allows a caller to get notified when a page becomes unlocked, rather than wait for it.

We add a new iocb field, ki_waitq, to pass in the necessary data for this to happen. We can unionize this with ki_cookie, since that is only used for polled IO. Polled IO can never co-exist with async callbacks, as it is (by definition) polled completions. struct wait_page_key is made public, and we define struct wait_page_async as the interface between the caller and the core.

Acked-by Johannes Weiner <[email protected]>

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

---
 include/linux/fs.h      |  7 ++++++-
 include/linux/pagemap.h | 17 ++++++++++++++++
 mm/filemap.c            | 45 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 67 insertions(+), 2 deletions(-)

include/linux/fs.h +6 -1

index 6c4ab4dc1cd7..6ac919b40596 100644
@@ -315,6 +315,8 @@ enum rw_hint {
 #define IOCB_SYNC              (1 << 5)
 #define IOCB_WRITE             (1 << 6)
 #define IOCB_NOWAIT            (1 << 7)
+/* iocb->ki_waitq is valid */
+#define IOCB_WAITQ             (1 << 8)
 
 struct kiocb {
        struct file             *ki_filp;

@@ -328,7 +330,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 */
+       };
 
        randomized_struct_fields_end
 };

include/linux/pagemap.h +17 -0

index 2f18221bb5c8..e053e1d9a4d7 100644
@@ -535,6 +535,7 @@ static inline int wake_page_match(struct wait_page_queue *wait_page,
 
 extern void __lock_page(struct page *page);
 extern int __lock_page_killable(struct page *page);
+extern int __lock_page_async(struct page *page, struct wait_page_queue *wait);
 extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
                                unsigned int flags);
 extern void unlock_page(struct page *page);

@@ -571,6 +572,22 @@ static inline int lock_page_killable(struct page *page)
        return 0;
 }
 
+/*
+ * lock_page_async - Lock the page, unless this would block. If the page
+ * is already locked, then queue a callback when the page becomes unlocked.
+ * This callback can then retry the operation.
+ *
+ * Returns 0 if the page is locked successfully, or -EIOCBQUEUED if the page
+ * was already locked and the callback defined in 'wait' was queued.
+ */
+static inline int lock_page_async(struct page *page,
+                                 struct wait_page_queue *wait)
+{
+       if (!trylock_page(page))
+               return __lock_page_async(page, wait);
+       return 0;
+}
+
 /*
  * lock_page_or_retry - Lock the page, unless this would block and the
  * caller indicated that it can handle a retry.

mm/filemap.c +35 -0

index c3175dbd8fba..e8aaf43bee9f 100644
@@ -1180,6 +1180,36 @@ int wait_on_page_bit_killable(struct page *page, int bit_nr)
 }
 EXPORT_SYMBOL(wait_on_page_bit_killable);
 
+static int __wait_on_page_locked_async(struct page *page,
+                                      struct wait_page_queue *wait, bool set)
+{
+       struct wait_queue_head *q = page_waitqueue(page);
+       int ret = 0;
+
+       wait->page = page;
+       wait->bit_nr = PG_locked;
+
+       spin_lock_irq(&q->lock);
+       __add_wait_queue_entry_tail(q, &wait->wait);
+       SetPageWaiters(page);
+       if (set)
+               ret = !trylock_page(page);
+       else
+               ret = PageLocked(page);
+       /*
+        * If we were succesful now, we know we're still on the
+        * waitqueue as we're still under the lock. This means it's
+        * safe to remove and return success, we know the callback
+        * isn't going to trigger.
+        */
+       if (!ret)
+               __remove_wait_queue(q, &wait->wait);
+       else
+               ret = -EIOCBQUEUED;
+       spin_unlock_irq(&q->lock);
+       return ret;
+}
+
 /**
  * put_and_wait_on_page_locked - Drop a reference and wait for it to be unlocked
  * @page: The page to wait for.

@@ -1342,6 +1372,11 @@ int __lock_page_killable(struct page *__page)
 }
 EXPORT_SYMBOL_GPL(__lock_page_killable);
 
+int __lock_page_async(struct page *page, struct wait_page_queue *wait)
+{
+       return __wait_on_page_locked_async(page, wait, true);
+}
+
 /*
  * Return values:
  * 1 - page is locked; mmap_lock is still held.

mm/filemap.c +4 -2

                /*
                 * A previous I/O error may have been due to temporary
                 * failures, eg. multipath errors.

@@ -2150,7 +2190,10 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
                }
 
                if (!PageUptodate(page)) {
-                       error = lock_page_killable(page);
+                       if (iocb->ki_flags & IOCB_WAITQ)
+                               error = lock_page_async(page, iocb->ki_waitq);
+                       else
+                               error = lock_page_killable(page);
                        if (unlikely(error))
                                goto readpage_error;
                        if (!PageUptodate(page)) {
--

2.27.0

mm/filemap.c ssize_t generic_file_buffered_read(struct kiocb *iocb, · line 2173

Quoted 9 lines

mm/filemap.c +5 -0

@@ -2131,6 +2166,11 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
                }
 
 readpage:
+               if (iocb->ki_flags & IOCB_NOWAIT) {
+                       unlock_page(page);
+                       put_page(page);
+                       goto would_block;
+               }

This hunk should have been part of "mm: allow read-ahead with IOCB_NOWAIT set" ...

btrfs uses generic_file_read_iter(), which already supports this.

Acked-by Chris Mason <[email protected]>

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

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 | 145 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 137 insertions(+), 8 deletions(-)

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

fs/io_uring.c +9 -5

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

@@ -2784,10 +2907,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 		unsigned long nr_segs = iter.nr_segs;
 		ssize_t ret2 = 0;
 
-		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_iter_do_read(req, &iter);
 
 		/* Catch -EAGAIN return for forced non-blocking submission */
 		if (!force_nonblock || (ret2 != -EAGAIN && ret2 != -EIO)) {

@@ -2799,17 +2919,26 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			ret = io_setup_async_rw(req, io_size, iovec,
 						inline_vecs, &iter);
 			if (ret)
-				goto out_free;
+				goto out;
 			/* any defer here is final, must blocking retry */
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
+			/* if we can retry, do so with the callbacks armed */
+			if (io_rw_should_retry(req)) {
+				ret2 = io_iter_do_read(req, &iter);
+				if (ret2 == -EIOCBQUEUED) {
+					goto out;
+				} else if (ret2 != -EAGAIN) {
+					kiocb_done(kiocb, ret2);

fs/io_uring.c +1 -0

+out:
 	return ret;
 }
 

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 | 145 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 137 insertions(+), 8 deletions(-)

...

Quoted 37 lines

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

fs/io_uring.c +13 -5

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

@@ -2784,10 +2907,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 		unsigned long nr_segs = iter.nr_segs;
 		ssize_t ret2 = 0;
 
-		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_iter_do_read(req, &iter);
 
 		/* Catch -EAGAIN return for forced non-blocking submission */
 		if (!force_nonblock || (ret2 != -EAGAIN && ret2 != -EIO)) {

@@ -2799,17 +2919,26 @@ static int io_read(struct io_kiocb *req, bool force_nonblock)
 			ret = io_setup_async_rw(req, io_size, iovec,
 						inline_vecs, &iter);
 			if (ret)
-				goto out_free;
+				goto out;
 			/* any defer here is final, must blocking retry */
 			if (!(req->flags & REQ_F_NOWAIT) &&
 			    !file_can_poll(req->file))
 				req->flags |= REQ_F_MUST_PUNT;
+			/* if we can retry, do so with the callbacks armed */
+			if (io_rw_should_retry(req)) {
+				ret2 = io_iter_do_read(req, &iter);
+				if (ret2 == -EIOCBQUEUED) {
+					goto out;
+				} else if (ret2 != -EAGAIN) {
+					kiocb_done(kiocb, ret2);
+					goto out;
+				}
+			}
+			kiocb->ki_flags &= ~IOCB_WAITQ;
 			return -EAGAIN;

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

Quoted 5 lines

fs/io_uring.c +0 -3

 		}
 	}
-out_free:
-	kfree(iovec);
-	req->flags &= ~REQ_F_NEED_CLEANUP;

This looks fishy. For instance, if it fails early on rw_verify_area(), how would it free yet on-stack iovec? Is it handled somehow?

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.