[PATCH -next] io_uring: add support for fchmod

[PATCH -next] io_uring: add support for fchmod

[lizetao]

Adds support for doing chmod through io_uring. IORING_OP_FCHMOD behaves like fchmod(2) and takes the same arguments.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 include/uapi/linux/io_uring.h |  1 +
 io_uring/fs.c                 | 32 ++++++++++++++++++++++++++++++++
 io_uring/fs.h                 |  3 +++
 io_uring/opdef.c              |  8 ++++++++
 4 files changed, 44 insertions(+)

diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 5d08435b95a8..de5cce11f937 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -262,6 +262,7 @@ enum io_uring_op {
 	IORING_OP_FTRUNCATE,
 	IORING_OP_BIND,
 	IORING_OP_LISTEN,
+	IORING_OP_FCHMOD,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,
diff --git a/io_uring/fs.c b/io_uring/fs.c index eccea851dd5a..835f66fb75ff 100644
--- a/io_uring/fs.c
+++ b/io_uring/fs.c
@@ -47,6 +47,11 @@ struct io_link {
 	int				flags;
 };
 
+struct io_fchmod {
+	struct file			*file;
+	umode_t				mode;
+};
+
 int io_renameat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)  {
 	struct io_rename *ren = io_kiocb_to_cmd(req, struct io_rename); @@ -291,3 +296,30 @@ void io_link_cleanup(struct io_kiocb *req)
 	putname(sl->oldpath);
 	putname(sl->newpath);
 }
+
+int io_fchmod_prep(struct io_kiocb *req, const struct io_uring_sqe 
+*sqe) {
+	struct io_fchmod *fc = io_kiocb_to_cmd(req, struct io_fchmod);
+
+	if (unlikely(sqe->off || sqe->addr || sqe->rw_flags ||
+		     sqe->buf_index || sqe->splice_fd_in))
+		return -EINVAL;
+	if (unlikely(req->flags & REQ_F_FIXED_FILE))
+		return -EBADF;
+
+	fc->mode = READ_ONCE(sqe->len);
+	req->flags |= REQ_F_FORCE_ASYNC;
+	return 0;
+}
+
+int io_fchmod(struct io_kiocb *req, unsigned int issue_flags) {
+	struct io_fchmod *fc = io_kiocb_to_cmd(req, struct io_fchmod);
+	int ret;
+
+	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
+
+	ret = vfs_fchmod(req->file, fc->mode);
+	io_req_set_res(req, ret, 0);
+	return IOU_OK;
+}
diff --git a/io_uring/fs.h b/io_uring/fs.h index 0bb5efe3d6bb..bc7d95e77b2f 100644
--- a/io_uring/fs.h
+++ b/io_uring/fs.h
@@ -18,3 +18,6 @@ int io_symlinkat(struct io_kiocb *req, unsigned int issue_flags);  int io_linkat_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);  int io_linkat(struct io_kiocb *req, unsigned int issue_flags);  void io_link_cleanup(struct io_kiocb *req);
+
+int io_fchmod_prep(struct io_kiocb *req, const struct io_uring_sqe 
+*sqe); int io_fchmod(struct io_kiocb *req, unsigned int issue_flags);
diff --git a/io_uring/opdef.c b/io_uring/opdef.c index 3de75eca1c92..eb5bf831513c 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -515,6 +515,11 @@ const struct io_issue_def io_issue_defs[] = {
 		.prep			= io_eopnotsupp_prep,
 #endif
 	},
