[PATCH 0/2] implement timeout update

[PATCH 0/2] implement timeout update

[Pavel Begunkov]

Timeout update is a IORING_OP_TIMEOUT_REMOVE request with timeout_flags
containing a new IORING_TIMEOUT_UPDATE flag. Even though naming may be
confusing, but update and remove are very similar both code and
functionality wise, so doesn't seem necessary to add a new opcode.

Updates don't support offsets, but I don't see a need either. Can
be implemented in the future by passing it in sqe->len.

Pavel Begunkov (2):
  io_uring: restructure io_timeout_cancel()
  io_uring: add timeout update

 fs/io_uring.c                 | 90 +++++++++++++++++++++++++++--------
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 71 insertions(+), 20 deletions(-)

-- 
2.24.0

[PATCH 1/2] io_uring: restructure io_timeout_cancel()

[Pavel Begunkov]

Add io_timeout_extract() helper, which searches and disarms timeouts,
but doesn't complete them. No functional changes.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 12e641c61708..bffcbec6c9be 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5639,24 +5639,10 @@ static enum hrtimer_restart io_timeout_fn(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-static int __io_timeout_cancel(struct io_kiocb *req)
-{
-	struct io_timeout_data *io = req->async_data;
-	int ret;
-
-	ret = hrtimer_try_to_cancel(&io->timer);
-	if (ret == -1)
-		return -EALREADY;
-	list_del_init(&req->timeout.list);
-
-	req_set_fail_links(req);
-	io_cqring_fill_event(req, -ECANCELED);
-	io_put_req_deferred(req, 1);
-	return 0;
-}
-
-static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
+static struct io_kiocb *io_timeout_extract(struct io_ring_ctx *ctx,
+					   __u64 user_data)
 {
+	struct io_timeout_data *io;
 	struct io_kiocb *req;
 	int ret = -ENOENT;
 
@@ -5668,9 +5654,27 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 	}
 
 	if (ret == -ENOENT)
-		return ret;
+		return ERR_PTR(ret);
+
+	io = req->async_data;
+	ret = hrtimer_try_to_cancel(&io->timer);
+	if (ret == -1)
+		return ERR_PTR(-EALREADY);
+	list_del_init(&req->timeout.list);
+	return req;
+}
 
