[PATCH 0/3] first batch of poll cleanups

[PATCH 0/3] first batch of poll cleanups

[Pavel Begunkov]

Few early readability changes for poll found while going through it.

# ./poll-mshot-update fails sometimes as below, but true w/o patches
submitted -16, 500
poll-many failed

p.s. not more expected changes for 5.12

Pavel Begunkov (3):
  io_uring: clean up io_poll_task_func()
  io_uring: refactor io_poll_complete()
  io_uring: simplify apoll hash removal

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

-- 
2.24.0

[PATCH 1/3] io_uring: clean up io_poll_task_func()

[Pavel Begunkov]

io_poll_complete() always fills an event (even an overflowed one), so we
always should do io_cqring_ev_posted() afterwards. And that's what is
currently happening, because second EPOLLONESHOT check is always true,
it can't return !done for oneshots.

Remove those branching, it's much easier to read.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 81e5d156af1c..f662b81bdc22 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4907,20 +4907,18 @@ static void io_poll_task_func(struct callback_head *cb)
 	if (io_poll_rewait(req, &req->poll)) {
 		spin_unlock_irq(&ctx->completion_lock);
 	} else {
-		bool done, post_ev;
+		bool done;
 
-		post_ev = done = io_poll_complete(req, req->result, 0);
+		done = io_poll_complete(req, req->result, 0);
 		if (done) {
 			hash_del(&req->hash_node);
-		} else if (!(req->poll.events & EPOLLONESHOT)) {
-			post_ev = true;
+		} else {
 			req->result = 0;
 			add_wait_queue(req->poll.head, &req->poll.wait);
 		}
 		spin_unlock_irq(&ctx->completion_lock);
+		io_cqring_ev_posted(ctx);
 
-		if (post_ev)
-			io_cqring_ev_posted(ctx);
 		if (done) {
 			nxt = io_put_req_find_next(req);
 			if (nxt)
-- 
2.24.0

[PATCH 2/3] io_uring: refactor io_poll_complete()

[Pavel Begunkov]

Remove error parameter from io_poll_complete(), 0 is always passed,
and do a bit of cleaning on top.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index f662b81bdc22..c5a0091e4042 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4875,18 +4875,19 @@ static void io_poll_remove_double(struct io_kiocb *req)
 	}
 }
 
-static bool io_poll_complete(struct io_kiocb *req, __poll_t mask, int error)
+static bool io_poll_complete(struct io_kiocb *req, __poll_t mask)
 	__must_hold(&req->ctx->completion_lock)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	unsigned flags = IORING_CQE_F_MORE;
+	int error;
 
-	if (!error && req->poll.canceled) {
+	if (READ_ONCE(req->poll.canceled)) {
 		error = -ECANCELED;
 		req->poll.events |= EPOLLONESHOT;
-	}
-	if (!error)
+	} else {
 		error = mangle_poll(mask);
+	}
 	if (req->poll.events & EPOLLONESHOT)
 		flags = 0;
 	if (!__io_cqring_fill_event(req, error, flags)) {
@@ -4909,7 +4910,7 @@ static void io_poll_task_func(struct callback_head *cb)
 	} else {
 		bool done;
 
-		done = io_poll_complete(req, req->result, 0);
+		done = io_poll_complete(req, req->result);
 		if (done) {
 			hash_del(&req->hash_node);
 		} else {
@@ -5395,7 +5396,7 @@ static int __io_poll_add(struct io_kiocb *req)
 
 	if (mask) { /* no async, we'd stolen it */
 		ipt.error = 0;
-		io_poll_complete(req, mask, 0);
+		io_poll_complete(req, mask);
 	}
 	spin_unlock_irq(&ctx->completion_lock);
 
-- 
2.24.0

[PATCH 3/3] io_uring: simplify apoll hash removal

[Pavel Begunkov]

hash_del() works well with non-hashed nodes.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c5a0091e4042..a724e63dd8c7 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5040,10 +5040,7 @@ static void io_async_task_func(struct callback_head *cb)
 		return;
 	}
 
-	/* If req is still hashed, it cannot have been canceled. Don't check. */
-	if (hash_hashed(&req->hash_node))
-		hash_del(&req->hash_node);
-
+	hash_del(&req->hash_node);
 	io_poll_remove_double(req);
 	spin_unlock_irq(&ctx->completion_lock);
 
-- 
2.24.0

Re: [PATCH 0/3] first batch of poll cleanups

[Jens Axboe]

On 4/9/21 2:13 AM, Pavel Begunkov wrote:
> Few early readability changes for poll found while going through it.

Thanks, looks good to me. Applied.

> # ./poll-mshot-update fails sometimes as below, but true w/o patches
> submitted -16, 500
> poll-many failed

Yeah I think it can run into overflow, the test case should be
improved. I'll take a look.

-- 
Jens Axboe

Re: [PATCH 0/3] first batch of poll cleanups

[Pavel Begunkov]

On 10/04/2021 21:44, Jens Axboe wrote:
> On 4/9/21 2:13 AM, Pavel Begunkov wrote:
>> Few early readability changes for poll found while going through it.
> 
> Thanks, looks good to me. Applied.
> 
>> # ./poll-mshot-update fails sometimes as below, but true w/o patches
>> submitted -16, 500
>> poll-many failed
> 
> Yeah I think it can run into overflow, the test case should be

fwiw, also hangs sometimes

> improved. I'll take a look.

Great, but it doesn't bother, was going to fix it myself but after
I'm done with other poll stuff. 

-- 
Pavel Begunkov

Re: [PATCH 0/3] first batch of poll cleanups

[Jens Axboe]

On 4/10/21 6:38 PM, Pavel Begunkov wrote:
> On 10/04/2021 21:44, Jens Axboe wrote:
>> On 4/9/21 2:13 AM, Pavel Begunkov wrote:
>>> Few early readability changes for poll found while going through it.
>>
>> Thanks, looks good to me. Applied.
>>
>>> # ./poll-mshot-update fails sometimes as below, but true w/o patches
>>> submitted -16, 500
>>> poll-many failed
>>
>> Yeah I think it can run into overflow, the test case should be
> 
> fwiw, also hangs sometimes

I've seen that too. Only with SQPOLL, but didn't try too hard without
it. Likely/hopefully the same root cause.

> Great, but it doesn't bother, was going to fix it myself but after
> I'm done with other poll stuff. 

Whoever gets there first, haven't had a look at it yet myself.

-- 
Jens Axboe