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

Xiaoguang Wang
2020-05-30T13:36:12Z

hi,

> On 28/05/2020 12:15, 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.
>
> It looks nicer. Especially if you'd add a helper as Jens supposed.
Sure, I'll add a helper in V4, thanks.

>
> The other thing, even though I hate treating a part of the fields differently
> from others, I don't like ->creds tossing either.
>
> Did you consider trying using only ->work.creds without adding req->creds? like
> in the untested incremental below. init_io_work() there is misleading, should be
> somehow played around better.
But if not adding a new req->creds, I think there will be some potential risks.
In current io_uring mainline codes, look at io_kiocb's memory layout
crash> struct -o io_kiocb
struct io_kiocb {
union {
[0] struct file *file;
[0] struct io_rw rw;
[0] struct io_poll_iocb poll;
[0] struct io_accept accept;
[0] struct io_sync sync;
[0] struct io_cancel cancel;
[0] struct io_timeout timeout;
[0] struct io_connect connect;
[0] struct io_sr_msg sr_msg;
[0] struct io_open open;
[0] struct io_close close;
[0] struct io_files_update files_update;
[0] struct io_fadvise fadvise;
[0] struct io_madvise madvise;
[0] struct io_epoll epoll;
[0] struct io_splice splice;
[0] struct io_provide_buf pbuf;
};
[64] struct io_async_ctx *io;
[72] int cflags;
[76] u8 opcode;
[78] u16 buf_index;
[80] struct io_ring_ctx *ctx;
[88] struct list_head list;
[104] unsigned int flags;
[108] refcount_t refs;
[112] struct task_struct *task;
[120] unsigned long fsize;
[128] u64 user_data;
[136] u32 result;
[140] u32 sequence;
[144] struct list_head link_list;
[160] struct list_head inflight_entry;
[176] struct percpu_ref *fixed_file_refs;
union {
struct {
[184] struct callback_head task_work;
[200] struct hlist_node hash_node;
[216] struct async_poll *apoll;
};
[184] struct io_wq_work work;
};
}
SIZE: 240

struct io_wq_work {
[0] struct io_wq_work_node list;
[8] void (*func)(struct io_wq_work **);
[16] struct files_struct *files;
[24] struct mm_struct *mm;
[32] const struct cred *creds;
[40] struct fs_struct *fs;
[48] unsigned int flags;
[52] pid_t task_pid;
}
SIZE: 56

The risk mainly comes from the union:
union {
/*
* Only commands that never go async can use the below fields,
* obviously. Right now only IORING_OP_POLL_ADD uses them, and
* async armed poll handlers for regular commands. The latter
* restore the work, if needed.
*/
struct {
struct callback_head task_work;
struct hlist_node hash_node;
struct async_poll *apoll;
};
struct io_wq_work work;
};

1, apoll and creds are in same memory offset, for 'async armed poll handlers' case,
apoll will be used, that means creds will be overwrited. In patch "io_uring: avoid
unnecessary io_wq_work copy for fast poll feature", I use REQ_F_WORK_INITIALIZED
to control whether to do io_wq_work restore, then your below codes will break:

static inline void io_req_work_drop_env(struct io_kiocb *req)
{
/* always init'ed, put before REQ_F_WORK_INITIALIZED check */
if (req->work.creds) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here req->work.creds will be invalid, or I still need to use some space
to record original req->work.creds, and do creds restore.

put_cred(req->work.creds);
req->work.creds = NULL;
}
if (!(req->flags & REQ_F_WORK_INITIALIZED))
return;

2, For IORING_OP_POLL_ADD case, current mainline codes will use task_work and hash_node,
32 bytes, that means io_wq_work's member list, func, files and mm would be overwrited,
but will not touch creds, it's safe now. But if we will add some new member to
struct {
struct callback_head task_work;
struct hlist_node hash_node;
struct async_poll *apoll;
};
say callback_head adds a new member, our check will still break.

3. IMO, io_wq_work is just to describe needed running environment for reqs that will be
punted to io-wq, for reqs submitted and completed inline should not touch this struct
from software design view, and current io_kiocb is 240 bytes, and a new pointer will be
248 bytes, still 4 cache lines for cache line 64 bytes.

Regards,
Xiaoguang Wang

>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 4dd3295d74f6..4086561ce444 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -643,7 +643,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;
> @@ -894,8 +893,16 @@ static const struct file_operations io_uring_fops;
> static inline void init_io_work(struct io_kiocb *req,
> void (*func)(struct io_wq_work **))
> {
> - req->work = (struct io_wq_work){ .func = func };
> - req->flags |= REQ_F_WORK_INITIALIZED;
> + struct io_wq_work *work = &req->work;
> +
> + /* work->creds are already initialised by a user */
> + work->list.next = NULL;
> + work->func = func;
> + work->files = NULL;
> + work->mm = NULL;
> + work->fs = NULL;
> + work->flags = REQ_F_WORK_INITIALIZED;
> + work->task_pid = 0;
> }
> struct sock *io_uring_get_socket(struct file *file)
> {
> @@ -1019,15 +1026,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)
> + req->work.creds = get_current_cred();
>
> - 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.fs && def->needs_fs) {
> spin_lock(&current->fs->lock);
> if (!current->fs->in_exec) {
> @@ -1044,6 +1045,12 @@ static inline void io_req_work_grab_env(struct io_kiocb *req,
>
> static inline void io_req_work_drop_env(struct io_kiocb *req)
> {
> + /* always init'ed, put before REQ_F_WORK_INITIALIZED check */
> + if (req->work.creds) {
> + put_cred(req->work.creds);
> + req->work.creds = NULL;
> + }
> +
> if (!(req->flags & REQ_F_WORK_INITIALIZED))
> return;
>
> @@ -1051,10 +1058,6 @@ static inline void io_req_work_drop_env(struct io_kiocb *req)
> mmdrop(req->work.mm);
> req->work.mm = NULL;
> }
> - if (req->work.creds) {
> - put_cred(req->work.creds);
> - req->work.creds = NULL;
> - }
> if (req->work.fs) {
> struct fs_struct *fs = req->work.fs;
>
> @@ -5901,12 +5904,12 @@ 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))
> + req->work.creds = idr_find(&ctx->personality_idr, id);
> + if (unlikely(!req->work.creds))
> return -EINVAL;
> - get_cred(req->creds);
> + get_cred(req->work.creds);
> } else
> - req->creds = NULL;
> + req->work.creds = NULL;
>
> /* same numerical values with corresponding REQ_F_*, safe to copy */
> req->flags |= sqe_flags;
>