Re: [PATCH v6 1/2] io_uring: avoid whole io_wq_work copy for requests completed inline

[Pavel Begunkov <asml.silence@xxxxxxxxx>]

On 09/06/2020 11:25, Xiaoguang Wang wrote:
> If requests can be submitted and completed inline, we don't need to
> initialize whole io_wq_work in io_init_req(), which is an expensive
> operation, add a new 'REQ_F_WORK_INITIALIZED' to control whether
> io_wq_work is initialized.

Basically it's "call io_req_init_async() before touching ->work" now.
This shouldn't be as easy to screw as was with ->func.

The only thing left that I don't like _too_ much to stop complaining
is ->creds handling. But this one should be easy, see incremental diff
below (not tested). If custom creds are provided, it initialises
req->work in req_init() and sets work.creds. And then we can remove
req->creds.

What do you think? Custom ->creds (aka personality) is a niche feature,
and the speedup is not so great to care.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index c03408342320..5df7e02852bb 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -648,7 +648,6 @@ struct io_kiocb {
 	unsigned int		flags;
 	refcount_t		refs;
 	struct task_struct	*task;
-	const struct cred	*creds;
 	unsigned long		fsize;
 	u64			user_data;
 	u32			result;
@@ -1031,14 +1030,9 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
 		mmgrab(current->mm);
 		req->work.mm = current->mm;
 	}
-	if (!req->work.creds) {
-		if (!req->creds) {
-			req->work.creds = get_current_cred();
-		} else {
-			req->work.creds = req->creds;
-			req->creds = NULL;
-		}
-	}
+	if (!req->work.creds)
+		req->work.creds = get_current_cred();
+
 	if (!req->work.fs && def->needs_fs) {
 		spin_lock(&current->fs->lock);
 		if (!current->fs->in_exec) {
@@ -5569,23 +5563,20 @@ static void __io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
 	struct io_kiocb *linked_timeout;
 	struct io_kiocb *nxt;
-	const struct cred *creds, *old_creds = NULL;
+	const struct cred *old_creds = NULL;
 	int ret;
 
 again:
 	linked_timeout = io_prep_linked_timeout(req);
 
-	if (req->flags & REQ_F_WORK_INITIALIZED)
-		creds = req->work.creds;
-	else
-		creds = req->creds;
-	if (creds && creds != current_cred()) {
+	if ((req->flags & REQ_F_WORK_INITIALIZED) && req->work.creds &&
+	     req->work.creds != current_cred()) {
 		if (old_creds)
 			revert_creds(old_creds);
-		if (old_creds == creds)
+		if (old_creds == req->work.creds)
 			old_creds = NULL; /* restored original creds */
 		else
-			old_creds = override_creds(creds);
+			old_creds = override_creds(req->work.creds);
 	}
 
 	ret = io_issue_sqe(req, sqe, true);
@@ -5939,12 +5930,11 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
 
 	id = READ_ONCE(sqe->personality);
 	if (id) {
-		req->creds = idr_find(&ctx->personality_idr, id);
-		if (unlikely(!req->creds))
+		io_req_init_async(req);
+		req->work.creds = idr_find(&ctx->personality_idr, id);
+		if (unlikely(!req->work.creds))
 			return -EINVAL;
-		get_cred(req->creds);
-	} else {
-		req->creds = NULL;
+		get_cred(req->work.creds);
 	}
 
 	/* same numerical values with corresponding REQ_F_*, safe to copy */


-- 
Pavel Begunkov