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

[Dylan Yudaken <dylany@fb.com>]

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.

Dylan