Re: [PATCH 1/3] io_uring: add io_uring_get_opcode

[Jens Axboe <axboe@kernel.dk>]

On 4/25/22 8:48 AM, Dylan Yudaken wrote:
> On Mon, 2022-04-25 at 08:29 -0600, Jens Axboe wrote:
>> On 4/25/22 7:21 AM, Dylan Yudaken wrote:
>>> On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote:
>>>> On 4/25/22 3:36 AM, Dylan Yudaken wrote:
>>>>> In some debug scenarios it is useful to have the text
>>>>> representation
>>>>> of
>>>>> the opcode. Add this function in preparation.
>>>>>
>>>>> Signed-off-by: Dylan Yudaken <dylany@fb.com>
>>>>> ---
>>>>>  fs/io_uring.c            | 91
>>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/io_uring.h |  5 +++
>>>>>  2 files changed, 96 insertions(+)
>>>>>
>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>> index e57d47a23682..326695f74b93 100644
>>>>> --- a/fs/io_uring.c
>>>>> +++ b/fs/io_uring.c
>>>>> @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep;
>>>>>  
>>>>>  static const struct file_operations io_uring_fops;
>>>>>  
>>>>> +const char *io_uring_get_opcode(u8 opcode)
>>>>> +{
>>>>> +       switch (opcode) {
>>>>> +       case IORING_OP_NOP:
>>>>> +               return "NOP";
>>>>> +       case IORING_OP_READV:
>>>>> +               return "READV";
>>>>> +       case IORING_OP_WRITEV:
>>>>> +               return "WRITEV";
>>>>> +       case IORING_OP_FSYNC:
>>>>> +               return "FSYNC";
>>>>> +       case IORING_OP_READ_FIXED:
>>>>> +               return "READ_FIXED";
>>>>> +       case IORING_OP_WRITE_FIXED:
>>>>> +               return "WRITE_FIXED";
>>>>> +       case IORING_OP_POLL_ADD:
>>>>> +               return "POLL_ADD";
>>>>> +       case IORING_OP_POLL_REMOVE:
>>>>> +               return "POLL_REMOVE";
>>>>> +       case IORING_OP_SYNC_FILE_RANGE:
>>>>> +               return "SYNC_FILE_RANGE";
>>>>> +       case IORING_OP_SENDMSG:
>>>>> +               return "SENDMSG";
>>>>> +       case IORING_OP_RECVMSG:
>>>>> +               return "RECVMSG";
>>>>> +       case IORING_OP_TIMEOUT:
>>>>> +               return "TIMEOUT";
>>>>> +       case IORING_OP_TIMEOUT_REMOVE:
>>>>> +               return "TIMEOUT_REMOVE";
>>>>> +       case IORING_OP_ACCEPT:
>>>>> +               return "ACCEPT";
>>>>> +       case IORING_OP_ASYNC_CANCEL:
>>>>> +               return "ASYNC_CANCEL";
>>>>> +       case IORING_OP_LINK_TIMEOUT:
>>>>> +               return "LINK_TIMEOUT";
>>>>> +       case IORING_OP_CONNECT:
>>>>> +               return "CONNECT";
>>>>> +       case IORING_OP_FALLOCATE:
>>>>> +               return "FALLOCATE";
>>>>> +       case IORING_OP_OPENAT:
>>>>> +               return "OPENAT";
>>>>> +       case IORING_OP_CLOSE:
>>>>> +               return "CLOSE";
>>>>> +       case IORING_OP_FILES_UPDATE:
>>>>> +               return "FILES_UPDATE";
>>>>> +       case IORING_OP_STATX:
>>>>> +               return "STATX";
>>>>> +       case IORING_OP_READ:
>>>>> +               return "READ";
>>>>> +       case IORING_OP_WRITE:
>>>>> +               return "WRITE";
>>>>> +       case IORING_OP_FADVISE:
>>>>> +               return "FADVISE";
>>>>> +       case IORING_OP_MADVISE:
>>>>> +               return "MADVISE";
>>>>> +       case IORING_OP_SEND:
>>>>> +               return "SEND";
>>>>> +       case IORING_OP_RECV:
>>>>> +               return "RECV";
>>>>> +       case IORING_OP_OPENAT2:
>>>>> +               return "OPENAT2";
>>>>> +       case IORING_OP_EPOLL_CTL:
>>>>> +               return "EPOLL_CTL";
>>>>> +       case IORING_OP_SPLICE:
>>>>> +               return "SPLICE";
>>>>> +       case IORING_OP_PROVIDE_BUFFERS:
>>>>> +               return "PROVIDE_BUFFERS";
>>>>> +       case IORING_OP_REMOVE_BUFFERS:
>>>>> +               return "REMOVE_BUFFERS";
>>>>> +       case IORING_OP_TEE:
>>>>> +               return "TEE";
>>>>> +       case IORING_OP_SHUTDOWN:
>>>>> +               return "SHUTDOWN";
>>>>> +       case IORING_OP_RENAMEAT:
>>>>> +               return "RENAMEAT";
>>>>> +       case IORING_OP_UNLINKAT:
>>>>> +               return "UNLINKAT";
>>>>> +       case IORING_OP_MKDIRAT:
>>>>> +               return "MKDIRAT";
>>>>> +       case IORING_OP_SYMLINKAT:
>>>>> +               return "SYMLINKAT";
>>>>> +       case IORING_OP_LINKAT:
>>>>> +               return "LINKAT";
>>>>> +       case IORING_OP_MSG_RING:
>>>>> +               return "MSG_RING";
>>>>> +       case IORING_OP_LAST:
>>>>> +               return "LAST";
>>>>> +       }
>>>>> +       return "UNKNOWN";
>>>>> +}
>>>>
>>>> My only worry here is that it's another place to touch when
>>>> adding an
>>>> opcode. I'm assuming the compiler doesn't warn if you're missing
>>>> one
>>>> since it's not strongly typed?
>>>
>>> It doesn't complain, but we could strongly type it to get it to? I
>>> don't think it will break anything (certainly does not locally).
>>> What
>>> about something like this:
>>
>> I think this would be fine. Would probably be cleaner if you just
>> make
>> io_uring_get_opcode() take an enum io_uring_op and just fwd declare
>> it
>> in io_uring.h?
> 
> I cannot do that bit, as there is a separate function for
> CONFIG_IO_URING=n. That would either require different signatures (u8
> vs enum io_uring_op) or including the enum even when not enabled in
> config. Both of these feel ugly.
> 
> I'll send a new series giving the enum a type.

Agree, let's keep the cast then.

-- 
Jens Axboe