+	[IORING_OP_FCHMOD] = {
+		.needs_file		= 1,
+		.prep			= io_fchmod_prep,
+		.issue			= io_fchmod,
+	},
 };
 
 const struct io_cold_def io_cold_defs[] = { @@ -744,6 +749,9 @@ const struct io_cold_def io_cold_defs[] = {
 	[IORING_OP_LISTEN] = {
 		.name			= "LISTEN",
 	},
+	[IORING_OP_FCHMOD] = {
+		.name			= "FCHMOD",
+	},
 };
 
 const char *io_uring_get_opcode(u8 opcode)
--
2.34.1

Re: [PATCH -next] io_uring: add support for fchmod

[Jens Axboe]

On 11/19/24 1:12 AM, lizetao wrote:
> Adds support for doing chmod through io_uring. IORING_OP_FCHMOD
> behaves like fchmod(2) and takes the same arguments.

Looks pretty straight forward. The only downside is the forced use of 
REQ_F_FORCE_ASYNC - did you look into how feasible it would be to allow
non-blocking issue of this? Would imagine the majority of fchmod calls
end up not blocking in the first place.

-- 
Jens Axboe

re: [PATCH -next] io_uring: add support for fchmod

[lizetao]

Hi,

>On 11/23/24 5:23 AM, lizetao wrote:
>> Hi
>> 
>>>> On 11/19/24 1:12 AM, lizetao wrote:
>>>> Adds support for doing chmod through io_uring. IORING_OP_FCHMOD 
>>>> behaves like fchmod(2) and takes the same arguments.
>> 
>>> Looks pretty straight forward. The only downside is the forced use of REQ_F_FORCE_ASYNC - did you look into how feasible it would be to allow non-blocking issue of this? Would imagine the majority of fchmod calls end up not blocking in the first place.
>> 
>> Yes, I considered fchmod to allow asynchronous execution and wrote a test case to test it, the results are as follows:
>> 
>> fchmod:
>> real	0m1.413s
>> user	0m0.253s
>> sys	0m1.079s
>> 
>> io_uring + fchmod:
>> real	0m1.268s
>> user	0m0.015s
>> sys	0m5.739s
>> 
>> There is about a 10% improvement.

> And that makes sense if you're keeping some fchmod inflight, as you'd generally just have one io-wq processing them and running things in parallel with submission. But what you you keep an indepth count of 1, eg do sync fchmod? Then it'd be considerably slower than the syscall.
Indeed, When performing REQ_F_FORCE_ASYNC operations at depth 1, performance is degraded. The results are as follows:

fchmod:
real	0m2.285s
user	0m0.050s
sys	0m1.996s

io_uring + fchmod:
real	0m2.541s
user	0m0.013s
sys	0m2.379s

>This isn't necessarily something to worry about, but fact is that if you can do a nonblock issue and have it succeed most of the time, that'll be more efficient (and faster for low/sync fchmod) than something that just offloads to io-wq. You can see that from your results too, comparing the sys number netween the two.
However, when I remove REQ_F_FORCE_ASYNC and use IO_URING_F_NONBLOCK, the performance is not improved. The measured results are as follows:
fchmod:
real	0m2.132s
user	0m0.048s
sys	0m1.845s

io_uring + fchmod:
real	0m2.196s
user	0m0.005s
sys	0m2.097s

>Hence why I'm asking if you looked into doing a nonblocking issue at all. This won't necessarily gate the inclusion of the patch, and it is something that can be changed down the line, I'm mostly just curious.
Does this result meet expectations? Or maybe I missed something, please let me know

>--
>Jens Axboe

---
Li Zetao

Re: [PATCH -next] io_uring: add support for fchmod

[Jens Axboe]

On 11/26/24 8:07 AM, lizetao wrote:
> Hi,
> 
>> On 11/23/24 5:23 AM, lizetao wrote:
>>> Hi
>>>
>>>>> On 11/19/24 1:12 AM, lizetao wrote:
>>>>> Adds support for doing chmod through io_uring. IORING_OP_FCHMOD 
>>>>> behaves like fchmod(2) and takes the same arguments.
>>>
>>>> Looks pretty straight forward. The only downside is the forced use
>>>> of REQ_F_FORCE_ASYNC - did you look into how feasible it would be
>>>> to allow non-blocking issue of this? Would imagine the majority of
>>>> fchmod calls end up not blocking in the first place.
>>>
>>> Yes, I considered fchmod to allow asynchronous execution and wrote a
>>> test case to test it, the results are as follows:

>>>
>>> fchmod:
>>> real	0m1.413s
>>> user	0m0.253s
>>> sys	0m1.079s
>>>
>>> io_uring + fchmod:
>>> real	0m1.268s
>>> user	0m0.015s
>>> sys	0m5.739s
>>>
>>> There is about a 10% improvement.
> 
>> And that makes sense if you're keeping some fchmod inflight, as you'd
>> generally just have one io-wq processing them and running things in
>> parallel with submission. But what you you keep an indepth count of
>> 1, eg do sync fchmod? Then it'd be considerably slower than the
>> syscall.
>
> Indeed, When performing REQ_F_FORCE_ASYNC operations at depth 1,
> performance is degraded. The results are as follows:
>
> fchmod:
> real	0m2.285s
> user	0m0.050s
> sys	0m1.996s
> 
> io_uring + fchmod:
> real	0m2.541s
> user	0m0.013s
> sys	0m2.379s

That's what I expected. But actually looks like io-wq does a good job in
this case, that's pretty close.

>> This isn't necessarily something to worry about, but fact is that if
>> you can do a nonblock issue and have it succeed most of the time,
>> that'll be more efficient (and faster for low/sync fchmod) than
>> something that just offloads to io-wq. You can see that from your
>> results too, comparing the sys number netween the two.
>
> However, when I remove REQ_F_FORCE_ASYNC and use IO_URING_F_NONBLOCK,
> the performance is not improved. The measured results are as follows:
> fchmod:
> real	0m2.132s
> user	0m0.048s
> sys	0m1.845s
> 
> io_uring + fchmod:
> real	0m2.196s
> user	0m0.005s
> sys	0m2.097s

You would not expect it to be faster, as it's really just doing the same
work through a different mechanism. I'd expect that to roughly be within
normal variance, and if you're not doing a submit_and_wait mechanism (eg
you're doing submit and wait separately, hence doing 2 syscalls for each
fchmod), then that likely explains the discrepancy, if there is any.

