[PATCH 5.19 0/3] 5.19 reverts

[PATCH 5.19 0/3] 5.19 reverts

[Pavel Begunkov]

We've got a couple of nops debugging features, which don't belong to
upstream, and IORING_CLOSE_FD_AND_FILE_SLOT nobody knows how to use
reliably and with a couple of unsolved problems.

All came in 5.19, let's revert them.

Pavel Begunkov (3):
  Revert "io_uring: support CQE32 for nop operation"
  Revert "io_uring: add buffer selection support to IORING_OP_NOP"
  io_uring: remove IORING_CLOSE_FD_AND_FILE_SLOT

 fs/io_uring.c                 | 46 +++--------------------------------
 include/uapi/linux/io_uring.h |  6 -----
 2 files changed, 4 insertions(+), 48 deletions(-)

-- 
2.36.1

[PATCH 5.19 1/3] Revert "io_uring: support CQE32 for nop operation"

[Pavel Begunkov]

This reverts commit 2bb04df7c2af9dad5d28771c723bc39b01cf7df4.

CQE32 nops were used for debugging and benchmarking but it doesn't
target any real use case. Revert it, we can return it back if someone
finds a good way to use it.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ca6170a66e62..bf556f77d4ab 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -784,12 +784,6 @@ struct io_msg {
 	u32 len;
 };
 
-struct io_nop {
-	struct file			*file;
-	u64				extra1;
-	u64				extra2;
-};
-
 struct io_async_connect {
 	struct sockaddr_storage		address;
 };
@@ -994,7 +988,6 @@ struct io_kiocb {
 		struct io_msg		msg;
 		struct io_xattr		xattr;
 		struct io_socket	sock;
-		struct io_nop		nop;
 		struct io_uring_cmd	uring_cmd;
 	};
 
@@ -5268,14 +5261,6 @@ static int io_splice(struct io_kiocb *req, unsigned int issue_flags)
 
 static int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
-	/*
-	 * If the ring is setup with CQE32, relay back addr/addr
-	 */
-	if (req->ctx->flags & IORING_SETUP_CQE32) {
-		req->nop.extra1 = READ_ONCE(sqe->addr);
-		req->nop.extra2 = READ_ONCE(sqe->addr2);
-	}
-
 	return 0;
 }
 
@@ -5296,11 +5281,7 @@ static int io_nop(struct io_kiocb *req, unsigned int issue_flags)
 	}
 
 	cflags = io_put_kbuf(req, issue_flags);
-	if (!(req->ctx->flags & IORING_SETUP_CQE32))
-		__io_req_complete(req, issue_flags, 0, cflags);
-	else
-		__io_req_complete32(req, issue_flags, 0, cflags,
-				    req->nop.extra1, req->nop.extra2);
+	__io_req_complete(req, issue_flags, 0, cflags);
 	return 0;
 }
 
-- 
2.36.1

