io_uring: submission path cleanup

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c +6 -14

+			io_queue_link_head(head);
+			*link = NULL;
+		}
+	} else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
 		req->flags |= REQ_F_LINK;
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		INIT_LIST_HEAD(&req->link_list);

@@ -3540,10 +3548,8 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 	}
 
 	for (i = 0; i < nr; i++) {
-		struct io_kiocb *req;
-		unsigned int sqe_flags;
+		struct io_kiocb *req = io_get_req(ctx, statep);
 
-		req = io_get_req(ctx, statep);
 		if (unlikely(!req)) {
 			if (!submitted)
 				submitted = -EAGAIN;

@@ -3563,8 +3569,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 		}
 
 		submitted++;
-		sqe_flags = req->sqe->flags;
-
 		req->ring_file = ring_file;
 		req->ring_fd = ring_fd;
 		req->has_user = *mm != NULL;

@@ -3572,14 +3576,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 		req->needs_fixed_file = async;
 		if (!io_submit_sqe(req, statep, &link))
 			break;
-		/*
-		 * If previous wasn't linked and we have a linked command,
-		 * that's the end of the chain. Submit the previous link.
-		 */
-		if (!(sqe_flags & IOSQE_IO_LINK) && link) {
-			io_queue_link_head(link);
-			link = NULL;
-		}
 	}
 
 	if (link)

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Quoted 56 lines · Pavel Begunkov

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

What do you think?

Quoted 69 lines · Jens Axboe

Quoted 56 lines · Pavel Begunkov

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Quoted 5 lines

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Quoted 74 lines · Jens Axboe

Quoted 73 lines · Jens Axboe

Quoted 56 lines · Pavel Begunkov

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

Quoted 5 lines

Quoted 1 line

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Yes, that's the idea. Already got a patch, if you haven't done it yet.

Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use.

Quoted 75 lines · Pavel Begunkov

Quoted 74 lines · Jens Axboe

Quoted 73 lines · Jens Axboe

Quoted 56 lines · Pavel Begunkov

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Yes, that's the idea. Already got a patch, if you haven't done it yet.

I haven't.

Quoted 3 lines

Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use.

Not sure I follow what you're saying here, can you elaborate?

Quoted 83 lines · Jens Axboe

Quoted 75 lines · Pavel Begunkov

Quoted 74 lines · Jens Axboe

Quoted 73 lines · Jens Axboe

Quoted 56 lines · Pavel Begunkov

Quoted 43 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +5 -3

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 
+	sqe_flags = READ_ONCE(req->sqe->flags);
 	req->user_data = READ_ONCE(req->sqe->user_data);
 	trace_io_uring_submit_sqe(ctx, req->user_data, true, req->in_async);
 
 	/* enforce forwards compatibility on users */