-	return __io_timeout_cancel(req);
+static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
+{
+	struct io_kiocb *req = io_timeout_extract(ctx, user_data);
+
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req_set_fail_links(req);
+	io_cqring_fill_event(req, -ECANCELED);
+	io_put_req_deferred(req, 1);
+	return 0;
 }
 
 static int io_timeout_remove_prep(struct io_kiocb *req,
-- 
2.24.0

[PATCH 2/2] io_uring: add timeout update

[Pavel Begunkov]

Support timeout updates through IORING_OP_TIMEOUT_REMOVE with passed in
IORING_TIMEOUT_UPDATE. Updates doesn't support offset timeout mode.
Oirignal timeout.off will be ignored as well.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c                 | 52 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/io_uring.h |  1 +
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bffcbec6c9be..63d0d546e661 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -453,6 +453,10 @@ struct io_timeout {
 struct io_timeout_rem {
 	struct file			*file;
 	u64				addr;
+
+	/* timeout update */
+	struct timespec64		ts;
+	u32				flags;
 };
 
 struct io_rw {
@@ -5677,17 +5681,51 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 	return 0;
 }
 
+static int io_timeout_update(struct io_ring_ctx *ctx, __u64 user_data,
+			     struct timespec64 *ts, enum hrtimer_mode mode)
+{
+	struct io_kiocb *req = io_timeout_extract(ctx, user_data);
+	struct io_timeout_data *data;
+
+	if (IS_ERR(req))
+		return PTR_ERR(req);
+
+	req->timeout.off = 0; /* noseq */
+	data = req->async_data;
+	list_add_tail(&req->timeout.list, &ctx->timeout_list);
+	hrtimer_init(&data->timer, CLOCK_MONOTONIC, mode);
+	data->timer.function = io_timeout_fn;
+	hrtimer_start(&data->timer, timespec64_to_ktime(*ts), mode);
+	return 0;
+}
+
 static int io_timeout_remove_prep(struct io_kiocb *req,
 				  const struct io_uring_sqe *sqe)
 {
+	struct io_timeout_rem *tr = &req->timeout_rem;
+	int ret;
+
 	if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
 		return -EINVAL;
 	if (unlikely(req->flags & (REQ_F_FIXED_FILE | REQ_F_BUFFER_SELECT)))
 		return -EINVAL;
-	if (sqe->ioprio || sqe->buf_index || sqe->len || sqe->timeout_flags)
+	if (sqe->ioprio || sqe->buf_index || sqe->len)
 		return -EINVAL;
 
-	req->timeout_rem.addr = READ_ONCE(sqe->addr);
+	tr->addr = READ_ONCE(sqe->addr);
+	tr->flags = READ_ONCE(sqe->timeout_flags);
+	if (tr->flags) {
+		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
+			return -EINVAL;
+		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
+			return -EINVAL;
+
+		ret = __io_sq_thread_acquire_mm(req->ctx);
+		if (ret)
+			return ret;
+		if (get_timespec64(&tr->ts, u64_to_user_ptr(sqe->addr2)))
+			return -EFAULT;
+	}
 	return 0;
 }
 
@@ -5696,11 +5734,19 @@ static int io_timeout_remove_prep(struct io_kiocb *req,
  */
 static int io_timeout_remove(struct io_kiocb *req)
 {
+	struct io_timeout_rem *tr = &req->timeout_rem;
 	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	spin_lock_irq(&ctx->completion_lock);
-	ret = io_timeout_cancel(ctx, req->timeout_rem.addr);
+	if (req->timeout_rem.flags & IORING_TIMEOUT_UPDATE) {
+		enum hrtimer_mode mode = (tr->flags & IORING_TIMEOUT_ABS)
+					? HRTIMER_MODE_ABS : HRTIMER_MODE_REL;
+
+		ret = io_timeout_update(ctx, tr->addr, &tr->ts, mode);
+	} else {
+		ret = io_timeout_cancel(ctx, tr->addr);
+	}
 
 	io_cqring_fill_event(req, ret);
 	io_commit_cqring(ctx);
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 6bb8229de892..12a6443ea60d 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -151,6 +151,7 @@ enum {
  * sqe->timeout_flags
  */
 #define IORING_TIMEOUT_ABS	(1U << 0)
+#define IORING_TIMEOUT_UPDATE	(1U << 31)
 
 /*
  * sqe->splice_flags
-- 
2.24.0

Re: [PATCH 2/2] io_uring: add timeout update

[Jens Axboe]

On 11/29/20 10:12 AM, Pavel Begunkov wrote:
> +	tr->flags = READ_ONCE(sqe->timeout_flags);
> +	if (tr->flags) {
> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
> +			return -EINVAL;
> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
> +			return -EINVAL;

These flag comparisons are a bit obtuse - perhaps warrants a comment?

> +		ret = __io_sq_thread_acquire_mm(req->ctx);
> +		if (ret)
> +			return ret;

Why is this done manually?

> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 6bb8229de892..12a6443ea60d 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -151,6 +151,7 @@ enum {
>   * sqe->timeout_flags
>   */
>  #define IORING_TIMEOUT_ABS	(1U << 0)
> +#define IORING_TIMEOUT_UPDATE	(1U << 31)

Why bit 31?

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: add timeout update

[Pavel Begunkov]

On 30/11/2020 18:15, Jens Axboe wrote:
> On 11/29/20 10:12 AM, Pavel Begunkov wrote:
>> +	tr->flags = READ_ONCE(sqe->timeout_flags);
>> +	if (tr->flags) {
>> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
>> +			return -EINVAL;
>> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
>> +			return -EINVAL;
> 
> These flag comparisons are a bit obtuse - perhaps warrants a comment?

Ok, the one below should be more readable.

if (tr->flags & IORING_TIMEOUT_UPDATE) {
	if (flags & ~ALLOWED_UPDATE_FLAGS)
		return -EINVAL;
	...
} else if (tr->flags) {
	/* timeout removal doesn't support flags */
	return -EINVAL;
}

> 
>> +		ret = __io_sq_thread_acquire_mm(req->ctx);
>> +		if (ret)
>> +			return ret;
> 
> Why is this done manually?

mm is only needed in *prep(), so don't want IO_WQ_WORK_MM to put it
into req->work since it also affects timeout remove reqs.

> 
>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>> index 6bb8229de892..12a6443ea60d 100644
>> --- a/include/uapi/linux/io_uring.h
>> +++ b/include/uapi/linux/io_uring.h
>> @@ -151,6 +151,7 @@ enum {
>>   * sqe->timeout_flags
>>   */
>>  #define IORING_TIMEOUT_ABS	(1U << 0)
>> +#define IORING_TIMEOUT_UPDATE	(1U << 31)
> 
> Why bit 31?

Left bits for other potential timeout modes, don't know which though.
Can return it to bit 1.


-- 
Pavel Begunkov

Re: [PATCH 2/2] io_uring: add timeout update

[Jens Axboe]

On 11/30/20 11:27 AM, Pavel Begunkov wrote:
> On 30/11/2020 18:15, Jens Axboe wrote:
>> On 11/29/20 10:12 AM, Pavel Begunkov wrote:
>>> +	tr->flags = READ_ONCE(sqe->timeout_flags);
>>> +	if (tr->flags) {
>>> +		if (!(tr->flags & IORING_TIMEOUT_UPDATE))
>>> +			return -EINVAL;
>>> +		if (tr->flags & ~(IORING_TIMEOUT_UPDATE|IORING_TIMEOUT_ABS))
>>> +			return -EINVAL;
>>
>> These flag comparisons are a bit obtuse - perhaps warrants a comment?
> 
> Ok, the one below should be more readable.
> 
> if (tr->flags & IORING_TIMEOUT_UPDATE) {
> 	if (flags & ~ALLOWED_UPDATE_FLAGS)
> 		return -EINVAL;
> 	...
> } else if (tr->flags) {
> 	/* timeout removal doesn't support flags */
> 	return -EINVAL;
> }

Yeah, that's actually readable.

>>> +		ret = __io_sq_thread_acquire_mm(req->ctx);
>>> +		if (ret)
>>> +			return ret;
>>
>> Why is this done manually?
> 
> mm is only needed in *prep(), so don't want IO_WQ_WORK_MM to put it
> into req->work since it also affects timeout remove reqs.

Don't think it's worth it, I'd just mark it needing it uncondtionally
instead. Reason being the obvious of not having stuff like this buried
deep in an op hander, but also that for non SQPOLL, we'll do these
inline always and it's a no-op. For SQPOLL we'd need to grab it, but
I'd rather pay that cost than have this in there.

>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
>>> index 6bb8229de892..12a6443ea60d 100644
>>> --- a/include/uapi/linux/io_uring.h
>>> +++ b/include/uapi/linux/io_uring.h
>>> @@ -151,6 +151,7 @@ enum {
>>>   * sqe->timeout_flags
>>>   */
>>>  #define IORING_TIMEOUT_ABS	(1U << 0)
>>> +#define IORING_TIMEOUT_UPDATE	(1U << 31)
>>
>> Why bit 31?
> 
> Left bits for other potential timeout modes, don't know which though.
> Can return it to bit 1.

Let's just use 1, there's no clear separation in there anyway.

-- 
Jens Axboe