Am 18.03.21 um 19:40 schrieb Jens Axboe:
> On 3/17/21 11:34 PM, Christoph Hellwig wrote:
>>> @@ -14,11 +14,22 @@
>>> /*
>>> * IO submission data structure (Submission Queue Entry)
>>> */
>>> +struct io_uring_sqe_hdr {
>>> + __u8 opcode; /* type of operation for this sqe */
>>> + __u8 flags; /* IOSQE_ flags */
>>> + __u16 ioprio; /* ioprio for the request */
>>> + __s32 fd; /* file descriptor to do IO on */
>>> +};
>>> +
>>> struct io_uring_sqe {
>>> +#ifdef __KERNEL__
>>> + struct io_uring_sqe_hdr hdr;
>>> +#else
>>> __u8 opcode; /* type of operation for this sqe */
>>> __u8 flags; /* IOSQE_ flags */
>>> __u16 ioprio; /* ioprio for the request */
>>> __s32 fd; /* file descriptor to do IO on */
>>> +#endif
>>> union {
>>> __u64 off; /* offset into file */
>>> __u64 addr2;
>>
>> Please don't do that ifdef __KERNEL__ mess. We never guaranteed
>> userspace API compatbility, just ABI compatibility.
>
> Right, but I'm the one that has to deal with the fallout. For the
> in-kernel one I can skip the __KERNEL__ part, and the layout is the
> same anyway.
>
>> But we really do have a biger problem here, and that is ioprio is
>> a field that is specific to the read and write commands and thus
>> should not be in the generic header. On the other hand the
>> personality is.
>>
>> So I'm not sure trying to retrofit this even makes all that much sense.
>>
>> Maybe we should just define io_uring_sqe_hdr the way it makes
>> sense:
>>
>> struct io_uring_sqe_hdr {
>> __u8 opcode;
>> __u8 flags;
>> __u16 personality;
>> __s32 fd;
>> __u64 user_data;
>> };
>>
>> and use that for all new commands going forward while marking the
>> old ones as legacy.
>>
>> io_uring_cmd_sqe would then be:
>>
>> struct io_uring_cmd_sqe {
>> struct io_uring_sqe_hdr hdr;
>> __u33 ioc;
>> __u32 len;
>> __u8 data[40];
>> };
>>
>> for example. Note the 32-bit opcode just like ioctl to avoid
>> getting into too much trouble due to collisions.
>
> I was debating that with myself too, it's essentially making
> the existing io_uring_sqe into io_uring_sqe_v1 and then making a new
> v2 one. That would impact _all_ commands, and we'd need some trickery
> to have newly compiled stuff use v2 and have existing applications
> continue to work with the v1 format. That's very different from having
> a single (or new) opcodes use a v2 format, effectively.
I think we should use v0 and v1.
I think io_init_req and io_prep_req could be merged into an io_init_prep_req()
which could then do:
switch (ctx->sqe_version)
case 0:
return io_init_prep_req_v0();
case 1:
return io_init_prep_req_v1();
default:
return -EINVAL;
The kernel would return IORING_FEAT_SQE_V1
and set ctx->sqe_version = 1 if IORING_SETUP_SQE_V1 was passed from
the caller.
liburing whould then need to pass struct io_uring *ring to
io_uring_prep_*(), io_uring_sqe_set_flags() and io_uring_sqe_set_data().
in order to use struct io_uring->sq.sqe_version to alter the behavior.
(I think we should also have a io_uring_sqe_set_personality() helper).
static inline void io_uring_prep_nop(struct io_uring *ring, struct io_uring_sqe *sqe)
{
struct io_uring_sqe_common *nop = &sqe->common;
if (ring->sq.sqe_version == 0)
io_uring_prep_rw_v0(IORING_OP_NOP, sqe, -1, NULL, 0, 0);
else
*nop = (struct io_uring_sqe_common) {
.hdr = {
.opcode = IORING_OP_NOP,
},
};
}
For new features the prep functions would return a pointer to
the specific structure (see also below).
static inline struct io_uring_sqe_file_cmd *
io_uring_prep_file_cmd(struct io_uring *ring, struct io_uring_sqe *sqe, int fd, uint32_t cmd_opcode)
{
struct io_uring_sqe_file_cmd *file_cmd = &sqe->file_cmd;
*file_cmd = (struct io_uring_sqe_file_cmd) {
.hdr = {
.opcode = IORING_OP_FILE_CMD,
},
.fd = fd,
.cmd_opcode = cmd_opcode,
}
return file_cmd;
}
The application could then also check for a n
In order to test v1 it should have a way to skip IORING_FEAT_SQE_V2
and all existing tests could have a helper function to toggle that
based on an environment variable, so that make runtests could run
each test in both modes.
> Looking into the feasibility of this. But if that is done, there are
> other things that need to be factored in, as I'm not at all interested
> in having a v3 down the line as well. And I'd need to be able to do this
> seamlessly, both from an application point of view, and a performance
> point of view (no stupid conversions inline).
> Things that come up when something like this is on the table
>
> - Should flags be extended? We're almost out... It hasn't been an
> issue so far, but seems a bit silly to go v2 and not at least leave
> a bit of room there. But obviously comes at a cost of losing eg 8
> bits somewhere else.
>
> - Is u8 enough for the opcode? Again, we're nowhere near the limits
> here, but eventually multiplexing might be necessary.
>
> That's just off the top of my head, probably other things to consider
> too.
What about using something like this:
struct io_uring_sqe_hdr {
__u64 user_data;
__u16 personality;
__u16 opcode;
__u32 flags;
};
I moved __s32 fd out of it as not all commands need it and some need more than
one. So I guess it's easier to have them in the per opcode structure.
and the io_file_get() should better be done in the per opcode prep_vX function.
struct io_uring_sqe_common {
struct io_uring_sqe_hdr hdr;
__u8 __reserved[48];
};
struct io_uring_sqe_rw_common {
struct io_uring_sqe_hdr hdr;
__s32 fd; /* file descriptor to do IO on */
__u32 len; /* buffer size or number of iovecs */
__u64 off; /* offset into file */
__u64 addr; /* pointer to buffer or iovecs */
__kernel_rwf_t rw_flags;
__u16 ioprio; /* ioprio for the request */
__u16 buf_index; /* index into fixed buffers, if used */
__u8 __reserved[16];
};
struct io_uring_sqe_file_cmd {
struct io_uring_sqe_hdr hdr;
__s32 fd; /* file descriptor to do IO on */
__u32 cmd_opcode; /* file specific command */
__u8 cmd_data[40]; /* command spefic data */
};
struct io_uring_sqe {
union {
struct io_uring_sqe_common common;
struct io_uring_sqe_common nop;
struct io_uring_sqe_rw_common readv;
struct io_uring_sqe_rw_common writev;
struct io_uring_sqe_rw_common read_fixed;
struct io_uring_sqe_rw_common write_fixed;
struct io_uring_sqe_file_cmd file_cmd;
};
};
Each _opcode_prep() function would then check hdr.flags for unsupported flags
and __reserved for zeros. Instead of having a global io_op_defs[] array
the _opcode_prep() function would have a static const definition for the opcode
and lease req->op_def (which would be const struct io_op_def *op_def);
Does that sound useful in anyway?
metze