-	if (unlikely(req->sqe->flags & ~SQE_VALID_FLAGS)) {
+	if (unlikely(sqe_flags & ~SQE_VALID_FLAGS)) {
 		ret = -EINVAL;
 		goto err_req;
 	}

@@ -3402,10 +3404,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (*link) {
 		struct io_kiocb *head = *link;
 
-		if (req->sqe->flags & IOSQE_IO_DRAIN)
+		if (sqe_flags & IOSQE_IO_DRAIN)
 			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
-		if (req->sqe->flags & IOSQE_IO_HARDLINK)
+		if (sqe_flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
 
 		if (io_alloc_async_ctx(req)) {

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 7 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Yes, that's the idea. Already got a patch, if you haven't done it yet.

Quoted 8 lines

I haven't.

Quoted 3 lines

Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use.

Not sure I follow what you're saying here, can you elaborate?

Sure

#define IOSQE_IO_LINK (1U << 2) /* links next sqe */
#define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */

That's 2 consequent bits, so 4 states:
0,0 -> not a link
1,0 -> common link
0,1 -> hard link
1,1 -> reserved, space for another link-quirk type

But that would require additional check, i.e.

if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ...

Quoted 60 lines · Pavel Begunkov

Quoted 37 lines · Jens Axboe

Quoted 29 lines · Pavel Begunkov

Quoted 28 lines · Jens Axboe

Quoted 27 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov

Review

Quoted 4 lines · Pavel Begunkov

(quoted diff) +3 -1

-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Yes, that's the idea. Already got a patch, if you haven't done it yet.

Quoted 8 lines

I haven't.

Quoted 3 lines

Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use.

Not sure I follow what you're saying here, can you elaborate?

Sure

#define IOSQE_IO_LINK (1U << 2) /* links next sqe */
#define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */

That's 2 consequent bits, so 4 states:
0,0 -> not a link
1,0 -> common link
0,1 -> hard link
1,1 -> reserved, space for another link-quirk type

But that would require additional check, i.e.

Quoted 1 line

if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ...

Ah, I see. In terms of usability, I think it makes more sense to have

IOSQE_LINK | IOSQE_HARDLINK

be the same as just IOSQE_LINK. It would be nice to retain that for

something else, but I think it'll be more confusing to users.

Quoted 61 lines · Jens Axboe

Quoted 60 lines · Pavel Begunkov

Quoted 37 lines · Jens Axboe

Quoted 29 lines · Pavel Begunkov

Quoted 28 lines · Jens Axboe

Quoted 27 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov

Review

Quoted 4 lines · Pavel Begunkov

(quoted diff) +3 -1

-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

This looks suspicious (as well as in the current revision). Returning back to my questions a few days ago can sqe->flags have IOSQE_IO_HARDLINK, but not IOSQE_IO_LINK? I don't find any check.

Quoted 2 lines

In other words, should it be as follows?
!(sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK))

Yeah, I think that should check for both. I'm fine with either approach in general:

- IOSQE_IO_HARDLINK must have IOSQE_IO_LINK set

or

- IOSQE_IO_HARDLINK implies IOSQE_IO_LINK

Seems like the former is easier to verify in terms of functionality, since we can rest easy if we check this early and -EINVAL if that isn't the case.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

What do you think?

If you agree, want to send in a patch for that for 5.5? Then I can respin for-5.6/io_uring on top of that, and we can apply your cleanups there.

Yes, that's the idea. Already got a patch, if you haven't done it yet.

Quoted 8 lines

I haven't.

Quoted 3 lines

Just was thinking, whether to add a check for not setting both flags at the same moment in the "imply" case. Would give us 1 state in 2 bits for future use.

Not sure I follow what you're saying here, can you elaborate?

Sure

#define IOSQE_IO_LINK (1U << 2) /* links next sqe */
#define IOSQE_IO_HARDLINK (1U << 3) /* like LINK, but stronger */

That's 2 consequent bits, so 4 states:
0,0 -> not a link
1,0 -> common link
0,1 -> hard link
1,1 -> reserved, space for another link-quirk type

But that would require additional check, i.e.

Quoted 7 lines

Quoted 1 line

if (flags&(LINK|HARDLINK) == (LINK|HARDLINK)) ...

Ah, I see. In terms of usability, I think it makes more sense to have

IOSQE_LINK | IOSQE_HARDLINK

be the same as just IOSQE_LINK. It would be nice to retain that for

Probably, you meant it to be the same as __IOSQE_HARDLINK__

Quoted 2 lines

something else, but I think it'll be more confusing to users.

Yeah, and it's easier for something like:

sqe->flags |= IOSQE_LINK;
[some code]
if (timer_or_whatever())

	sqe->flags |= IOSQE_HARDLINK;

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +0 -0

index bac9e711e38d..a880ed1409cb 100644

fs/io_uring.c +4 -0

+			io_queue_link_head(head);
+			*link = NULL;
+		}
+	} else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {

Quoted 20 lines · Pavel Begunkov

Move io_queue_link_head() to links handling code in io_submit_sqe(), so it wouldn't need extra checks and would have better data locality.

---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

fs/io_uring.c +1 -0

index bac9e711e38d..a880ed1409cb 100644
@@ -3373,13 +3373,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 			  struct io_kiocb **link)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned int sqe_flags;
 	int ret;
 

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3379

Quoted 1 line

fs/io_uring.c +1 -0

+	sqe_flags = READ_ONCE(req->sqe->flags);

Just out of curiosity, why READ_ONCE it necessary here? I though, that since io_submit_sqes happens within a uring_lock, it's already protected. Do I miss something?

fs/io_uring.c static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state, · line 3428

Quoted 8 lines

fs/io_uring.c +3 -1

@@ -3421,9 +3423,15 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		}
 		trace_io_uring_link(ctx, req, head);
 		list_add_tail(&req->link_list, &head->link_list);
-	} else if (req->sqe->flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
+
+		/* last request of a link, enqueue the link */
+		if (!(sqe_flags & IOSQE_IO_LINK)) {

Yes, as you mentioned in the previous email, it seems correct that if IOSQE_IO_HARDLINK imply IOSQE_IO_LINK, then here we need to check both.

Quoted 4 lines

fs/io_uring.c +4 -0

+			io_queue_link_head(head);
+			*link = NULL;
+		}
+	} else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {

Pretty straighforward cleanups. The last patch saves the exact behaviour, but do link enqueuing from a more suitable place.

Pavel Begunkov (3):
io_uring: rename prev to head
io_uring: move trace_submit_sqe into submit_sqe
io_uring: move *queue_link_head() from common path

 fs/io_uring.c | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

Pretty straighforward cleanups. The last patch saves the exact behaviour, but do link enqueuing from a more suitable place.

v2: rebase

Pavel Begunkov (3):
io_uring: rename prev to head
io_uring: move trace_submit_sqe into submit_sqe
io_uring: move *queue_link_head() from common path

 fs/io_uring.c | 47 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 26 deletions(-)