[PATCH 0/5] misc update for openclose and provided buffer

[PATCH 0/5] misc update for openclose and provided buffer

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

Patch 1 and 2 are bug fixes for openclose
Patch 3 is to support better close logic
Patch 4 is code clean
Patch 5 is a bug fix for ring-mapped provided buffer

Hao Xu (5):
  io_uring: openclose: fix bug of closing wrong fixed file
  io_uring: openclose: fix bug of unexpected return value in
    IORING_CLOSE_FD_AND_FILE_SLOT mode
  io_uring: openclose: support separate return value for
    IORING_CLOSE_FD_AND_FILE_SLOT
  io_uring: remove duplicate kbuf release
  io_uring: kbuf: consume ring buffer in partial io case

 io_uring/io_uring.c  |  6 ------
 io_uring/kbuf.c      |  9 +++++++--
 io_uring/kbuf.h      | 10 ++++++++--
 io_uring/openclose.c | 10 +++++++---
 io_uring/rsrc.c      |  2 +-
 5 files changed, 23 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH 1/5] io_uring: openclose: fix bug of closing wrong fixed file

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

Don't update ret until fixed file is closed, otherwise the file slot
becomes the error code.

Fixes: a7c41b4687f5 ("io_uring: let IORING_OP_FILES_UPDATE support choosing fixed file slots")
Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 io_uring/rsrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io_uring/rsrc.c b/io_uring/rsrc.c
index d78e7f2ea91f..cf8c85d1fb59 100644
--- a/io_uring/rsrc.c
+++ b/io_uring/rsrc.c
@@ -705,8 +705,8 @@ static int io_files_update_with_index_alloc(struct io_kiocb *req,
 		if (ret < 0)
 			break;
 		if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
-			ret = -EFAULT;
 			__io_close_fixed(req, issue_flags, ret);
+			ret = -EFAULT;
 			break;
 		}
 	}
-- 
2.25.1

[PATCH 2/5] io_uring: openclose: fix bug of unexpected return value in IORING_CLOSE_FD_AND_FILE_SLOT mode

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

In IORING_CLOSE_FD_AND_FILE_SLOT mode, if we successfully close fixed
file but fails in close->fd >= fdt->max_fds check, cqe-res = 0 is
returned, which should be -EBADF.

Fixes: a7c41b4687f5 ("io_uring: let IORING_OP_FILES_UPDATE support choosing fixed file slots")
Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 io_uring/openclose.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/io_uring/openclose.c b/io_uring/openclose.c
index b35bf5f66dd9..4eb1f23e028a 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -248,7 +248,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_close *close = io_kiocb_to_cmd(req);
 	struct fdtable *fdt;
 	struct file *file;
-	int ret = -EBADF;
+	int ret;
 
 	if (close->file_slot) {
 		ret = io_close_fixed(req, issue_flags);
@@ -256,6 +256,7 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
 			goto err;
 	}
 
+	ret = -EBADF;
 	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
 	if (close->fd >= fdt->max_fds) {
-- 
2.25.1

[PATCH 3/5] io_uring: openclose: support separate return value for IORING_CLOSE_FD_AND_FILE_SLOT

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

In IORING_CLOSE_FD_AND_FILE_SLOT mode, we just stop and return error
code if either of fixed or normal file close fails. But we can actually
continue to do the close even one of them fails. What we need to do is
put the two result in two place: for normal file close, put the result
in cqe->res like the previous behaviour, while for fixed file close,
put it in cqe->flags. Users should check both member to get the status
of fixed and normal file close.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 io_uring/openclose.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/io_uring/openclose.c b/io_uring/openclose.c
index 4eb1f23e028a..da930081c03c 100644
--- a/io_uring/openclose.c
+++ b/io_uring/openclose.c
@@ -248,12 +248,15 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_close *close = io_kiocb_to_cmd(req);
 	struct fdtable *fdt;
 	struct file *file;
-	int ret;
+	int ret, ret2;
 
 	if (close->file_slot) {
 		ret = io_close_fixed(req, issue_flags);
-		if (ret || !(close->flags & IORING_CLOSE_FD_AND_FILE_SLOT))
+		if (!(close->flags & IORING_CLOSE_FD_AND_FILE_SLOT))
 			goto err;
+		else
+			ret2 = ret;
+
 	}
 
 	ret = -EBADF;
@@ -286,6 +289,6 @@ int io_close(struct io_kiocb *req, unsigned int issue_flags)
 err:
 	if (ret < 0)
 		req_set_fail(req);
-	io_req_set_res(req, ret, 0);
+	io_req_set_res(req, ret, ret2);
 	return IOU_OK;
 }
-- 
2.25.1

[PATCH 4/5] io_uring: remove duplicate kbuf release

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

__io_req_complete_put() is called by __io_req_complete_post, and before
that we already put kbuf, no need to do that in __io_req_complete_put()
again.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 io_uring/io_uring.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 1572ebe3cff1..a94ddd0ba507 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -1245,12 +1245,6 @@ static void __io_req_complete_put(struct io_kiocb *req)
 			}
 		}
 		io_req_put_rsrc(req);
-		/*
-		 * Selected buffer deallocation in io_clean_op() assumes that
-		 * we don't hold ->completion_lock. Clean them here to avoid
-		 * deadlocks.
-		 */
-		io_put_kbuf_comp(req);
 		io_dismantle_req(req);
 		io_put_task(req->task, 1);
 		wq_list_add_head(&req->comp_list, &ctx->locked_free_list);
