[PATCH 0/3] io_uring: submission path cleanup

[PATCH 0/3] io_uring: submission path cleanup

[Pavel Begunkov]

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(-)

-- 
2.24.0

[PATCH 1/3] io_uring: rename prev to head

[Pavel Begunkov]

Calling "prev" a head of a link is a bit misleading. Rename it

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 339b57aac5ca..96ddfc52cb0f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3399,10 +3399,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	 * conditions are true (normal request), then just queue it.
 	 */
 	if (*link) {
-		struct io_kiocb *prev = *link;
+		struct io_kiocb *head = *link;
 
 		if (req->sqe->flags & IOSQE_IO_DRAIN)
-			(*link)->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
+			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
 		if (req->sqe->flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
@@ -3415,11 +3415,11 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		ret = io_req_defer_prep(req);
 		if (ret) {
 			/* fail even hard links since we don't submit */
-			prev->flags |= REQ_F_FAIL_LINK;
+			head->flags |= REQ_F_FAIL_LINK;
 			goto err_req;
 		}
-		trace_io_uring_link(ctx, req, prev);
-		list_add_tail(&req->link_list, &prev->link_list);
+		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)) {
 		req->flags |= REQ_F_LINK;
 		if (req->sqe->flags & IOSQE_IO_HARDLINK)
-- 
2.24.0

[PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe

[Pavel Begunkov]

For better locality, call trace_io_uring_submit_sqe() from submit_sqe()
rather than io_submit_sqes(). No functional change.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 96ddfc52cb0f..bac9e711e38d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3375,7 +3375,8 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
-	req->user_data = req->sqe->user_data;
+	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)) {
@@ -3569,8 +3570,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 		req->has_user = *mm != NULL;
 		req->in_async = async;
 		req->needs_fixed_file = async;
-		trace_io_uring_submit_sqe(ctx, req->sqe->user_data,
-					  true, async);
 		if (!io_submit_sqe(req, statep, &link))
 			break;
 		/*
-- 
2.24.0

[PATCH 3/3] io_uring: move *queue_link_head() from common path

[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 <asml.silence@gmail.com>
---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bac9e711e38d..a880ed1409cb 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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)) {
@@ -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)) {
+			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)
-- 
2.24.0

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 3749 bytes --]

On 17/12/2019 02:22, Pavel Begunkov wrote:
> 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 <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bac9e711e38d..a880ed1409cb 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -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)) {
> @@ -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))

> +			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)
> 

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Dmitry Dolgov]

> On Tue, Dec 17, 2019 at 02:22:09AM +0300, Pavel Begunkov wrote:
>
> 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(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bac9e711e38d..a880ed1409cb 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -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);

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?

> @@ -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.

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

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov]

On 12/17/2019 5:00 PM, Dmitry Dolgov wrote:
>> On Tue, Dec 17, 2019 at 02:22:09AM +0300, Pavel Begunkov wrote:
>>
>> 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(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index bac9e711e38d..a880ed1409cb 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -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);
> 
> 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?
> 
SQEs are rw-shared with the userspace, that's it. Probably, there are
more places where proper READ_ONCE() annotations have been lost.

>> @@ -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.
> 
>> +			io_queue_link_head(head);
>> +			*link = NULL;
>> +		}
>> +	} else if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {

-- 
Pavel Begunkov

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Jens Axboe]

On 12/16/19 4:38 PM, Pavel Begunkov wrote:
> On 17/12/2019 02:22, Pavel Begunkov wrote:
>> 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 <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index bac9e711e38d..a880ed1409cb 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -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)) {
>> @@ -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))

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?

-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Jens Axboe]

On 12/17/19 9:45 AM, Jens Axboe wrote:
> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>> 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 <asml.silence@gmail.com>
>>> ---
>>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index bac9e711e38d..a880ed1409cb 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -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)) {
>>> @@ -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))
> 
> 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?

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.

-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 3409 bytes --]

On 17/12/2019 20:37, Jens Axboe wrote:
> On 12/17/19 9:45 AM, Jens Axboe wrote:
>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>> 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 <asml.silence@gmail.com>
>>>> ---
>>>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index bac9e711e38d..a880ed1409cb 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -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)) {
>>>> @@ -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))
>>
>> 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?
> 
> 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.

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Jens Axboe]

On 12/17/19 10:52 AM, Pavel Begunkov wrote:
> On 17/12/2019 20:37, Jens Axboe wrote:
>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>> 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 <asml.silence@gmail.com>
>>>>> ---
>>>>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>>>
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index bac9e711e38d..a880ed1409cb 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -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)) {
>>>>> @@ -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))
>>>
>>> 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?
>>
>> 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.

> 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?


-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 4147 bytes --]

On 17/12/2019 21:01, Jens Axboe wrote:
> On 12/17/19 10:52 AM, Pavel Begunkov wrote:
>> On 17/12/2019 20:37, Jens Axboe wrote:
>>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>>> 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 <asml.silence@gmail.com>
>>>>>> ---
>>>>>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>>>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>> index bac9e711e38d..a880ed1409cb 100644
>>>>>> --- a/fs/io_uring.c
>>>>>> +++ b/fs/io_uring.c
>>>>>> @@ -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)) {
>>>>>> @@ -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))
>>>>
>>>> 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?
>>>
>>> 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.
> 
>> 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)) ...


-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Jens Axboe]

On 12/17/19 11:05 AM, Pavel Begunkov wrote:
> On 17/12/2019 21:01, Jens Axboe wrote:
>> On 12/17/19 10:52 AM, Pavel Begunkov wrote:
>>> On 17/12/2019 20:37, Jens Axboe wrote:
>>>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>>>> 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 <asml.silence@gmail.com>
>>>>>>> ---
>>>>>>>  fs/io_uring.c | 32 ++++++++++++++------------------
>>>>>>>  1 file changed, 14 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>>> index bac9e711e38d..a880ed1409cb 100644
>>>>>>> --- a/fs/io_uring.c
>>>>>>> +++ b/fs/io_uring.c
>>>>>>> @@ -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)) {
>>>>>>> @@ -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))
>>>>>
>>>>> 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?
>>>>
>>>> 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.
>>
>>> 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)) ...

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.

-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 2828 bytes --]

On 17/12/2019 21:07, Jens Axboe wrote:
> On 12/17/19 11:05 AM, Pavel Begunkov wrote:
>> On 17/12/2019 21:01, Jens Axboe wrote:
>>> On 12/17/19 10:52 AM, Pavel Begunkov wrote:
>>>> On 17/12/2019 20:37, Jens Axboe wrote:
>>>>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>>>>> -	} 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))
>>>>>>
>>>>>> 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?
>>>>>
>>>>> 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.
>>>
>>>> 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)) ...
> 
> 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__

> 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;

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] io_uring: submission path cleanup

[Jens Axboe]

On 12/16/19 4:22 PM, Pavel Begunkov wrote:
> 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(-)

Can you respin this on top of the hardlink patch?

-- 
Jens Axboe

Re: [PATCH 3/3] io_uring: move *queue_link_head() from common path

[Jens Axboe]

On 12/17/19 11:12 AM, Pavel Begunkov wrote:
> On 17/12/2019 21:07, Jens Axboe wrote:
>> On 12/17/19 11:05 AM, Pavel Begunkov wrote:
>>> On 17/12/2019 21:01, Jens Axboe wrote:
>>>> On 12/17/19 10:52 AM, Pavel Begunkov wrote:
>>>>> On 17/12/2019 20:37, Jens Axboe wrote:
>>>>>> On 12/17/19 9:45 AM, Jens Axboe wrote:
>>>>>>> On 12/16/19 4:38 PM, Pavel Begunkov wrote:
>>>>>>>> On 17/12/2019 02:22, Pavel Begunkov wrote:
>>>>>>>>> -	} 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))
>>>>>>>
>>>>>>> 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?
>>>>>>
>>>>>> 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.
>>>>
>>>>> 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)) ...
>>
>> 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__
> 
>> 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;

Precisely. So let's keep it as-is.


-- 
Jens Axboe

[PATCH v2 0/3] io_uring: submission path cleanup

[Pavel Begunkov]

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(-)

-- 
2.24.0

[PATCH 1/3] io_uring: rename prev to head

[Pavel Begunkov]

Calling "prev" a head of a link is a bit misleading. Rename it

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index eb6d897ea087..e8ce224dc82c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3399,10 +3399,10 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	 * conditions are true (normal request), then just queue it.
 	 */
 	if (*link) {
-		struct io_kiocb *prev = *link;
+		struct io_kiocb *head = *link;
 
 		if (req->sqe->flags & IOSQE_IO_DRAIN)
-			(*link)->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
+			head->flags |= REQ_F_DRAIN_LINK | REQ_F_IO_DRAIN;
 
 		if (req->sqe->flags & IOSQE_IO_HARDLINK)
 			req->flags |= REQ_F_HARDLINK;
@@ -3415,11 +3415,11 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 		ret = io_req_defer_prep(req);
 		if (ret) {
 			/* fail even hard links since we don't submit */
-			prev->flags |= REQ_F_FAIL_LINK;
+			head->flags |= REQ_F_FAIL_LINK;
 			goto err_req;
 		}
-		trace_io_uring_link(ctx, req, prev);
-		list_add_tail(&req->link_list, &prev->link_list);
+		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)) {
 		req->flags |= REQ_F_LINK;
 		if (req->sqe->flags & IOSQE_IO_HARDLINK)
-- 
2.24.0

[PATCH 2/3] io_uring: move trace_submit_sqe into submit_sqe

[Pavel Begunkov]

For better locality, call trace_io_uring_submit_sqe() from submit_sqe()
rather than io_submit_sqes(). No functional change.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e8ce224dc82c..ee461bcd3121 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3375,7 +3375,8 @@ static bool io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
-	req->user_data = req->sqe->user_data;
+	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)) {
@@ -3569,8 +3570,6 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 		req->has_user = *mm != NULL;
 		req->in_async = async;
 		req->needs_fixed_file = async;
-		trace_io_uring_submit_sqe(ctx, req->sqe->user_data,
-					  true, async);
 		if (!io_submit_sqe(req, statep, &link))
 			break;
 		/*
-- 
2.24.0

[PATCH v2 3/3] io_uring: move *queue_link_head() from common path

[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 <asml.silence@gmail.com>
---
 fs/io_uring.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ee461bcd3121..8ab96ca0ad28 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -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)) {
@@ -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|IOSQE_IO_HARDLINK))) {
+			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|IOSQE_IO_HARDLINK)) && link) {
-			io_queue_link_head(link);
-			link = NULL;
-		}
 	}
 
 	if (link)
-- 
2.24.0

Re: [PATCH v2 0/3] io_uring: submission path cleanup

[Jens Axboe]

On 12/17/19 12:26 PM, Pavel Begunkov wrote:
> 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(-)

Applied, thanks.

-- 
Jens Axboe