From: Christoph Hellwig <hch@lst.de> To: Jens Axboe <axboe@kernel.dk> Cc: io-uring@vger.kernel.org, joshi.k@samsung.com, hch@lst.de, kbusch@kernel.org, linux-nvme@lists.infradead.org, metze@samba.org Subject: Re: [PATCH 4/8] io_uring: add support for IORING_OP_URING_CMD Date: Thu, 18 Mar 2021 06:42:54 +0100 [thread overview] Message-ID: <20210318054254.GC28063@lst.de> (raw) In-Reply-To: <20210317221027.366780-5-axboe@kernel.dk> On Wed, Mar 17, 2021 at 04:10:23PM -0600, Jens Axboe wrote: > +/* > + * Called by consumers of io_uring_cmd, if they originally returned > + * -EIOCBQUEUED upon receiving the command. > + */ > +void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret) > +{ > + struct io_kiocb *req = container_of(cmd, struct io_kiocb, uring_cmd); > + > + if (ret < 0) > + req_set_fail_links(req); > + io_req_complete(req, ret); > +} > +EXPORT_SYMBOL(io_uring_cmd_done); This really should be EXPORT_SYMBOL_GPL. But more importantly I'm not sure it is an all that useful interface. All useful non-trivial ioctls tend to access user memory, so something that queues up work in the task context like in Joshis patch should really be part of the documented interface. > + > +static int io_uring_cmd_prep(struct io_kiocb *req, > + const struct io_uring_sqe *sqe) > +{ > + const struct io_uring_cmd_sqe *csqe = (const void *) sqe; We really should not need this casting. The struct io_uring_sqe usage in io_uring.c needs to be replaced with a union or some other properly type safe way to handle this. > + ret = file->f_op->uring_cmd(&req->uring_cmd, issue_flags); > + /* queued async, consumer will call io_uring_cmd_done() when complete */ > + if (ret == -EIOCBQUEUED) > + return 0; > + io_uring_cmd_done(&req->uring_cmd, ret); > + return 0; This can be simplified to: if (ret != -EIOCBQUEUED) io_uring_cmd_done(&req->uring_cmd, ret); return 0; > +/* > + * Note that the first member here must be a struct file, as the > + * io_uring command layout depends on that. > + */ > +struct io_uring_cmd { > + struct file *file; > + __u16 op; > + __u16 unused; > + __u32 len; > + __u64 pdu[5]; /* 40 bytes available inline for free use */ > +}; I am a little worried about exposting this internal structure to random drivers. OTOH we need something that eventually allows a container_of to io_kiocb for the completion, so I can't think of anything better at the moment either.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@lst.de> To: Jens Axboe <axboe@kernel.dk> Cc: io-uring@vger.kernel.org, joshi.k@samsung.com, hch@lst.de, kbusch@kernel.org, linux-nvme@lists.infradead.org, metze@samba.org Subject: Re: [PATCH 4/8] io_uring: add support for IORING_OP_URING_CMD Date: Thu, 18 Mar 2021 06:42:54 +0100 [thread overview] Message-ID: <20210318054254.GC28063@lst.de> (raw) In-Reply-To: <20210317221027.366780-5-axboe@kernel.dk> On Wed, Mar 17, 2021 at 04:10:23PM -0600, Jens Axboe wrote: > +/* > + * Called by consumers of io_uring_cmd, if they originally returned > + * -EIOCBQUEUED upon receiving the command. > + */ > +void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret) > +{ > + struct io_kiocb *req = container_of(cmd, struct io_kiocb, uring_cmd); > + > + if (ret < 0) > + req_set_fail_links(req); > + io_req_complete(req, ret); > +} > +EXPORT_SYMBOL(io_uring_cmd_done); This really should be EXPORT_SYMBOL_GPL. But more importantly I'm not sure it is an all that useful interface. All useful non-trivial ioctls tend to access user memory, so something that queues up work in the task context like in Joshis patch should really be part of the documented interface. > + > +static int io_uring_cmd_prep(struct io_kiocb *req, > + const struct io_uring_sqe *sqe) > +{ > + const struct io_uring_cmd_sqe *csqe = (const void *) sqe; We really should not need this casting. The struct io_uring_sqe usage in io_uring.c needs to be replaced with a union or some other properly type safe way to handle this. > + ret = file->f_op->uring_cmd(&req->uring_cmd, issue_flags); > + /* queued async, consumer will call io_uring_cmd_done() when complete */ > + if (ret == -EIOCBQUEUED) > + return 0; > + io_uring_cmd_done(&req->uring_cmd, ret); > + return 0; This can be simplified to: if (ret != -EIOCBQUEUED) io_uring_cmd_done(&req->uring_cmd, ret); return 0; > +/* > + * Note that the first member here must be a struct file, as the > + * io_uring command layout depends on that. > + */ > +struct io_uring_cmd { > + struct file *file; > + __u16 op; > + __u16 unused; > + __u32 len; > + __u64 pdu[5]; /* 40 bytes available inline for free use */ > +}; I am a little worried about exposting this internal structure to random drivers. OTOH we need something that eventually allows a container_of to io_kiocb for the completion, so I can't think of anything better at the moment either. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2021-03-18 5:43 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-17 22:10 [PATCHSET v4 0/8] io_uring passthrough support Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-17 22:10 ` [PATCH 1/8] io_uring: split up io_uring_sqe into hdr + main Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-18 5:34 ` Christoph Hellwig 2021-03-18 5:34 ` Christoph Hellwig 2021-03-18 18:40 ` Jens Axboe 2021-03-18 18:40 ` Jens Axboe 2021-03-19 11:20 ` Stefan Metzmacher 2021-03-19 11:20 ` Stefan Metzmacher 2021-03-19 13:29 ` Christoph Hellwig 2021-03-19 13:29 ` Christoph Hellwig 2022-02-24 22:34 ` Luis Chamberlain 2021-03-17 22:10 ` [PATCH 2/8] io_uring: add infrastructure around io_uring_cmd_sqe issue type Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-17 22:10 ` [PATCH 3/8] fs: add file_operations->uring_cmd() Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-18 5:38 ` Christoph Hellwig 2021-03-18 5:38 ` Christoph Hellwig 2021-03-18 18:41 ` Jens Axboe 2021-03-18 18:41 ` Jens Axboe 2022-02-17 1:27 ` Luis Chamberlain 2022-02-17 1:25 ` Luis Chamberlain 2021-03-17 22:10 ` [PATCH 4/8] io_uring: add support for IORING_OP_URING_CMD Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-18 5:42 ` Christoph Hellwig [this message] 2021-03-18 5:42 ` Christoph Hellwig 2021-03-18 18:43 ` Jens Axboe 2021-03-18 18:43 ` Jens Axboe 2021-03-17 22:10 ` [PATCH 5/8] block: wire up support for file_operations->uring_cmd() Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-18 5:44 ` Christoph Hellwig 2021-03-18 5:44 ` Christoph Hellwig 2021-03-17 22:10 ` [PATCH 6/8] block: add example ioctl Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2021-03-18 5:45 ` Christoph Hellwig 2021-03-18 5:45 ` Christoph Hellwig 2021-03-18 12:43 ` Pavel Begunkov 2021-03-18 12:43 ` Pavel Begunkov 2021-03-18 18:44 ` Jens Axboe 2021-03-18 18:44 ` Jens Axboe 2021-03-17 22:10 ` [PATCH 7/8] net: wire up support for file_operations->uring_cmd() Jens Axboe 2021-03-17 22:10 ` Jens Axboe 2022-02-17 1:03 ` Luis Chamberlain 2021-03-17 22:10 ` [PATCH 8/8] net: add example SOCKET_URING_OP_SIOCINQ/SOCKET_URING_OP_SIOCOUTQ Jens Axboe 2021-03-17 22:10 ` Jens Axboe
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210318054254.GC28063@lst.de \ --to=hch@lst.de \ --cc=axboe@kernel.dk \ --cc=io-uring@vger.kernel.org \ --cc=joshi.k@samsung.com \ --cc=kbusch@kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=metze@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.