[PATCH 0/5] random 5.19 patches

[PATCH 0/5] random 5.19 patches

[Pavel Begunkov]

Resending some leftovers

Pavel Begunkov (5):
  io_uring: use right helpers for file assign locking
  io_uring: refactor io_assign_file error path
  io_uring: store rsrc node in req instead of refs
  io_uring: add a helper for putting rsrc nodes
  io_uring: kill ctx arg from io_req_put_rsrc

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

-- 
2.35.2

[PATCH 1/5] io_uring: use right helpers for file assign locking

[Pavel Begunkov]

We have io_ring_submit_[un]lock() functions helping us with conditional
->uring_lock locking, use them in io_file_get_fixed() instead of hand
coding.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6988bdc182e4..423427e2203f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7371,8 +7371,7 @@ static inline struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
 	struct file *file = NULL;
 	unsigned long file_ptr;
 
-	if (issue_flags & IO_URING_F_UNLOCKED)
-		mutex_lock(&ctx->uring_lock);
+	io_ring_submit_lock(ctx, issue_flags);
 
 	if (unlikely((unsigned int)fd >= ctx->nr_user_files))
 		goto out;
@@ -7384,8 +7383,7 @@ static inline struct file *io_file_get_fixed(struct io_kiocb *req, int fd,
 	req->flags |= (file_ptr << REQ_F_SUPPORT_NOWAIT_BIT);
 	io_req_set_rsrc_node(req, ctx, 0);
 out:
-	if (issue_flags & IO_URING_F_UNLOCKED)
-		mutex_unlock(&ctx->uring_lock);
+	io_ring_submit_unlock(ctx, issue_flags);
 	return file;
 }
 
-- 
2.35.2

[PATCH 2/5] io_uring: refactor io_assign_file error path

[Pavel Begunkov]

All io_assign_file() callers do error handling themselves,
req_set_fail() in the io_assign_file()'s fail path needlessly bloats the
kernel and is not the best abstraction to have. Simplify the error path.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 423427e2203f..9626bc1cb0a0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7117,12 +7117,8 @@ static bool io_assign_file(struct io_kiocb *req, unsigned int issue_flags)
 		req->file = io_file_get_fixed(req, req->cqe.fd, issue_flags);
 	else
 		req->file = io_file_get_normal(req, req->cqe.fd);
-	if (req->file)
-		return true;
 
-	req_set_fail(req);
-	req->cqe.res = -EBADF;
-	return false;
+	return !!req->file;
 }
 
 static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)
-- 
2.35.2

Re: [PATCH 2/5] io_uring: refactor io_assign_file error path

[Jens Axboe]

On 4/18/22 1:51 PM, Pavel Begunkov wrote:
> All io_assign_file() callers do error handling themselves,
> req_set_fail() in the io_assign_file()'s fail path needlessly bloats the
> kernel and is not the best abstraction to have. Simplify the error path.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 423427e2203f..9626bc1cb0a0 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -7117,12 +7117,8 @@ static bool io_assign_file(struct io_kiocb *req, unsigned int issue_flags)
>  		req->file = io_file_get_fixed(req, req->cqe.fd, issue_flags);
>  	else
>  		req->file = io_file_get_normal(req, req->cqe.fd);
> -	if (req->file)
> -		return true;
>  
> -	req_set_fail(req);
> -	req->cqe.res = -EBADF;
> -	return false;
> +	return !!req->file;

Wouldn't it be cleaner to just do:


diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7625b29153b9..b91bcd52cc95 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7098,15 +7098,8 @@ static bool io_assign_file(struct io_kiocb *req, unsigned int issue_flags)
 		return true;
 
 	if (req->flags & REQ_F_FIXED_FILE)
-		req->file = io_file_get_fixed(req, req->fd, issue_flags);
-	else
-		req->file = io_file_get_normal(req, req->fd);
-	if (req->file)
-		return true;
-
-	req_set_fail(req);
-	req->result = -EBADF;
-	return false;
+		return io_file_get_fixed(req, req->fd, issue_flags);
+	return io_file_get_normal(req, req->fd);
 }
 
 static int io_issue_sqe(struct io_kiocb *req, unsigned int issue_flags)

-- 
Jens Axboe

Re: [PATCH 2/5] io_uring: refactor io_assign_file error path

[Jens Axboe]

