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

[Pavel Begunkov <asml.silence@gmail.com>]

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