On 2/23/20 6:07 PM, Andres Freund wrote:
> Hi,
>
> There's currently two places that know to call io_*_prep() for
> sqes. io_req_defer_prep() and io_issue_sqe(). E.g. for READV there's:
>
> static int io_req_defer_prep(struct io_kiocb *req,
> const struct io_uring_sqe *sqe)
> ...
> case IORING_OP_READV:
> case IORING_OP_READ_FIXED:
> case IORING_OP_READ:
> ret = io_read_prep(req, sqe, true);
> break;
>
> and
>
> static int io_issue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> struct io_kiocb **nxt, bool force_nonblock)
> {
> ...
> case IORING_OP_READV:
> case IORING_OP_READ_FIXED:
> case IORING_OP_READ:
> if (sqe) {
> ret = io_read_prep(req, sqe, force_nonblock);
> if (ret < 0)
> break;
> }
> ret = io_read(req, nxt, force_nonblock);
> break;
>
> that seems a bit unnecessary. How about breaking that out into a
> separate function? I can write up a patch, just didn't want to do so if
> there's a reason for the current split.
>
>
> Alternatively it'd could all be just be dispatches via io_op_defs, but
> that'd be a bigger change with potential performance implications. And
> it'd benefit from prior deduplication anyway.
The reason for the split is that if we defer a request, it has to be
prepared up front. If the request has been deferred, then the
io_issue_sqe() invocation has sqe == NULL. Hence we only run the prep
handler once, and read the sqe just once.
This could of course be compacted with some indirect function calls, but
I didn't want to pay the overhead of doing so... The downside is that
the code is a bit bigger.
--
Jens Axboe
