[PATCH 0/3] text representation of opcode in trace

[PATCH 0/3] text representation of opcode in trace

[Dylan Yudaken]

This series adds the text representation of opcodes into the trace. This
makes it much quicker to understand traces without having to translate
opcodes in your head.

Patch 1 is the translation function.
Patch 2 is a small cleanup
Patch 3 uses the translator in the trace logic

Dylan Yudaken (3):
  io_uring: add io_uring_get_opcode
  io_uring: rename op -> opcode
  io_uring: use the text representation of ops in trace

 fs/io_uring.c                   | 91 +++++++++++++++++++++++++++++++++
 include/linux/io_uring.h        |  5 ++
 include/trace/events/io_uring.h | 42 +++++++++------
 3 files changed, 121 insertions(+), 17 deletions(-)


base-commit: 155bc9505dbd6613585abbf0be6466f1c21536c4
-- 
2.30.2

[PATCH 1/3] io_uring: add io_uring_get_opcode

[Dylan Yudaken]

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";
+}
+
 struct sock *io_uring_get_socket(struct file *file)
 {
 #if defined(CONFIG_UNIX)
diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
index 1814e698d861..24651c229ed2 100644
--- a/include/linux/io_uring.h
+++ b/include/linux/io_uring.h
@@ -10,6 +10,7 @@ struct sock *io_uring_get_socket(struct file *file);
 void __io_uring_cancel(bool cancel_all);
 void __io_uring_free(struct task_struct *tsk);
 void io_uring_unreg_ringfd(void);
+const char *io_uring_get_opcode(u8 opcode);
 
 static inline void io_uring_files_cancel(void)
 {
@@ -42,6 +43,10 @@ static inline void io_uring_files_cancel(void)
 static inline void io_uring_free(struct task_struct *tsk)
 {
 }
+static inline const char *io_uring_get_opcode(u8 opcode)
+{
+	return "";
+}
 #endif
 
 #endif
-- 
2.30.2

[PATCH 2/3] io_uring: rename op -> opcode

[Dylan Yudaken]

do this for consistency with the other trace messages

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 include/trace/events/io_uring.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h
index 42534ec2ab9d..c122d2167aa4 100644
--- a/include/trace/events/io_uring.h
+++ b/include/trace/events/io_uring.h
@@ -530,7 +530,7 @@ TRACE_EVENT(io_uring_req_failed,
 	),
 
 	TP_printk("ring %p, req %p, user_data 0x%llx, "
-		  "op %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
+		  "opcode %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
 		  "len=%u, rw_flags=0x%x, buf_index=%d, "
 		  "personality=%d, file_index=%d, pad=0x%llx/%llx, error=%d",
 		  __entry->ctx, __entry->req, __entry->user_data,
-- 
2.30.2

[PATCH 3/3] io_uring: use the text representation of ops in trace

[Dylan Yudaken]

It is annoying to translate opcodes to textwhen tracing io_uring. Use the
io_uring_get_opcode function instead to use the text representation.

A downside here might have been that if the opcode is invalid it will not
be obvious, however the opcode is already overridden in these cases to
0 (NOP) in io_init_req(). Therefore this is a non issue.

Signed-off-by: Dylan Yudaken <dylany@fb.com>
---
 include/trace/events/io_uring.h | 42 ++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h
index c122d2167aa4..240afbb75302 100644
--- a/include/trace/events/io_uring.h
+++ b/include/trace/events/io_uring.h
@@ -7,6 +7,7 @@
 
 #include <linux/tracepoint.h>
 #include <uapi/linux/io_uring.h>
+#include <linux/io_uring.h>
 
 struct io_wq_work;
 
@@ -87,9 +88,11 @@ TRACE_EVENT(io_uring_register,
 		__entry->ret		= ret;
 	),
 
-	TP_printk("ring %p, opcode %d, nr_user_files %d, nr_user_bufs %d, "
+	TP_printk("ring %p, opcode %s, nr_user_files %d, nr_user_bufs %d, "
 			  "ret %ld",
-			  __entry->ctx, __entry->opcode, __entry->nr_files,
+			  __entry->ctx,
+			  io_uring_get_opcode(__entry->opcode),
+			  __entry->nr_files,
 			  __entry->nr_bufs, __entry->ret)
 );
 
@@ -169,8 +172,9 @@ TRACE_EVENT(io_uring_queue_async_work,
 		__entry->rw		= rw;
 	),
 
-	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d, flags 0x%x, %s queue, work %p",
-		__entry->ctx, __entry->req, __entry->user_data, __entry->opcode,
+	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%x, %s queue, work %p",
+		__entry->ctx, __entry->req, __entry->user_data,
+		io_uring_get_opcode(__entry->opcode),
 		__entry->flags, __entry->rw ? "hashed" : "normal", __entry->work)
 );
 
@@ -205,8 +209,9 @@ TRACE_EVENT(io_uring_defer,
 		__entry->opcode	= opcode;
 	),
 
-	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d",
-		__entry->ctx, __entry->req, __entry->data, __entry->opcode)
+	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s",
+		__entry->ctx, __entry->req, __entry->data,
+		io_uring_get_opcode(__entry->opcode))
 );
 
 /**
@@ -305,9 +310,9 @@ TRACE_EVENT(io_uring_fail_link,
 		__entry->link		= link;
 	),
 
-	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d, link %p",
-		__entry->ctx, __entry->req, __entry->user_data, __entry->opcode,
-		__entry->link)
+	TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, link %p",
+		__entry->ctx, __entry->req, __entry->user_data,
+		io_uring_get_opcode(__entry->opcode), __entry->link)
 );
 
 /**
@@ -389,9 +394,9 @@ TRACE_EVENT(io_uring_submit_sqe,
 		__entry->sq_thread	= sq_thread;
 	),
 
-	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, flags 0x%x, "
+	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, flags 0x%x, "
 		  "non block %d, sq_thread %d", __entry->ctx, __entry->req,
-		  __entry->user_data, __entry->opcode,
+		  __entry->user_data, io_uring_get_opcode(__entry->opcode),
 		  __entry->flags, __entry->force_nonblock, __entry->sq_thread)
 );
 
@@ -433,8 +438,9 @@ TRACE_EVENT(io_uring_poll_arm,
 		__entry->events		= events;
 	),
 
-	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, mask 0x%x, events 0x%x",
-		  __entry->ctx, __entry->req, __entry->user_data, __entry->opcode,
+	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, mask 0x%x, events 0x%x",
+		  __entry->ctx, __entry->req, __entry->user_data,
+		  io_uring_get_opcode(__entry->opcode),
 		  __entry->mask, __entry->events)
 );
 
@@ -470,8 +476,9 @@ TRACE_EVENT(io_uring_task_add,
 		__entry->mask		= mask;
 	),
 
-	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, mask %x",
-		__entry->ctx, __entry->req, __entry->user_data, __entry->opcode,
+	TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, mask %x",
+		__entry->ctx, __entry->req, __entry->user_data,
+		io_uring_get_opcode(__entry->opcode),
 		__entry->mask)
 );
 
@@ -530,11 +537,12 @@ TRACE_EVENT(io_uring_req_failed,
 	),
 
 	TP_printk("ring %p, req %p, user_data 0x%llx, "
-		  "opcode %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
+		  "opcode %s, flags 0x%x, prio=%d, off=%llu, addr=%llu, "
 		  "len=%u, rw_flags=0x%x, buf_index=%d, "
 		  "personality=%d, file_index=%d, pad=0x%llx/%llx, error=%d",
 		  __entry->ctx, __entry->req, __entry->user_data,
-		  __entry->opcode, __entry->flags, __entry->ioprio,
+		  io_uring_get_opcode(__entry->opcode),
+		  __entry->flags, __entry->ioprio,
 		  (unsigned long long)__entry->off,
 		  (unsigned long long) __entry->addr, __entry->len,
 		  __entry->op_flags,
-- 
2.30.2

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

[Jens Axboe]

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?

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

-- 
Jens Axboe

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

[Dylan Yudaken]

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

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[Dylan Yudaken]

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

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

[Jens Axboe]

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