Skip to contentSkip to conversation

Incrementally consumed buffer fixes

PATCH New Jens Axboe 1 participant 3 messages Mar 19

Subsystems (2)
Stable branch io_uring
lore
Share & tools
lore

Jens Axboe

author maintainer
#0 lore raw

cover Incrementally consumed buffer fixes

Hi,

Michael reported two separate issues with incrementally consumed buffers:

1) In case of EOF, or just a 0-byte read in general, BUF_MORE isn't set. As further completions will happen from the given buffer, it should be set.

2) For non-pollable files, the buffers are consumed upfront, yet BUF_MORE is lost as the commit doesn't happen at completion time.

Fix both of these.

Signature · 1 line
Jens Axboe
author maintainer
#1 lore raw

PATCH 1/2 io_uring/kbuf: fix missing BUF_MORE for incremental buffers at EOF

For a zero length transfer, io_kbuf_inc_commit() is called with !len. Since we never enter the while loop to consume the buffers, io_kbuf_inc_commit() ends up returning true, consuming the buffer. But if no data was consumed, by definition it cannot have consumed the buffer. Return false for that case.

Reported-by Martin Michaelis <[email protected]>
Cc [email protected]
Fixes ae98dbf43d75 ("io_uring/kbuf: add support for incremental buffer consumption")
Link https://github.com/axboe/liburing/issues/1553
Signed-off-by Jens Axboe <[email protected]>
---
 io_uring/kbuf.c | 4 ++++
 1 file changed, 4 insertions(+)
io_uring/kbuf.c +4 -0
index e7f444953dfb..a4cb6752b7aa 100644
@@ -34,6 +34,10 @@ struct io_provide_buf {
 
 static bool io_kbuf_inc_commit(struct io_buffer_list *bl, int len)
 {
+	/* No data consumed, return false early to avoid consuming the buffer */
+	if (!len)
+		return false;
+
 	while (len) {
 		struct io_uring_buf *buf;
 		u32 buf_len, this_len;
Signature · 1 line
2.53.0
author maintainer
#2 lore raw

PATCH 2/2 io_uring/kbuf: propagate BUF_MORE through early buffer commit path

In reply to Jens Axboe

When io_should_commit() returns true (eg for non-pollable files), buffer commit happens at buffer selection time and sel->buf_list is set to NULL. When __io_put_kbufs() generates CQE flags at completion time, it calls __io_put_kbuf_ring() which finds a NULL buffer_list and hence cannot determine whether the buffer was consumed or not. This means that IORING_CQE_F_BUF_MORE is never set for non-pollable input with incrementally consumed buffers.

Likewise for io_buffers_select(), which always commits upfront and discards the return value of io_kbuf_commit().

Add REQ_F_BUF_MORE to store the result of io_kbuf_commit() during early commit. Then __io_put_kbuf_ring() can check this flag and set IORING_F_BUF_MORE accordingy.

Reported-by Martin Michaelis <[email protected]>
Cc [email protected]
Fixes ae98dbf43d75 ("io_uring/kbuf: add support for incremental buffer consumption")
Link https://github.com/axboe/liburing/issues/1553
Signed-off-by Jens Axboe <[email protected]>
---
 include/linux/io_uring_types.h |  3 +++
 io_uring/kbuf.c                | 10 +++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)
include/linux/io_uring_types.h +3 -0
index dd1420bfcb73..214fdbd49052 100644
@@ -541,6 +541,7 @@ enum {
 	REQ_F_BL_NO_RECYCLE_BIT,
 	REQ_F_BUFFERS_COMMIT_BIT,
 	REQ_F_BUF_NODE_BIT,
+	REQ_F_BUF_MORE_BIT,
 	REQ_F_HAS_METADATA_BIT,
 	REQ_F_IMPORT_BUFFER_BIT,
 	REQ_F_SQE_COPIED_BIT,

@@ -626,6 +627,8 @@ enum {
 	REQ_F_BUFFERS_COMMIT	= IO_REQ_FLAG(REQ_F_BUFFERS_COMMIT_BIT),
 	/* buf node is valid */
 	REQ_F_BUF_NODE		= IO_REQ_FLAG(REQ_F_BUF_NODE_BIT),
+	/* incremental buffer consumption, more space available */
+	REQ_F_BUF_MORE		= IO_REQ_FLAG(REQ_F_BUF_MORE_BIT),
 	/* request has read/write metadata assigned */
 	REQ_F_HAS_METADATA	= IO_REQ_FLAG(REQ_F_HAS_METADATA_BIT),
 	/*
io_uring/kbuf.c +7 -3
index a4cb6752b7aa..f72f38d22d2b 100644
@@ -216,7 +216,8 @@ static struct io_br_sel io_ring_buffer_select(struct io_kiocb *req, size_t *len,
 	sel.addr = u64_to_user_ptr(READ_ONCE(buf->addr));
 
 	if (io_should_commit(req, issue_flags)) {
-		io_kbuf_commit(req, sel.buf_list, *len, 1);
+		if (!io_kbuf_commit(req, sel.buf_list, *len, 1))
+			req->flags |= REQ_F_BUF_MORE;
 		sel.buf_list = NULL;
 	}
 	return sel;

@@ -349,7 +350,8 @@ int io_buffers_select(struct io_kiocb *req, struct buf_sel_arg *arg,
 		 */
 		if (ret > 0) {
 			req->flags |= REQ_F_BUFFERS_COMMIT | REQ_F_BL_NO_RECYCLE;
-			io_kbuf_commit(req, sel->buf_list, arg->out_len, ret);
+			if (!io_kbuf_commit(req, sel->buf_list, arg->out_len, ret))
+				req->flags |= REQ_F_BUF_MORE;
 		}
 	} else {
 		ret = io_provided_buffers_select(req, &arg->out_len, sel->buf_list, arg->iovs);

@@ -395,8 +397,10 @@ static inline bool __io_put_kbuf_ring(struct io_kiocb *req,
 
 	if (bl)
 		ret = io_kbuf_commit(req, bl, len, nr);
+	if (ret && (req->flags & REQ_F_BUF_MORE))
+		ret = false;
 
-	req->flags &= ~REQ_F_BUFFER_RING;
+	req->flags &= ~(REQ_F_BUFFER_RING | REQ_F_BUF_MORE);
 	return ret;
 }
Signature · 1 line
2.53.0