-- 
2.25.1

[PATCH 5/5] io_uring: kbuf: fix bug of not consuming ring buffer in partial io case

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

When we use ring-mapped provided buffer, we should consume it before
arm poll if partial io has been done. Otherwise the buffer may be used
by other requests and thus we lost the data.

Fixes: c7fb19428d67 ("io_uring: add support for ring mapped supplied buffers")
Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 io_uring/kbuf.c |  9 +++++++--
 io_uring/kbuf.h | 10 ++++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/io_uring/kbuf.c b/io_uring/kbuf.c
index d2b2b4728381..0680b63af1d2 100644
--- a/io_uring/kbuf.c
+++ b/io_uring/kbuf.c
@@ -49,8 +49,13 @@ void __io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)
 	 */
 	if (req->flags & REQ_F_BUFFER_RING) {
 		if (req->buf_list) {
-			req->buf_index = req->buf_list->bgid;
-			req->flags &= ~REQ_F_BUFFER_RING;
+			if (req->flags & REQ_F_PARTIAL_IO) {
+				req->buf_list->head++;
+				req->buf_list = NULL;
+			} else {
+				req->buf_index = req->buf_list->bgid;
+				req->flags &= ~REQ_F_BUFFER_RING;
+			}
 		}
 		return;
 	}
diff --git a/io_uring/kbuf.h b/io_uring/kbuf.h
index b58d9d20c97e..9ecb175e60a9 100644
--- a/io_uring/kbuf.h
+++ b/io_uring/kbuf.h
@@ -58,8 +58,14 @@ static inline void io_kbuf_recycle(struct io_kiocb *req, unsigned issue_flags)
 {
 	if (!(req->flags & (REQ_F_BUFFER_SELECTED|REQ_F_BUFFER_RING)))
 		return;
-	/* don't recycle if we already did IO to this buffer */
-	if (req->flags & REQ_F_PARTIAL_IO)
+	/*
+	 * For legacy provided buffer mode, don't recycle if we already did
+	 * IO to this buffer. For ring-mapped provided buffer mode, we should
+	 * increment ring->head to explicitly monopolize the buffer to avoid
+	 * multiple use.
+	 */
+	if ((req->flags & REQ_F_BUFFER_SELECTED) &&
+	    (req->flags & REQ_F_PARTIAL_IO))
 		return;
 	__io_kbuf_recycle(req, issue_flags);
 }
-- 
2.25.1

Re: [PATCH 0/5] misc update for openclose and provided buffer

[Pavel Begunkov]

On 6/10/22 10:07, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> Patch 1 and 2 are bug fixes for openclose
> Patch 3 is to support better close logic
> Patch 4 is code clean
> Patch 5 is a bug fix for ring-mapped provided buffer

Looks that at least the problem from 1/5 also exists in 5.19,
can you split out 5.19 fixes and send them separately?


> Hao Xu (5):
>    io_uring: openclose: fix bug of closing wrong fixed file
>    io_uring: openclose: fix bug of unexpected return value in
>      IORING_CLOSE_FD_AND_FILE_SLOT mode
>    io_uring: openclose: support separate return value for
>      IORING_CLOSE_FD_AND_FILE_SLOT
>    io_uring: remove duplicate kbuf release
>    io_uring: kbuf: consume ring buffer in partial io case
> 
>   io_uring/io_uring.c  |  6 ------
>   io_uring/kbuf.c      |  9 +++++++--
>   io_uring/kbuf.h      | 10 ++++++++--
>   io_uring/openclose.c | 10 +++++++---
>   io_uring/rsrc.c      |  2 +-
>   5 files changed, 23 insertions(+), 14 deletions(-)
> 

-- 
Pavel Begunkov

Re: [PATCH 0/5] misc update for openclose and provided buffer

[Hao Xu]

On 6/10/22 18:13, Pavel Begunkov wrote:
> On 6/10/22 10:07, Hao Xu wrote:
>> From: Hao Xu <howeyxu@tencent.com>
>>
>> Patch 1 and 2 are bug fixes for openclose
>> Patch 3 is to support better close logic
>> Patch 4 is code clean
>> Patch 5 is a bug fix for ring-mapped provided buffer
> 
> Looks that at least the problem from 1/5 also exists in 5.19,
> can you split out 5.19 fixes and send them separately?

Sure.

> 
> 
>> Hao Xu (5):
>>    io_uring: openclose: fix bug of closing wrong fixed file
>>    io_uring: openclose: fix bug of unexpected return value in
>>      IORING_CLOSE_FD_AND_FILE_SLOT mode
>>    io_uring: openclose: support separate return value for
>>      IORING_CLOSE_FD_AND_FILE_SLOT
>>    io_uring: remove duplicate kbuf release
>>    io_uring: kbuf: consume ring buffer in partial io case
>>
>>   io_uring/io_uring.c  |  6 ------
>>   io_uring/kbuf.c      |  9 +++++++--
>>   io_uring/kbuf.h      | 10 ++++++++--
>>   io_uring/openclose.c | 10 +++++++---
>>   io_uring/rsrc.c      |  2 +-
>>   5 files changed, 23 insertions(+), 14 deletions(-)
>>
>