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

[Dylan Yudaken <dylany@fb.com>]

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:

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 326695f74b93..90ecd656cc13 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1257,7 +1257,7 @@ static const struct file_operations
io_uring_fops;
 
 const char *io_uring_get_opcode(u8 opcode)
 {
-       switch (opcode) {
+       switch ((enum io_uring_op)opcode) {
        case IORING_OP_NOP:
                return "NOP";
        case IORING_OP_READV:
diff --git a/include/uapi/linux/io_uring.h
b/include/uapi/linux/io_uring.h
index 980d82eb196e..a10b216ede3e 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -103,7 +103,7 @@ enum {
 #define IORING_SETUP_R_DISABLED        (1U << 6)       /* start with
ring disabled */
 #define IORING_SETUP_SUBMIT_ALL        (1U << 7)       /* continue
submit on error */
 
-enum {
+enum io_uring_op {
        IORING_OP_NOP,
        IORING_OP_READV,
        IORING_OP_WRITEV,


> 
> In any case, the LAST one is just a sentinel, both that and beyond
> should return eg INVALID.
> 

Will do