And you'd also need to actually be able to remove REQ_F_FORCE_ASYNC to
have this as something that could be included. Otherwise if vfs_fchmod()
blocks, then you're now stalling the whole pipeline. Removing it just as
a test is fine, as you did.

>> Hence why I'm asking if you looked into doing a nonblocking issue at
>> all. This won't necessarily gate the inclusion of the patch, and it
>> is something that can be changed down the line, I'm mostly just
>> curious.
>
> Does this result meet expectations? Or maybe I missed something,
> please let me know

Yep that looks like I expected. io-wq offload will be fine if you're
doing a bunch of fchmod, in fact it'll probably end up being faster as
you reported. But if you're doing single (or few) fchmod at the time,
then io-wq offload will be a bit slower.

-- 
Jens Axboe

Re: [PATCH -next] io_uring: add support for fchmod

[Pavel Begunkov]

On 11/26/24 15:07, lizetao wrote:
>>>>> On 11/19/24 1:12 AM, lizetao wrote:
>>>>> Adds support for doing chmod through io_uring. IORING_OP_FCHMOD
>>>>> behaves like fchmod(2) and takes the same arguments.
>>>
>>>> Looks pretty straight forward. The only downside is the forced use of REQ_F_FORCE_ASYNC - did you look into how feasible it would be to allow non-blocking issue of this? Would imagine the majority of fchmod calls end up not blocking in the first place.
>>>
>>> Yes, I considered fchmod to allow asynchronous execution and wrote a test case to test it, the results are as follows:
>>>

FYI, this email got into spam.

>>> fchmod:
>>> real	0m1.413s
>>> user	0m0.253s
>>> sys	0m1.079s
>>>
>>> io_uring + fchmod:
>>> real	0m1.268s
>>> user	0m0.015s
>>> sys	0m5.739s
>>>
>>> There is about a 10% improvement.
> 
>> And that makes sense if you're keeping some fchmod inflight, as you'd generally just have one io-wq processing them and running things in parallel with submission. But what you you keep an indepth count of 1, eg do sync fchmod? Then it'd be considerably slower than the syscall.
> Indeed, When performing REQ_F_FORCE_ASYNC operations at depth 1, performance is degraded. The results are as follows:
> 
> fchmod:
> real	0m2.285s
> user	0m0.050s
> sys	0m1.996s
> 
> io_uring + fchmod:
> real	0m2.541s
> user	0m0.013s
> sys	0m2.379s
> 
>> This isn't necessarily something to worry about, but fact is that if you can do a nonblock issue and have it succeed most of the time, that'll be more efficient (and faster for low/sync fchmod) than something that just offloads to io-wq. You can see that from your results too, comparing the sys number netween the two.
> However, when I remove REQ_F_FORCE_ASYNC and use IO_URING_F_NONBLOCK, the performance is not improved. The measured results are as follows:
> fchmod:
> real	0m2.132s
> user	0m0.048s
> sys	0m1.845s
> 
> io_uring + fchmod:
> real	0m2.196s
> user	0m0.005s
> sys	0m2.097s
> 
>> Hence why I'm asking if you looked into doing a nonblocking issue at all. This won't necessarily gate the inclusion of the patch, and it is something that can be changed down the line, I'm mostly just curious.
> Does this result meet expectations? Or maybe I missed something, please let me know