[PATCH 5.19 2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"

[Pavel Begunkov]

This reverts commit 3d200242a6c968af321913b635fc4014b238cba4.

Buffer selection with nops was used for debugging and benchmarking but
is useless in real life. Let's revert it before it's released.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bf556f77d4ab..1b95c6750a81 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = {
 	[IORING_OP_NOP] = {
 		.audit_skip		= 1,
 		.iopoll			= 1,
-		.buffer_select		= 1,
 	},
 	[IORING_OP_READV] = {
 		.needs_file		= 1,
@@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
  */
 static int io_nop(struct io_kiocb *req, unsigned int issue_flags)
 {
-	unsigned int cflags;
-	void __user *buf;
-
-	if (req->flags & REQ_F_BUFFER_SELECT) {
-		size_t len = 1;
-
-		buf = io_buffer_select(req, &len, issue_flags);
-		if (!buf)
-			return -ENOBUFS;
-	}
-
-	cflags = io_put_kbuf(req, issue_flags);
-	__io_req_complete(req, issue_flags, 0, cflags);
+	__io_req_complete(req, issue_flags, 0, 0);
 	return 0;
 }
 
-- 
2.36.1

[PATCH 5.19 3/3] io_uring: remove IORING_CLOSE_FD_AND_FILE_SLOT

[Pavel Begunkov]

This partially reverts a7c41b4687f5902af70cd559806990930c8a307b

Even though IORING_CLOSE_FD_AND_FILE_SLOT might save cycles for some
users, but it tries to do two things at a time and it's not clear how to
handle errors and what to return in a single result field when one part
fails and another completes well. Kill it for now.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1b95c6750a81..1b0b6099e717 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -576,7 +576,6 @@ struct io_close {
 	struct file			*file;
 	int				fd;
 	u32				file_slot;
-	u32				flags;
 };
 
 struct io_timeout_data {
@@ -5966,18 +5965,14 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
 
 static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 {
-	if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
+	if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
 		return -EINVAL;
 	if (req->flags & REQ_F_FIXED_FILE)
 		return -EBADF;
 
 	req->close.fd = READ_ONCE(sqe->fd);
 	req->close.file_slot = READ_ONCE(sqe->file_index);
-	req->close.flags = READ_ONCE(sqe->close_flags);
-	if (req->close.flags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
-		return -EINVAL;
-	if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
-	    req->close.file_slot && req->close.fd)
+	if (req->close.file_slot && req->close.fd)
 		return -EINVAL;
 
 	return 0;
@@ -5993,8 +5988,7 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (req->close.file_slot) {
 		ret = io_close_fixed(req, issue_flags);
-		if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
-			goto err;
+		goto err;
 	}
 
 	spin_lock(&files->file_lock);
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 776e0278f9dd..53e7dae92e42 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -47,7 +47,6 @@ struct io_uring_sqe {
 		__u32		unlink_flags;
 		__u32		hardlink_flags;
 		__u32		xattr_flags;
-		__u32		close_flags;
 	};
 	__u64	user_data;	/* data to be passed back at completion time */
 	/* pack this to avoid bogus arm OABI complaints */
@@ -259,11 +258,6 @@ enum io_uring_op {
  */
 #define IORING_ACCEPT_MULTISHOT	(1U << 0)
 
-/*
- * close flags, store in sqe->close_flags
- */
-#define IORING_CLOSE_FD_AND_FILE_SLOT	(1U << 0)
-
 /*
  * IO completion data structure (Completion Queue Entry)
  */
-- 
2.36.1

Re: [PATCH 5.19 0/3] 5.19 reverts

[Jens Axboe]

On Tue, 14 Jun 2022 17:51:15 +0100, Pavel Begunkov wrote:
> We've got a couple of nops debugging features, which don't belong to
> upstream, and IORING_CLOSE_FD_AND_FILE_SLOT nobody knows how to use
> reliably and with a couple of unsolved problems.
> 
> All came in 5.19, let's revert them.
> 
> Pavel Begunkov (3):
>   Revert "io_uring: support CQE32 for nop operation"
>   Revert "io_uring: add buffer selection support to IORING_OP_NOP"
>   io_uring: remove IORING_CLOSE_FD_AND_FILE_SLOT
> 
> [...]

Applied, thanks!

[1/3] Revert "io_uring: support CQE32 for nop operation"
      commit: 8899ce4b2f7364a90e3b9cf332dfd9993c61f46c
[2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"
      commit: aa165d6d2bb55f8b1bb5047fd634311681316fa2
[3/3] io_uring: remove IORING_CLOSE_FD_AND_FILE_SLOT
      commit: d884b6498d2f022098502e106d5a45ab635f2e9a

Best regards,
-- 
Jens Axboe

Re: [PATCH 5.19 2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"

[Dylan Yudaken]

On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote:
> This reverts commit 3d200242a6c968af321913b635fc4014b238cba4.
> 
> Buffer selection with nops was used for debugging and benchmarking
> but
> is useless in real life. Let's revert it before it's released.
> 
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bf556f77d4ab..1b95c6750a81 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = {
>         [IORING_OP_NOP] = {
>                 .audit_skip             = 1,
>                 .iopoll                 = 1,
> -               .buffer_select          = 1,
>         },
>         [IORING_OP_READV] = {
>                 .needs_file             = 1,
> @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req,
> const struct io_uring_sqe *sqe)
>   */
>  static int io_nop(struct io_kiocb *req, unsigned int issue_flags)
>  {
> -       unsigned int cflags;
> -       void __user *buf;
> -
> -       if (req->flags & REQ_F_BUFFER_SELECT) {
> -               size_t len = 1;
> -
> -               buf = io_buffer_select(req, &len, issue_flags);
> -               if (!buf)
> -                       return -ENOBUFS;
> -       }
> -
> -       cflags = io_put_kbuf(req, issue_flags);
> -       __io_req_complete(req, issue_flags, 0, cflags);
> +       __io_req_complete(req, issue_flags, 0, 0);
>         return 0;
>  }
>  

The liburing test case I added in "buf-ring: add tests that cycle
through the provided buffer ring" relies on this.

I don't mind either way if this is kept or that liburing patch is
reverted, but it should be consistent. What do you think?

Re: [PATCH 5.19 2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"

[Jens Axboe]

On 6/14/22 12:21 PM, Dylan Yudaken wrote:
> On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote:
>> This reverts commit 3d200242a6c968af321913b635fc4014b238cba4.
>>
>> Buffer selection with nops was used for debugging and benchmarking
>> but
>> is useless in real life. Let's revert it before it's released.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>  fs/io_uring.c | 15 +--------------
>>  1 file changed, 1 insertion(+), 14 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index bf556f77d4ab..1b95c6750a81 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[] = {
>>         [IORING_OP_NOP] = {
>>                 .audit_skip             = 1,
>>                 .iopoll                 = 1,
>> -               .buffer_select          = 1,
>>         },
>>         [IORING_OP_READV] = {
>>                 .needs_file             = 1,
>> @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb *req,
>> const struct io_uring_sqe *sqe)
>>   */
>>  static int io_nop(struct io_kiocb *req, unsigned int issue_flags)
>>  {
>> -       unsigned int cflags;
>> -       void __user *buf;
>> -
>> -       if (req->flags & REQ_F_BUFFER_SELECT) {
>> -               size_t len = 1;
>> -
>> -               buf = io_buffer_select(req, &len, issue_flags);
>> -               if (!buf)
>> -                       return -ENOBUFS;
>> -       }
>> -
>> -       cflags = io_put_kbuf(req, issue_flags);
>> -       __io_req_complete(req, issue_flags, 0, cflags);
>> +       __io_req_complete(req, issue_flags, 0, 0);
>>         return 0;
>>  }
>>  
> 
> The liburing test case I added in "buf-ring: add tests that cycle
> through the provided buffer ring" relies on this.

Good point.

> I don't mind either way if this is kept or that liburing patch is
> reverted, but it should be consistent. What do you think?

It was useful for benchmarking as well, but it'd be a trivial patch to
do for targeted testing.

I'm fine with killing it, but can also be persuaded not to ;-)

-- 
Jens Axboe

Re: [PATCH 5.19 2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"

[Dylan Yudaken]

On Tue, 2022-06-14 at 12:26 -0600, Jens Axboe wrote:
> On 6/14/22 12:21 PM, Dylan Yudaken wrote:
> > On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote:
> > > This reverts commit 3d200242a6c968af321913b635fc4014b238cba4.
> > > 
> > > Buffer selection with nops was used for debugging and
> > > benchmarking
> > > but
> > > is useless in real life. Let's revert it before it's released.
> > > 
> > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> > > ---
> > >  fs/io_uring.c | 15 +--------------
> > >  1 file changed, 1 insertion(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/io_uring.c b/fs/io_uring.c
> > > index bf556f77d4ab..1b95c6750a81 100644
> > > --- a/fs/io_uring.c
> > > +++ b/fs/io_uring.c
> > > @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[]
> > > = {
> > >         [IORING_OP_NOP] = {
> > >                 .audit_skip             = 1,
> > >                 .iopoll                 = 1,
> > > -               .buffer_select          = 1,
> > >         },
> > >         [IORING_OP_READV] = {
> > >                 .needs_file             = 1,
> > > @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb
> > > *req,
> > > const struct io_uring_sqe *sqe)
> > >   */
> > >  static int io_nop(struct io_kiocb *req, unsigned int
> > > issue_flags)
> > >  {
> > > -       unsigned int cflags;
> > > -       void __user *buf;
> > > -
> > > -       if (req->flags & REQ_F_BUFFER_SELECT) {
> > > -               size_t len = 1;
> > > -
> > > -               buf = io_buffer_select(req, &len, issue_flags);
> > > -               if (!buf)
> > > -                       return -ENOBUFS;
> > > -       }
> > > -
> > > -       cflags = io_put_kbuf(req, issue_flags);
> > > -       __io_req_complete(req, issue_flags, 0, cflags);
> > > +       __io_req_complete(req, issue_flags, 0, 0);
> > >         return 0;
> > >  }
> > >  
> > 
> > The liburing test case I added in "buf-ring: add tests that cycle
> > through the provided buffer ring" relies on this.
> 
> Good point.
> 
> > I don't mind either way if this is kept or that liburing patch is
> > reverted, but it should be consistent. What do you think?
> 
> It was useful for benchmarking as well, but it'd be a trivial patch
> to
> do for targeted testing.
> 
> I'm fine with killing it, but can also be persuaded not to ;-)
> 

I guess it's better to kill the liburing test which can always be made
to work with something other than NOP, than keeping code in the kernel
just for a liburing test..

I can send a revert now

Re: [PATCH 5.19 2/3] Revert "io_uring: add buffer selection support to IORING_OP_NOP"

[Pavel Begunkov]

On 6/15/22 08:33, Dylan Yudaken wrote:
> On Tue, 2022-06-14 at 12:26 -0600, Jens Axboe wrote:
>> On 6/14/22 12:21 PM, Dylan Yudaken wrote:
>>> On Tue, 2022-06-14 at 17:51 +0100, Pavel Begunkov wrote:
>>>> This reverts commit 3d200242a6c968af321913b635fc4014b238cba4.
>>>>
>>>> Buffer selection with nops was used for debugging and
>>>> benchmarking
>>>> but
>>>> is useless in real life. Let's revert it before it's released.
>>>>
>>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>>>> ---
>>>>   fs/io_uring.c | 15 +--------------
>>>>   1 file changed, 1 insertion(+), 14 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index bf556f77d4ab..1b95c6750a81 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -1114,7 +1114,6 @@ static const struct io_op_def io_op_defs[]
>>>> = {
>>>>          [IORING_OP_NOP] = {
>>>>                  .audit_skip             = 1,
>>>>                  .iopoll                 = 1,
>>>> -               .buffer_select          = 1,
>>>>          },
>>>>          [IORING_OP_READV] = {
>>>>                  .needs_file             = 1,
>>>> @@ -5269,19 +5268,7 @@ static int io_nop_prep(struct io_kiocb
>>>> *req,
>>>> const struct io_uring_sqe *sqe)
>>>>    */
>>>>   static int io_nop(struct io_kiocb *req, unsigned int
>>>> issue_flags)
>>>>   {
>>>> -       unsigned int cflags;
>>>> -       void __user *buf;
>>>> -
>>>> -       if (req->flags & REQ_F_BUFFER_SELECT) {
>>>> -               size_t len = 1;
>>>> -
>>>> -               buf = io_buffer_select(req, &len, issue_flags);
>>>> -               if (!buf)
>>>> -                       return -ENOBUFS;
>>>> -       }
>>>> -
>>>> -       cflags = io_put_kbuf(req, issue_flags);
>>>> -       __io_req_complete(req, issue_flags, 0, cflags);
>>>> +       __io_req_complete(req, issue_flags, 0, 0);
>>>>          return 0;
>>>>   }
>>>>   
>>>
>>> The liburing test case I added in "buf-ring: add tests that cycle
>>> through the provided buffer ring" relies on this.
>>
>> Good point.
>>
>>> I don't mind either way if this is kept or that liburing patch is
>>> reverted, but it should be consistent. What do you think?
>>
>> It was useful for benchmarking as well, but it'd be a trivial patch
>> to
>> do for targeted testing.
>>
>> I'm fine with killing it, but can also be persuaded not to ;-)
>>
> 
> I guess it's better to kill the liburing test which can always be made
> to work with something other than NOP, than keeping code in the kernel
> just for a liburing test..

Can we do same testing but without nops? Didn't read through the tests,
but e.g. read(nr_bytes=1) and check all but the first byte?


-- 
Pavel Begunkov