io_uring: -EAGAIN on write path in case of O_DIRECT

Commit Message

Roman Penyaev March 25, 2019, 10:16 a.m. UTC

Hi Jens,

I gave a try to use io_uring and stumbled upon -EAGAIN on write path
in direct mode if page cache is already populated or has been populated
in-between by some buffered read. I'am talking about
generic_file_direct_write() call, which checks filemap_range_has_page()
on IOCB_NOWAIT path.

To proceed further with tests I simply did the same thing, like you did
in io_read(), and in case of -EAGAIN async worker does the rest.  So the
following chunk works well:

protection,
                  * which will be released by another thread in
@@ -1036,7 +1038,19 @@ static int io_write(struct io_kiocb *req, const 
struct sqe_submit *s,
                                                 SB_FREEZE_WRITE);
                 }
                 kiocb->ki_flags |= IOCB_WRITE;
-               io_rw_done(kiocb, call_write_iter(file, kiocb, &iter));
+
+               ret2 = call_write_iter(file, kiocb, &iter);
+               if (!force_nonblock || ret2 != -EAGAIN) {
+                       io_rw_done(kiocb, ret2);
+               } else {
+                       /*
+                        * If ->needs_lock is true, we're already in 
async
+                        * context.
+                        */
+                       if (!s->needs_lock)
+                               io_async_list_note(WRITE, req, 
iov_count);
+                       ret = -EAGAIN;
+               }


Does it make sense?

--
Roman

Comments

Jens Axboe March 25, 2019, 4:05 p.m. UTC | #1

On 3/25/19 4:16 AM, Roman Penyaev wrote:
> Hi Jens,
> 
> I gave a try to use io_uring and stumbled upon -EAGAIN on write path
> in direct mode if page cache is already populated or has been populated
> in-between by some buffered read. I'am talking about
> generic_file_direct_write() call, which checks filemap_range_has_page()
> on IOCB_NOWAIT path.
> 
> To proceed further with tests I simply did the same thing, like you did
> in io_read(), and in case of -EAGAIN async worker does the rest.  So the
> following chunk works well:
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6aaa30580a2b..ccb656168ae4 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1022,6 +1022,8 @@ static int io_write(struct io_kiocb *req, const 
> struct sqe_submit *s,
> 
>          ret = rw_verify_area(WRITE, file, &kiocb->ki_pos, iov_count);
>          if (!ret) {
> +               ssize_t ret2;
> +
>                  /*
>                   * Open-code file_start_write here to grab freeze 
> protection,
>                   * which will be released by another thread in
> @@ -1036,7 +1038,19 @@ static int io_write(struct io_kiocb *req, const 
> struct sqe_submit *s,
>                                                  SB_FREEZE_WRITE);
>                  }
>                  kiocb->ki_flags |= IOCB_WRITE;
> -               io_rw_done(kiocb, call_write_iter(file, kiocb, &iter));
> +
> +               ret2 = call_write_iter(file, kiocb, &iter);
> +               if (!force_nonblock || ret2 != -EAGAIN) {
> +                       io_rw_done(kiocb, ret2);
> +               } else {
> +                       /*
> +                        * If ->needs_lock is true, we're already in 
> async
> +                        * context.
> +                        */
> +                       if (!s->needs_lock)
> +                               io_async_list_note(WRITE, req, 
> iov_count);
> +                       ret = -EAGAIN;
> +               }
> 
> 
> Does it make sense?

Yeah it makes sense, we need to punt that to async as well if we hit
EAGAIN at that point. Can you send a signed-off-by patch?

Patch

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6aaa30580a2b..ccb656168ae4 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1022,6 +1022,8 @@  static int io_write(struct io_kiocb *req, const 
struct sqe_submit *s,

         ret = rw_verify_area(WRITE, file, &kiocb->ki_pos, iov_count);
         if (!ret) {
+               ssize_t ret2;
+
                 /*
                  * Open-code file_start_write here to grab freeze