Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
@ 2019-10-09  1:19 Jackie Liu
  2019-10-09  1:19 ` [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async Jackie Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Jackie Liu @ 2019-10-09  1:19 UTC (permalink / raw)
  To: axboe; +Cc: linux-block

__io_get_deferred_req is used to get all defer lists, including defer_list
and timeout_list, but io_sequence_defer should be only cares about the sequence.

Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
 fs/io_uring.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8a0381f1a43b..8ec2443eb019 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
 				     struct io_kiocb *req)
 {
-	/* timeout requests always honor sequence */
-	if (!(req->flags & REQ_F_TIMEOUT) &&
-	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
+	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
 		return false;
 
 	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
@@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
 		return NULL;
 
 	req = list_first_entry(list, struct io_kiocb, list);
-	if (!io_sequence_defer(ctx, req)) {
-		list_del_init(&req->list);
-		return req;
-	}
+	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
+		return NULL;
 
-	return NULL;
+	list_del_init(&req->list);
+	return req;
 }
 
 static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
-- 
2.23.0




^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async
  2019-10-09  1:19 [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jackie Liu
@ 2019-10-09  1:19 ` Jackie Liu
  2019-10-10 16:10   ` Jens Axboe
  2019-10-10 16:10 ` [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jens Axboe
  2019-10-11  2:24 ` yangerkun
  2 siblings, 1 reply; 15+ messages in thread
From: Jackie Liu @ 2019-10-09  1:19 UTC (permalink / raw)
  To: axboe; +Cc: linux-block

There is no function change, just to clean up the code, use s->in_async
to make the code know where it is.

Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
---
 fs/io_uring.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8ec2443eb019..3bb638b26cb7 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -268,7 +268,7 @@ struct sqe_submit {
 	unsigned short			index;
 	u32				sequence;
 	bool				has_user;
-	bool				needs_lock;
+	bool				in_async;
 	bool				needs_fixed_file;
 };
 
@@ -1390,11 +1390,7 @@ static int io_read(struct io_kiocb *req, const struct sqe_submit *s,
 		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)
+			if (!s->in_async)
 				io_async_list_note(READ, req, iov_count);
 			ret = -EAGAIN;
 		}
@@ -1432,8 +1428,7 @@ static int io_write(struct io_kiocb *req, const struct sqe_submit *s,
 
 	ret = -EAGAIN;
 	if (force_nonblock && !(kiocb->ki_flags & IOCB_DIRECT)) {
-		/* If ->needs_lock is true, we're already in async context. */
-		if (!s->needs_lock)
+		if (!s->in_async)
 			io_async_list_note(WRITE, req, iov_count);
 		goto out_free;
 	}
@@ -1464,11 +1459,7 @@ static int io_write(struct io_kiocb *req, const struct sqe_submit *s,
 		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)
+			if (!s->in_async)
 				io_async_list_note(WRITE, req, iov_count);
 			ret = -EAGAIN;
 		}
@@ -2029,10 +2020,10 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 			return -EAGAIN;
 
 		/* workqueue context doesn't hold uring_lock, grab it now */
-		if (s->needs_lock)
+		if (s->in_async)
 			mutex_lock(&ctx->uring_lock);
 		io_iopoll_req_issued(req);
-		if (s->needs_lock)
+		if (s->in_async)
 			mutex_unlock(&ctx->uring_lock);
 	}
 
@@ -2096,7 +2087,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 
 		if (!ret) {
 			s->has_user = cur_mm != NULL;
-			s->needs_lock = true;
+			s->in_async = true;
 			do {
 				ret = __io_submit_sqe(ctx, req, s, false);
 				/*
@@ -2552,7 +2543,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
 						-EFAULT);
 		} else {
 			sqes[i].has_user = has_user;
-			sqes[i].needs_lock = true;
+			sqes[i].in_async = true;
 			sqes[i].needs_fixed_file = true;
 			io_submit_sqe(ctx, &sqes[i], statep, &link, true);
 			submitted++;
@@ -2738,7 +2729,7 @@ static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit,
 
 out:
 		s.has_user = true;
-		s.needs_lock = false;
+		s.in_async = false;
 		s.needs_fixed_file = false;
 		submit++;
 
-- 
2.23.0




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-09  1:19 [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jackie Liu
  2019-10-09  1:19 ` [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async Jackie Liu
@ 2019-10-10 16:10 ` Jens Axboe
  2019-10-11  2:24 ` yangerkun
  2 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2019-10-10 16:10 UTC (permalink / raw)
  To: Jackie Liu; +Cc: linux-block

On 10/8/19 7:19 PM, Jackie Liu wrote:
> __io_get_deferred_req is used to get all defer lists, including defer_list
> and timeout_list, but io_sequence_defer should be only cares about the sequence.

Applied, but modified to keep commit message at 72 chars wide.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async
  2019-10-09  1:19 ` [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async Jackie Liu
@ 2019-10-10 16:10   ` Jens Axboe
  0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2019-10-10 16:10 UTC (permalink / raw)
  To: Jackie Liu; +Cc: linux-block

On 10/8/19 7:19 PM, Jackie Liu wrote:
> There is no function change, just to clean up the code, use s->in_async
> to make the code know where it is.

This seems to be a somewhat older code base, as it doesn't have
the changes done post 5.3 (io_rw_done -> kiocb_done, for example).
I hand applied it, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-09  1:19 [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jackie Liu
  2019-10-09  1:19 ` [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async Jackie Liu
  2019-10-10 16:10 ` [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jens Axboe
@ 2019-10-11  2:24 ` yangerkun
  2019-10-11  2:35   ` Jens Axboe
  2 siblings, 1 reply; 15+ messages in thread
From: yangerkun @ 2019-10-11  2:24 UTC (permalink / raw)
  To: Jackie Liu, axboe; +Cc: linux-block



On 2019/10/9 9:19, Jackie Liu wrote:
> __io_get_deferred_req is used to get all defer lists, including defer_list
> and timeout_list, but io_sequence_defer should be only cares about the sequence.
> 
> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
> ---
>   fs/io_uring.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 8a0381f1a43b..8ec2443eb019 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>   static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>   				     struct io_kiocb *req)
>   {
> -	/* timeout requests always honor sequence */
> -	if (!(req->flags & REQ_F_TIMEOUT) &&
> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>   		return false;
>   
>   	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>   		return NULL;
>   
>   	req = list_first_entry(list, struct io_kiocb, list);
> -	if (!io_sequence_defer(ctx, req)) {
> -		list_del_init(&req->list);
> -		return req;
> -	}
> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
> +		return NULL;
Hi,

For timeout req, we should also compare the sequence to determine to 
return NULL or the req. But now we will return req directly. Actually, 
no need to compare req->flags with REQ_F_TIMEOUT.

Another problem, before this change, a timeout req can also drain the 
sqe(io_queue_sqe->io_req_defer and add to refer list), i am not sure is 
it a right or wrong logic, but your patch fix that.

Thanks,
Kun.
>   
> -	return NULL;
> +	list_del_init(&req->list);
> +	return req;
>   }
>   
>   static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  2:24 ` yangerkun
@ 2019-10-11  2:35   ` Jens Axboe
  2019-10-11  3:06     ` Jackie Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  2:35 UTC (permalink / raw)
  To: yangerkun, Jackie Liu; +Cc: linux-block

On 10/10/19 8:24 PM, yangerkun wrote:
> 
> 
> On 2019/10/9 9:19, Jackie Liu wrote:
>> __io_get_deferred_req is used to get all defer lists, including defer_list
>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>
>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>> ---
>>    fs/io_uring.c | 13 +++++--------
>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 8a0381f1a43b..8ec2443eb019 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>    				     struct io_kiocb *req)
>>    {
>> -	/* timeout requests always honor sequence */
>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>    		return false;
>>    
>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>    		return NULL;
>>    
>>    	req = list_first_entry(list, struct io_kiocb, list);
>> -	if (!io_sequence_defer(ctx, req)) {
>> -		list_del_init(&req->list);
>> -		return req;
>> -	}
>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>> +		return NULL;
> Hi,
> 
> For timeout req, we should also compare the sequence to determine to
> return NULL or the req. But now we will return req directly. Actually,
> no need to compare req->flags with REQ_F_TIMEOUT.

Yes, not sure how I missed this, the patch is broken as-is. I will kill
it, cleanup can be done differently.

> Another problem, before this change, a timeout req can also drain the
> sqe(io_queue_sqe->io_req_defer and add to refer list), i am not sure is
> it a right or wrong logic, but your patch fix that.

We should factor the cq_timeouts into that.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  2:35   ` Jens Axboe
@ 2019-10-11  3:06     ` Jackie Liu
  2019-10-11  3:17       ` Jens Axboe
  0 siblings, 1 reply; 15+ messages in thread
From: Jackie Liu @ 2019-10-11  3:06 UTC (permalink / raw)
  To: Jens Axboe; +Cc: yangerkun, linux-block



> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
> 
> On 10/10/19 8:24 PM, yangerkun wrote:
>> 
>> 
>> On 2019/10/9 9:19, Jackie Liu wrote:
>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>> 
>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>> ---
>>>   fs/io_uring.c | 13 +++++--------
>>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index 8a0381f1a43b..8ec2443eb019 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>   static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>   				     struct io_kiocb *req)
>>>   {
>>> -	/* timeout requests always honor sequence */
>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>   		return false;
>>> 
>>>   	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>   		return NULL;
>>> 
>>>   	req = list_first_entry(list, struct io_kiocb, list);
>>> -	if (!io_sequence_defer(ctx, req)) {
>>> -		list_del_init(&req->list);
>>> -		return req;
>>> -	}
>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>> +		return NULL;
>> Hi,
>> 
>> For timeout req, we should also compare the sequence to determine to
>> return NULL or the req. But now we will return req directly. Actually,
>> no need to compare req->flags with REQ_F_TIMEOUT.
> 
> Yes, not sure how I missed this, the patch is broken as-is. I will kill
> it, cleanup can be done differently.

Sorry for miss it, I don't wanna change the logic, it's not my original meaning.
Personal opinion, timeout list should not be mixed with defer_list, which makes
the code more complicated and difficult to understand.

--
BR, Jackie Liu




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:06     ` Jackie Liu
@ 2019-10-11  3:17       ` Jens Axboe
  2019-10-11  3:26         ` Jens Axboe
  2019-10-11  3:27         ` Jackie Liu
  0 siblings, 2 replies; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  3:17 UTC (permalink / raw)
  To: Jackie Liu; +Cc: yangerkun, linux-block

On 10/10/19 9:06 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>
>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>
>>>
>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>
>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>> ---
>>>>    fs/io_uring.c | 13 +++++--------
>>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>    				     struct io_kiocb *req)
>>>>    {
>>>> -	/* timeout requests always honor sequence */
>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>    		return false;
>>>>
>>>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>    		return NULL;
>>>>
>>>>    	req = list_first_entry(list, struct io_kiocb, list);
>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>> -		list_del_init(&req->list);
>>>> -		return req;
>>>> -	}
>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>> +		return NULL;
>>> Hi,
>>>
>>> For timeout req, we should also compare the sequence to determine to
>>> return NULL or the req. But now we will return req directly. Actually,
>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>
>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>> it, cleanup can be done differently.
> 
> Sorry for miss it, I don't wanna change the logic, it's not my
> original meaning.

No worries, mistakes happen.

> Personal opinion, timeout list should not be mixed with defer_list,
> which makes the code more complicated and difficult to understand.

Not sure why you feel they are mixed? They are in separate lists, but
they share using the sequence logic. In that respet they are really not
that different, the sequence will trigger either one of them. Either as
a timeout, or as a "can now be issued". Hence the code handling them is
both shared and identical.

I do agree that the check could be cleaner, which is probably how the
mistake in this patch happened in the first place.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:17       ` Jens Axboe
@ 2019-10-11  3:26         ` Jens Axboe
  2019-10-11  3:27         ` Jackie Liu
  1 sibling, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  3:26 UTC (permalink / raw)
  To: Jackie Liu; +Cc: yangerkun, linux-block

On 10/10/19 9:17 PM, Jens Axboe wrote:
> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>
>>
>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>
>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>
>>>>
>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>
>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>> ---
>>>>>     fs/io_uring.c | 13 +++++--------
>>>>>     1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>     static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>     				     struct io_kiocb *req)
>>>>>     {
>>>>> -	/* timeout requests always honor sequence */
>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>     		return false;
>>>>>
>>>>>     	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>     		return NULL;
>>>>>
>>>>>     	req = list_first_entry(list, struct io_kiocb, list);
>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>> -		list_del_init(&req->list);
>>>>> -		return req;
>>>>> -	}
>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>> +		return NULL;
>>>> Hi,
>>>>
>>>> For timeout req, we should also compare the sequence to determine to
>>>> return NULL or the req. But now we will return req directly. Actually,
>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>
>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>> it, cleanup can be done differently.
>>
>> Sorry for miss it, I don't wanna change the logic, it's not my
>> original meaning.
> 
> No worries, mistakes happen.
> 
>> Personal opinion, timeout list should not be mixed with defer_list,
>> which makes the code more complicated and difficult to understand.
> 
> Not sure why you feel they are mixed? They are in separate lists, but
> they share using the sequence logic. In that respet they are really not
> that different, the sequence will trigger either one of them. Either as
> a timeout, or as a "can now be issued". Hence the code handling them is
> both shared and identical.
> 
> I do agree that the check could be cleaner, which is probably how the
> mistake in this patch happened in the first place.

I think we should just make it clear if the sequence checking is for
one of the paths - we don't want to defer anything based on a timeout,
just the timeout itself. That will also take care of the issue that
yangerkun brought up.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:17       ` Jens Axboe
  2019-10-11  3:26         ` Jens Axboe
@ 2019-10-11  3:27         ` Jackie Liu
  2019-10-11  3:34           ` Jens Axboe
  1 sibling, 1 reply; 15+ messages in thread
From: Jackie Liu @ 2019-10-11  3:27 UTC (permalink / raw)
  To: Jens Axboe; +Cc: yangerkun, linux-block



> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
> 
> On 10/10/19 9:06 PM, Jackie Liu wrote:
>> 
>> 
>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>> 
>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>> 
>>>> 
>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>> 
>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>> ---
>>>>>   fs/io_uring.c | 13 +++++--------
>>>>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>>>> 
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>   static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>   				     struct io_kiocb *req)
>>>>>   {
>>>>> -	/* timeout requests always honor sequence */
>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>   		return false;
>>>>> 
>>>>>   	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>   		return NULL;
>>>>> 
>>>>>   	req = list_first_entry(list, struct io_kiocb, list);
>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>> -		list_del_init(&req->list);
>>>>> -		return req;
>>>>> -	}
>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>> +		return NULL;
>>>> Hi,
>>>> 
>>>> For timeout req, we should also compare the sequence to determine to
>>>> return NULL or the req. But now we will return req directly. Actually,
>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>> 
>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>> it, cleanup can be done differently.
>> 
>> Sorry for miss it, I don't wanna change the logic, it's not my
>> original meaning.
> 
> No worries, mistakes happen.
> 
>> Personal opinion, timeout list should not be mixed with defer_list,
>> which makes the code more complicated and difficult to understand.
> 
> Not sure why you feel they are mixed? They are in separate lists, but
> they share using the sequence logic. In that respet they are really not
> that different, the sequence will trigger either one of them. Either as
> a timeout, or as a "can now be issued". Hence the code handling them is
> both shared and identical.

I not sure, I think I need reread the code of timeout command.

> 
> I do agree that the check could be cleaner, which is probably how the
> mistake in this patch happened in the first place.
> 

Yes, I agree with you. io_sequence_defer should be only cares about the sequence.
Thanks for point out this mistake.

--
BR, Jackie Liu




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:27         ` Jackie Liu
@ 2019-10-11  3:34           ` Jens Axboe
  2019-10-11  3:41             ` Jackie Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  3:34 UTC (permalink / raw)
  To: Jackie Liu; +Cc: yangerkun, linux-block

On 10/10/19 9:27 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
>>
>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>
>>>
>>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>>
>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>
>>>>>
>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>
>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>> ---
>>>>>>    fs/io_uring.c | 13 +++++--------
>>>>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>> --- a/fs/io_uring.c
>>>>>> +++ b/fs/io_uring.c
>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>    				     struct io_kiocb *req)
>>>>>>    {
>>>>>> -	/* timeout requests always honor sequence */
>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>    		return false;
>>>>>>
>>>>>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>    		return NULL;
>>>>>>
>>>>>>    	req = list_first_entry(list, struct io_kiocb, list);
>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>> -		list_del_init(&req->list);
>>>>>> -		return req;
>>>>>> -	}
>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>> +		return NULL;
>>>>> Hi,
>>>>>
>>>>> For timeout req, we should also compare the sequence to determine to
>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>
>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>> it, cleanup can be done differently.
>>>
>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>> original meaning.
>>
>> No worries, mistakes happen.
>>
>>> Personal opinion, timeout list should not be mixed with defer_list,
>>> which makes the code more complicated and difficult to understand.
>>
>> Not sure why you feel they are mixed? They are in separate lists, but
>> they share using the sequence logic. In that respet they are really not
>> that different, the sequence will trigger either one of them. Either as
>> a timeout, or as a "can now be issued". Hence the code handling them is
>> both shared and identical.
> 
> I not sure, I think I need reread the code of timeout command.
> 
>>
>> I do agree that the check could be cleaner, which is probably how the
>> mistake in this patch happened in the first place.
>>
> 
> Yes, I agree with you. io_sequence_defer should be only cares about
> the sequence.  Thanks for point out this mistake.

How about something like this? More cleanly separates them to avoid
mixing flags. The regular defer code will honor the flags, the timeout
code will just bypass those.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index c92cb09cc262..4a2bb81cb1f1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	return ctx;
 }
 
+static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
+				       struct io_kiocb *req)
+{
+	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
+}
+
 static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
 				     struct io_kiocb *req)
 {
-	/* timeout requests always honor sequence */
-	if (!(req->flags & REQ_F_TIMEOUT) &&
-	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
+	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
 		return false;
 
-	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
+	return __io_sequence_defer(ctx, req);
 }
 
-static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
-					      struct list_head *list)
+static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
 {
 	struct io_kiocb *req;
 
-	if (list_empty(list))
+	if (list_empty(&ctx->defer_list))
 		return NULL;
 
-	req = list_first_entry(list, struct io_kiocb, list);
+	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
 	if (!io_sequence_defer(ctx, req)) {
 		list_del_init(&req->list);
 		return req;
@@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
 	return NULL;
 }
 
-static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
-{
-	return __io_get_deferred_req(ctx, &ctx->defer_list);
-}
-
 static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
 {
-	return __io_get_deferred_req(ctx, &ctx->timeout_list);
+	struct io_kiocb *req;
+
+	if (list_empty(&ctx->timeout_list))
+		return NULL;
+
+	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
+	if (!__io_sequence_defer(ctx, req)) {
+		list_del_init(&req->list);
+		return req;
+	}
+
+	return NULL;
 }
 
 static void __io_commit_cqring(struct io_ring_ctx *ctx)

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:34           ` Jens Axboe
@ 2019-10-11  3:41             ` Jackie Liu
  2019-10-11  3:42               ` Jens Axboe
  0 siblings, 1 reply; 15+ messages in thread
From: Jackie Liu @ 2019-10-11  3:41 UTC (permalink / raw)
  To: Jens Axboe; +Cc: yangerkun, linux-block



> 2019年10月11日 11:34,Jens Axboe <axboe@kernel.dk> 写道:
> 
> On 10/10/19 9:27 PM, Jackie Liu wrote:
>> 
>> 
>>> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
>>> 
>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>> 
>>>> 
>>>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>>> 
>>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>> 
>>>>>> 
>>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>> 
>>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>>> ---
>>>>>>>   fs/io_uring.c | 13 +++++--------
>>>>>>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>>> --- a/fs/io_uring.c
>>>>>>> +++ b/fs/io_uring.c
>>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>>   static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>>   				     struct io_kiocb *req)
>>>>>>>   {
>>>>>>> -	/* timeout requests always honor sequence */
>>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>   		return false;
>>>>>>> 
>>>>>>>   	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>>   		return NULL;
>>>>>>> 
>>>>>>>   	req = list_first_entry(list, struct io_kiocb, list);
>>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>>> -		list_del_init(&req->list);
>>>>>>> -		return req;
>>>>>>> -	}
>>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>>> +		return NULL;
>>>>>> Hi,
>>>>>> 
>>>>>> For timeout req, we should also compare the sequence to determine to
>>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>> 
>>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>>> it, cleanup can be done differently.
>>>> 
>>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>>> original meaning.
>>> 
>>> No worries, mistakes happen.
>>> 
>>>> Personal opinion, timeout list should not be mixed with defer_list,
>>>> which makes the code more complicated and difficult to understand.
>>> 
>>> Not sure why you feel they are mixed? They are in separate lists, but
>>> they share using the sequence logic. In that respet they are really not
>>> that different, the sequence will trigger either one of them. Either as
>>> a timeout, or as a "can now be issued". Hence the code handling them is
>>> both shared and identical.
>> 
>> I not sure, I think I need reread the code of timeout command.
>> 
>>> 
>>> I do agree that the check could be cleaner, which is probably how the
>>> mistake in this patch happened in the first place.
>>> 
>> 
>> Yes, I agree with you. io_sequence_defer should be only cares about
>> the sequence.  Thanks for point out this mistake.
> 
> How about something like this? More cleanly separates them to avoid
> mixing flags. The regular defer code will honor the flags, the timeout
> code will just bypass those.
> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c92cb09cc262..4a2bb81cb1f1 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
> 	return ctx;
> }
> 
> +static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
> +				       struct io_kiocb *req)
> +{
> +	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
> +}
> +
> static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
> 				     struct io_kiocb *req)
> {
> -	/* timeout requests always honor sequence */
> -	if (!(req->flags & REQ_F_TIMEOUT) &&
> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
> 		return false;
> 
> -	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
> +	return __io_sequence_defer(ctx, req);
> }
> 
> -static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
> -					      struct list_head *list)
> +static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
> {
> 	struct io_kiocb *req;
> 
> -	if (list_empty(list))
> +	if (list_empty(&ctx->defer_list))
> 		return NULL;
> 
> -	req = list_first_entry(list, struct io_kiocb, list);
> +	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
> 	if (!io_sequence_defer(ctx, req)) {
> 		list_del_init(&req->list);
> 		return req;
> @@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
> 	return NULL;
> }
> 
> -static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
> -{
> -	return __io_get_deferred_req(ctx, &ctx->defer_list);
> -}
> -
> static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
> {
> -	return __io_get_deferred_req(ctx, &ctx->timeout_list);
> +	struct io_kiocb *req;
> +
> +	if (list_empty(&ctx->timeout_list))
> +		return NULL;
> +
> +	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
> +	if (!__io_sequence_defer(ctx, req)) {
> +		list_del_init(&req->list);
> +		return req;
> +	}
> +
> +	return NULL;
> }
> 
> static void __io_commit_cqring(struct io_ring_ctx *ctx)
> 

Much better, clearly and easy understand.

--
BR, Jackie Liu




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:41             ` Jackie Liu
@ 2019-10-11  3:42               ` Jens Axboe
  2019-10-11  3:43                 ` Jackie Liu
  0 siblings, 1 reply; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  3:42 UTC (permalink / raw)
  To: Jackie Liu; +Cc: yangerkun, linux-block

On 10/10/19 9:41 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月11日 11:34,Jens Axboe <axboe@kernel.dk> 写道:
>>
>> On 10/10/19 9:27 PM, Jackie Liu wrote:
>>>
>>>
>>>> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
>>>>
>>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>>>
>>>>>
>>>>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>>>>
>>>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>>>
>>>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>>>> ---
>>>>>>>>    fs/io_uring.c | 13 +++++--------
>>>>>>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>>>> --- a/fs/io_uring.c
>>>>>>>> +++ b/fs/io_uring.c
>>>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>>>    				     struct io_kiocb *req)
>>>>>>>>    {
>>>>>>>> -	/* timeout requests always honor sequence */
>>>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>>    		return false;
>>>>>>>>
>>>>>>>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>>>    		return NULL;
>>>>>>>>
>>>>>>>>    	req = list_first_entry(list, struct io_kiocb, list);
>>>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>>>> -		list_del_init(&req->list);
>>>>>>>> -		return req;
>>>>>>>> -	}
>>>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>>>> +		return NULL;
>>>>>>> Hi,
>>>>>>>
>>>>>>> For timeout req, we should also compare the sequence to determine to
>>>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>>>
>>>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>>>> it, cleanup can be done differently.
>>>>>
>>>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>>>> original meaning.
>>>>
>>>> No worries, mistakes happen.
>>>>
>>>>> Personal opinion, timeout list should not be mixed with defer_list,
>>>>> which makes the code more complicated and difficult to understand.
>>>>
>>>> Not sure why you feel they are mixed? They are in separate lists, but
>>>> they share using the sequence logic. In that respet they are really not
>>>> that different, the sequence will trigger either one of them. Either as
>>>> a timeout, or as a "can now be issued". Hence the code handling them is
>>>> both shared and identical.
>>>
>>> I not sure, I think I need reread the code of timeout command.
>>>
>>>>
>>>> I do agree that the check could be cleaner, which is probably how the
>>>> mistake in this patch happened in the first place.
>>>>
>>>
>>> Yes, I agree with you. io_sequence_defer should be only cares about
>>> the sequence.  Thanks for point out this mistake.
>>
>> How about something like this? More cleanly separates them to avoid
>> mixing flags. The regular defer code will honor the flags, the timeout
>> code will just bypass those.
>>
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index c92cb09cc262..4a2bb81cb1f1 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>> 	return ctx;
>> }
>>
>> +static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
>> +				       struct io_kiocb *req)
>> +{
>> +	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>> +}
>> +
>> static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>> 				     struct io_kiocb *req)
>> {
>> -	/* timeout requests always honor sequence */
>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>> 		return false;
>>
>> -	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>> +	return __io_sequence_defer(ctx, req);
>> }
>>
>> -static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>> -					      struct list_head *list)
>> +static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>> {
>> 	struct io_kiocb *req;
>>
>> -	if (list_empty(list))
>> +	if (list_empty(&ctx->defer_list))
>> 		return NULL;
>>
>> -	req = list_first_entry(list, struct io_kiocb, list);
>> +	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
>> 	if (!io_sequence_defer(ctx, req)) {
>> 		list_del_init(&req->list);
>> 		return req;
>> @@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>> 	return NULL;
>> }
>>
>> -static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>> -{
>> -	return __io_get_deferred_req(ctx, &ctx->defer_list);
>> -}
>> -
>> static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
>> {
>> -	return __io_get_deferred_req(ctx, &ctx->timeout_list);
>> +	struct io_kiocb *req;
>> +
>> +	if (list_empty(&ctx->timeout_list))
>> +		return NULL;
>> +
>> +	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
>> +	if (!__io_sequence_defer(ctx, req)) {
>> +		list_del_init(&req->list);
>> +		return req;
>> +	}
>> +
>> +	return NULL;
>> }
>>
>> static void __io_commit_cqring(struct io_ring_ctx *ctx)
>>
> 
> Much better, clearly and easy understand.

Agree, this is easier to read as well, and harder to inadvertently
break. Can I add your reviewed-by to this one?

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:42               ` Jens Axboe
@ 2019-10-11  3:43                 ` Jackie Liu
  2019-10-11  3:47                   ` Jens Axboe
  0 siblings, 1 reply; 15+ messages in thread
From: Jackie Liu @ 2019-10-11  3:43 UTC (permalink / raw)
  To: Jens Axboe; +Cc: yangerkun, linux-block



> 2019年10月11日 11:42,Jens Axboe <axboe@kernel.dk> 写道:
> 
> On 10/10/19 9:41 PM, Jackie Liu wrote:
>> 
>> 
>>> 2019年10月11日 11:34,Jens Axboe <axboe@kernel.dk> 写道:
>>> 
>>> On 10/10/19 9:27 PM, Jackie Liu wrote:
>>>> 
>>>> 
>>>>> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
>>>>> 
>>>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>>>> 
>>>>>> 
>>>>>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>>>>> 
>>>>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>>>> 
>>>>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>>>>> ---
>>>>>>>>>   fs/io_uring.c | 13 +++++--------
>>>>>>>>>   1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>>>> 
>>>>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>>>>> --- a/fs/io_uring.c
>>>>>>>>> +++ b/fs/io_uring.c
>>>>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>>>>   static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>>>>   				     struct io_kiocb *req)
>>>>>>>>>   {
>>>>>>>>> -	/* timeout requests always honor sequence */
>>>>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>>>   		return false;
>>>>>>>>> 
>>>>>>>>>   	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>>>>   		return NULL;
>>>>>>>>> 
>>>>>>>>>   	req = list_first_entry(list, struct io_kiocb, list);
>>>>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>>>>> -		list_del_init(&req->list);
>>>>>>>>> -		return req;
>>>>>>>>> -	}
>>>>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>>>>> +		return NULL;
>>>>>>>> Hi,
>>>>>>>> 
>>>>>>>> For timeout req, we should also compare the sequence to determine to
>>>>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>>>> 
>>>>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>>>>> it, cleanup can be done differently.
>>>>>> 
>>>>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>>>>> original meaning.
>>>>> 
>>>>> No worries, mistakes happen.
>>>>> 
>>>>>> Personal opinion, timeout list should not be mixed with defer_list,
>>>>>> which makes the code more complicated and difficult to understand.
>>>>> 
>>>>> Not sure why you feel they are mixed? They are in separate lists, but
>>>>> they share using the sequence logic. In that respet they are really not
>>>>> that different, the sequence will trigger either one of them. Either as
>>>>> a timeout, or as a "can now be issued". Hence the code handling them is
>>>>> both shared and identical.
>>>> 
>>>> I not sure, I think I need reread the code of timeout command.
>>>> 
>>>>> 
>>>>> I do agree that the check could be cleaner, which is probably how the
>>>>> mistake in this patch happened in the first place.
>>>>> 
>>>> 
>>>> Yes, I agree with you. io_sequence_defer should be only cares about
>>>> the sequence.  Thanks for point out this mistake.
>>> 
>>> How about something like this? More cleanly separates them to avoid
>>> mixing flags. The regular defer code will honor the flags, the timeout
>>> code will just bypass those.
>>> 
>>> 
>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>> index c92cb09cc262..4a2bb81cb1f1 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>> 	return ctx;
>>> }
>>> 
>>> +static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
>>> +				       struct io_kiocb *req)
>>> +{
>>> +	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>> +}
>>> +
>>> static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>> 				     struct io_kiocb *req)
>>> {
>>> -	/* timeout requests always honor sequence */
>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>> 		return false;
>>> 
>>> -	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>> +	return __io_sequence_defer(ctx, req);
>>> }
>>> 
>>> -static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>> -					      struct list_head *list)
>>> +static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>>> {
>>> 	struct io_kiocb *req;
>>> 
>>> -	if (list_empty(list))
>>> +	if (list_empty(&ctx->defer_list))
>>> 		return NULL;
>>> 
>>> -	req = list_first_entry(list, struct io_kiocb, list);
>>> +	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
>>> 	if (!io_sequence_defer(ctx, req)) {
>>> 		list_del_init(&req->list);
>>> 		return req;
>>> @@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>> 	return NULL;
>>> }
>>> 
>>> -static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>>> -{
>>> -	return __io_get_deferred_req(ctx, &ctx->defer_list);
>>> -}
>>> -
>>> static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
>>> {
>>> -	return __io_get_deferred_req(ctx, &ctx->timeout_list);
>>> +	struct io_kiocb *req;
>>> +
>>> +	if (list_empty(&ctx->timeout_list))
>>> +		return NULL;
>>> +
>>> +	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
>>> +	if (!__io_sequence_defer(ctx, req)) {
>>> +		list_del_init(&req->list);
>>> +		return req;
>>> +	}
>>> +
>>> +	return NULL;
>>> }
>>> 
>>> static void __io_commit_cqring(struct io_ring_ctx *ctx)
>>> 
>> 
>> Much better, clearly and easy understand.
> 
> Agree, this is easier to read as well, and harder to inadvertently
> break. Can I add your reviewed-by to this one?
> 
> 

Yes, please, Reviewed-by: Jackie Liu <liuyun01@kylinos.cn>


--
BR, Jackie Liu




^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer
  2019-10-11  3:43                 ` Jackie Liu
@ 2019-10-11  3:47                   ` Jens Axboe
  0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2019-10-11  3:47 UTC (permalink / raw)
  To: Jackie Liu; +Cc: yangerkun, linux-block

On 10/10/19 9:43 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月11日 11:42,Jens Axboe <axboe@kernel.dk> 写道:
>>
>> On 10/10/19 9:41 PM, Jackie Liu wrote:
>>>
>>>
>>>> 2019年10月11日 11:34,Jens Axboe <axboe@kernel.dk> 写道:
>>>>
>>>> On 10/10/19 9:27 PM, Jackie Liu wrote:
>>>>>
>>>>>
>>>>>> 2019年10月11日 11:17,Jens Axboe <axboe@kernel.dk> 写道:
>>>>>>
>>>>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>>>>>
>>>>>>>
>>>>>>>> 2019年10月11日 10:35,Jens Axboe <axboe@kernel.dk> 写道:
>>>>>>>>
>>>>>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>>>>>> ---
>>>>>>>>>>    fs/io_uring.c | 13 +++++--------
>>>>>>>>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>>>>>> --- a/fs/io_uring.c
>>>>>>>>>> +++ b/fs/io_uring.c
>>>>>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>>>>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>>>>>    				     struct io_kiocb *req)
>>>>>>>>>>    {
>>>>>>>>>> -	/* timeout requests always honor sequence */
>>>>>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>>>>>    		return false;
>>>>>>>>>>
>>>>>>>>>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>>>>>    		return NULL;
>>>>>>>>>>
>>>>>>>>>>    	req = list_first_entry(list, struct io_kiocb, list);
>>>>>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>>>>>> -		list_del_init(&req->list);
>>>>>>>>>> -		return req;
>>>>>>>>>> -	}
>>>>>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>>>>>> +		return NULL;
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> For timeout req, we should also compare the sequence to determine to
>>>>>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>>>>>
>>>>>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>>>>>> it, cleanup can be done differently.
>>>>>>>
>>>>>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>>>>>> original meaning.
>>>>>>
>>>>>> No worries, mistakes happen.
>>>>>>
>>>>>>> Personal opinion, timeout list should not be mixed with defer_list,
>>>>>>> which makes the code more complicated and difficult to understand.
>>>>>>
>>>>>> Not sure why you feel they are mixed? They are in separate lists, but
>>>>>> they share using the sequence logic. In that respet they are really not
>>>>>> that different, the sequence will trigger either one of them. Either as
>>>>>> a timeout, or as a "can now be issued". Hence the code handling them is
>>>>>> both shared and identical.
>>>>>
>>>>> I not sure, I think I need reread the code of timeout command.
>>>>>
>>>>>>
>>>>>> I do agree that the check could be cleaner, which is probably how the
>>>>>> mistake in this patch happened in the first place.
>>>>>>
>>>>>
>>>>> Yes, I agree with you. io_sequence_defer should be only cares about
>>>>> the sequence.  Thanks for point out this mistake.
>>>>
>>>> How about something like this? More cleanly separates them to avoid
>>>> mixing flags. The regular defer code will honor the flags, the timeout
>>>> code will just bypass those.
>>>>
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index c92cb09cc262..4a2bb81cb1f1 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>> 	return ctx;
>>>> }
>>>>
>>>> +static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
>>>> +				       struct io_kiocb *req)
>>>> +{
>>>> +	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>> +}
>>>> +
>>>> static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>> 				     struct io_kiocb *req)
>>>> {
>>>> -	/* timeout requests always honor sequence */
>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>> 		return false;
>>>>
>>>> -	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>> +	return __io_sequence_defer(ctx, req);
>>>> }
>>>>
>>>> -static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>> -					      struct list_head *list)
>>>> +static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>>>> {
>>>> 	struct io_kiocb *req;
>>>>
>>>> -	if (list_empty(list))
>>>> +	if (list_empty(&ctx->defer_list))
>>>> 		return NULL;
>>>>
>>>> -	req = list_first_entry(list, struct io_kiocb, list);
>>>> +	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
>>>> 	if (!io_sequence_defer(ctx, req)) {
>>>> 		list_del_init(&req->list);
>>>> 		return req;
>>>> @@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>> 	return NULL;
>>>> }
>>>>
>>>> -static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
>>>> -{
>>>> -	return __io_get_deferred_req(ctx, &ctx->defer_list);
>>>> -}
>>>> -
>>>> static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
>>>> {
>>>> -	return __io_get_deferred_req(ctx, &ctx->timeout_list);
>>>> +	struct io_kiocb *req;
>>>> +
>>>> +	if (list_empty(&ctx->timeout_list))
>>>> +		return NULL;
>>>> +
>>>> +	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
>>>> +	if (!__io_sequence_defer(ctx, req)) {
>>>> +		list_del_init(&req->list);
>>>> +		return req;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> }
>>>>
>>>> static void __io_commit_cqring(struct io_ring_ctx *ctx)
>>>>
>>>
>>> Much better, clearly and easy understand.
>>
>> Agree, this is easier to read as well, and harder to inadvertently
>> break. Can I add your reviewed-by to this one?
>>
>>
> 
> Yes, please, Reviewed-by: Jackie Liu <liuyun01@kylinos.cn>

Great thanks, now queued up.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-09  1:19 [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jackie Liu
2019-10-09  1:19 ` [PATCH 2/2] io_uring: replace s->needs_lock with s->in_async Jackie Liu
2019-10-10 16:10   ` Jens Axboe
2019-10-10 16:10 ` [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer Jens Axboe
2019-10-11  2:24 ` yangerkun
2019-10-11  2:35   ` Jens Axboe
2019-10-11  3:06     ` Jackie Liu
2019-10-11  3:17       ` Jens Axboe
2019-10-11  3:26         ` Jens Axboe
2019-10-11  3:27         ` Jackie Liu
2019-10-11  3:34           ` Jens Axboe
2019-10-11  3:41             ` Jackie Liu
2019-10-11  3:42               ` Jens Axboe
2019-10-11  3:43                 ` Jackie Liu
2019-10-11  3:47                   ` Jens Axboe

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org linux-block@archiver.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/ public-inbox