-- 
Pavel Begunkov

RE: [PATCH -next] io_uring: add support for fchmod

[lizetao]

Hi,

> -----Original Message-----
> From: Pavel Begunkov <asml.silence@gmail.com>
> Sent: Tuesday, December 3, 2024 10:44 PM
> To: lizetao <lizetao1@huawei.com>; Jens Axboe <axboe@kernel.dk>
> Cc: io-uring@vger.kernel.org
> Subject: Re: [PATCH -next] io_uring: add support for fchmod
> 
> On 11/26/24 15:07, lizetao wrote:
> >>>>> On 11/19/24 1:12 AM, lizetao wrote:
> >>>>> Adds support for doing chmod through io_uring.
> IORING_OP_FCHMOD
> >>>>> behaves like fchmod(2) and takes the same arguments.
> >>>
> >>>> Looks pretty straight forward. The only downside is the forced use of
> REQ_F_FORCE_ASYNC - did you look into how feasible it would be to allow
> non-blocking issue of this? Would imagine the majority of fchmod calls end
> up not blocking in the first place.
> >>>
> >>> Yes, I considered fchmod to allow asynchronous execution and wrote a
> test case to test it, the results are as follows:
> >>>
> 
> FYI, this email got into spam.
Sorry to bother everyone, but I would like to know if there are any plans to implement
asynchronous system calls through io_uring, and which system calls are in the planning.

--
Li Zetao

Re: [PATCH -next] io_uring: add support for fchmod

[Jens Axboe]

On 12/3/24 6:54 PM, lizetao wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Pavel Begunkov <asml.silence@gmail.com>
>> Sent: Tuesday, December 3, 2024 10:44 PM
>> To: lizetao <lizetao1@huawei.com>; Jens Axboe <axboe@kernel.dk>
>> Cc: io-uring@vger.kernel.org
>> Subject: Re: [PATCH -next] io_uring: add support for fchmod
>>
>> On 11/26/24 15:07, lizetao wrote:
>>>>>>> On 11/19/24 1:12 AM, lizetao wrote:
>>>>>>> Adds support for doing chmod through io_uring.
>> IORING_OP_FCHMOD
>>>>>>> behaves like fchmod(2) and takes the same arguments.
>>>>>
>>>>>> Looks pretty straight forward. The only downside is the forced use of
>> REQ_F_FORCE_ASYNC - did you look into how feasible it would be to allow
>> non-blocking issue of this? Would imagine the majority of fchmod calls end
>> up not blocking in the first place.
>>>>>
>>>>> Yes, I considered fchmod to allow asynchronous execution and wrote a
>> test case to test it, the results are as follows:
>>>>>
>>
>> FYI, this email got into spam.
> Sorry to bother everyone, but I would like to know if there are any
> plans to implement asynchronous system calls through io_uring, and
> which system calls are in the planning.

No specific plans, existing syscalls are mostly sync by nature of the
API for them. Supporting syscalls through io_uring in an efficient
manner generally necessitates changing something ala:

syscall_foo(..)
{
	return do_foo_syscall(...);
}

into

syscall_foo(...)
{
	start_foo_syscall(...);
	return wait_on_foo_syscall(...);
}

where the act of issuing and waiting for the completion of it are two
separate entities, generally where the waiting is a waitqueue and the
wait_on_foo_syscall() simply waits on it to be completed, and io_uring
can use this waitqueue to get a callback when it has finished.

That allows efficient processing of syscalls through io_uring. If you
don't do that, then you're stuck with do_foo_syscall(), and then
io_uring can only support it by punting to the io-wq worker threads
which will do the sync do_foo_sycall() part.

-- 
Jens Axboe