On 4/18/22 2:50 PM, Jens Axboe wrote:
> On 4/18/22 1:51 PM, Pavel Begunkov wrote:
>> All io_assign_file() callers do error handling themselves,
>> req_set_fail() in the io_assign_file()'s fail path needlessly bloats the
>> kernel and is not the best abstraction to have. Simplify the error path.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 423427e2203f..9626bc1cb0a0 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -7117,12 +7117,8 @@ static bool io_assign_file(struct io_kiocb *req, unsigned int issue_flags)
>>  		req->file = io_file_get_fixed(req, req->cqe.fd, issue_flags);
>>  	else
>>  		req->file = io_file_get_normal(req, req->cqe.fd);
>> -	if (req->file)
>> -		return true;
>>  
>> -	req_set_fail(req);
>> -	req->cqe.res = -EBADF;
>> -	return false;
>> +	return !!req->file;
> 
> Wouldn't it be cleaner to just do:

As soon as I sent that, realize we're missing the file assignment
in that case. Stupid. Looks fine as-is.

-- 
Jens Axboe

[PATCH 3/5] io_uring: store rsrc node in req instead of refs

[Pavel Begunkov]

req->fixed_rsrc_refs keeps a pointer to rsrc node pcpu references, but
it's more natural just to store rsrc node directly. There were some
reasons for that in the past but not anymore.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 9626bc1cb0a0..c26de427b05d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -931,7 +931,7 @@ struct io_kiocb {
 	struct io_ring_ctx		*ctx;
 	struct task_struct		*task;
 
-	struct percpu_ref		*fixed_rsrc_refs;
+	struct io_rsrc_node		*rsrc_node;
 	/* store used ubuf, so we can prevent reloading */
 	struct io_mapped_ubuf		*imu;
 
@@ -1329,20 +1329,20 @@ static inline void io_req_put_rsrc_locked(struct io_kiocb *req,
 					  struct io_ring_ctx *ctx)
 	__must_hold(&ctx->uring_lock)
 {
-	struct percpu_ref *ref = req->fixed_rsrc_refs;
+	struct io_rsrc_node *node = req->rsrc_node;
 
-	if (ref) {
-		if (ref == &ctx->rsrc_node->refs)
+	if (node) {
+		if (node == ctx->rsrc_node)
 			ctx->rsrc_cached_refs++;
 		else
-			percpu_ref_put(ref);
+			percpu_ref_put(&node->refs);
 	}
 }
 
 static inline void io_req_put_rsrc(struct io_kiocb *req, struct io_ring_ctx *ctx)
 {
-	if (req->fixed_rsrc_refs)
-		percpu_ref_put(req->fixed_rsrc_refs);
+	if (req->rsrc_node)
+		percpu_ref_put(&req->rsrc_node->refs);
 }
 
 static __cold void io_rsrc_refs_drop(struct io_ring_ctx *ctx)
@@ -1365,8 +1365,8 @@ static inline void io_req_set_rsrc_node(struct io_kiocb *req,
 					struct io_ring_ctx *ctx,
 					unsigned int issue_flags)
 {
-	if (!req->fixed_rsrc_refs) {
-		req->fixed_rsrc_refs = &ctx->rsrc_node->refs;
+	if (!req->rsrc_node) {
+		req->rsrc_node = ctx->rsrc_node;
 
 		if (!(issue_flags & IO_URING_F_UNLOCKED)) {
 			lockdep_assert_held(&ctx->uring_lock);
@@ -1374,7 +1374,7 @@ static inline void io_req_set_rsrc_node(struct io_kiocb *req,
 			if (unlikely(ctx->rsrc_cached_refs < 0))
 				io_rsrc_refs_refill(ctx);
 		} else {
-			percpu_ref_get(req->fixed_rsrc_refs);
+			percpu_ref_get(&req->rsrc_node->refs);
 		}
 	}
 }
@@ -7606,7 +7606,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	req->flags = sqe_flags = READ_ONCE(sqe->flags);
 	req->cqe.user_data = READ_ONCE(sqe->user_data);
 	req->file = NULL;
-	req->fixed_rsrc_refs = NULL;
+	req->rsrc_node = NULL;
 	req->task = current;
 
 	if (unlikely(opcode >= IORING_OP_LAST)) {
-- 
2.35.2

[PATCH 4/5] io_uring: add a helper for putting rsrc nodes

[Pavel Begunkov]

Add a simple helper to encapsulating dropping rsrc nodes references,
it's cleaner and will help if we'd change rsrc refcounting or play with
percpu_ref_put() [no]inlining.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c26de427b05d..c67748eabbd5 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1325,6 +1325,11 @@ static inline void io_req_set_refcount(struct io_kiocb *req)
 
 #define IO_RSRC_REF_BATCH	100
 
+static void io_rsrc_put_node(struct io_rsrc_node *node, int nr)
+{
+	percpu_ref_put_many(&node->refs, nr);
+}
+
 static inline void io_req_put_rsrc_locked(struct io_kiocb *req,
 					  struct io_ring_ctx *ctx)
 	__must_hold(&ctx->uring_lock)
@@ -1335,21 +1340,21 @@ static inline void io_req_put_rsrc_locked(struct io_kiocb *req,
 		if (node == ctx->rsrc_node)
 			ctx->rsrc_cached_refs++;
 		else
-			percpu_ref_put(&node->refs);
+			io_rsrc_put_node(node, 1);
 	}
 }
 
 static inline void io_req_put_rsrc(struct io_kiocb *req, struct io_ring_ctx *ctx)
 {
 	if (req->rsrc_node)
-		percpu_ref_put(&req->rsrc_node->refs);
+		io_rsrc_put_node(req->rsrc_node, 1);
 }
 
 static __cold void io_rsrc_refs_drop(struct io_ring_ctx *ctx)
 	__must_hold(&ctx->uring_lock)
 {
 	if (ctx->rsrc_cached_refs) {
-		percpu_ref_put_many(&ctx->rsrc_node->refs, ctx->rsrc_cached_refs);
+		io_rsrc_put_node(ctx->rsrc_node, ctx->rsrc_cached_refs);
 		ctx->rsrc_cached_refs = 0;
 	}
 }
-- 
2.35.2

[PATCH 5/5] io_uring: kill ctx arg from io_req_put_rsrc

[Pavel Begunkov]

The ctx argument of io_req_put_rsrc() is not used, kill it.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c67748eabbd5..3905b3ec87b8 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1344,7 +1344,7 @@ static inline void io_req_put_rsrc_locked(struct io_kiocb *req,
 	}
 }
 
-static inline void io_req_put_rsrc(struct io_kiocb *req, struct io_ring_ctx *ctx)
+static inline void io_req_put_rsrc(struct io_kiocb *req)
 {
 	if (req->rsrc_node)
 		io_rsrc_put_node(req->rsrc_node, 1);
@@ -2173,7 +2173,7 @@ static void __io_req_complete_post(struct io_kiocb *req, s32 res,
 				req->link = NULL;
 			}
 		}
-		io_req_put_rsrc(req, ctx);
+		io_req_put_rsrc(req);
 		/*
 		 * Selected buffer deallocation in io_clean_op() assumes that
 		 * we don't hold ->completion_lock. Clean them here to avoid
@@ -2336,7 +2336,7 @@ static __cold void io_free_req(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
-	io_req_put_rsrc(req, ctx);
+	io_req_put_rsrc(req);
 	io_dismantle_req(req);
 	io_put_task(req->task, 1);
 
-- 
2.35.2

Re: [PATCH 0/5] random 5.19 patches

[Jens Axboe]

On Mon, 18 Apr 2022 20:51:10 +0100, Pavel Begunkov wrote:
> Resending some leftovers
> 
> Pavel Begunkov (5):
>   io_uring: use right helpers for file assign locking
>   io_uring: refactor io_assign_file error path
>   io_uring: store rsrc node in req instead of refs
>   io_uring: add a helper for putting rsrc nodes
>   io_uring: kill ctx arg from io_req_put_rsrc
> 
> [...]

Applied, thanks!

[1/5] io_uring: use right helpers for file assign locking
      commit: 602b87b4a9cc56c6054b4524a40ecb5b42e9c722
[2/5] io_uring: refactor io_assign_file error path
      commit: 6d51914bcd061b6c25d761470b4bbf1ea4470be9
[3/5] io_uring: store rsrc node in req instead of refs
      commit: bf9bab6e6369c4b3f03a97345bd401925d8b315c
[4/5] io_uring: add a helper for putting rsrc nodes
      commit: 86ec2e629c84f6d57e1ccfc638b6b475aca699a6
[5/5] io_uring: kill ctx arg from io_req_put_rsrc
      commit: 111141d5824e947a9a129393315d8473a9356af4

Best regards,
-- 
Jens Axboe