[PATCH v8 06/10] io_uring/rw: add support to send metadata along with read/write

[Christoph Hellwig]

> +enum io_uring_sqe_ext_cap_bits {
> +	EXT_CAP_PI_BIT,
> +	/*
> +	 * not a real extended capability; just to make sure that we don't
> +	 * overflow
> +	 */
> +	EXT_CAP_LAST_BIT,
> +};
> +
> +/* extended capability flags */
> +#define EXT_CAP_PI	(1U << EXT_CAP_PI_BIT)

This is getting into nitpicking, but is the a good reason to have that
enum, which is never used as a type and the values or only defined to
actually define the bit positions below?  That's a bit confusing to
me.

Also please document the ABI for EXT_CAP_PI, right now this is again
entirely undocumented.

> +/* Second half of SQE128 for IORING_OP_READ/WRITE */
> +struct io_uring_sqe_ext {
> +	__u64	rsvd0[4];
> +	/* if sqe->ext_cap is EXT_CAP_PI, last 32 bytes are for PI */
> +	union {
> +		__u64	rsvd1[4];
> +		struct {
> +			__u16	flags;
> +			__u16	app_tag;
> +			__u32	len;
> +			__u64	addr;
> +			__u64	seed;
> +			__u64	rsvd;
> +		} rw_pi;
> +	};

And this is not what I though we discussed before.  By having a
union here you imply some kind of "type" again that is switched
on a value, and not flags indication the presence of potential
multiple optional and combinable features.  This is what I would
have expected here based on the previous discussion:

struct io_uring_sqe_ext {
	/*
	 * Reservered for please tell me what and why it is in the beginning
	 * and not the end:
	 */
	__u64	rsvd0[4];

	/*
	 * Only valid when EXT_CAP_PI is set:
	 */
	__u16	pi_flags; /* or make this generic flags, dunno? */
	__u16	app_tag;
	__u32	pi_len;
	__u64	pi_addr;
	__u64	pi_seed;

	__u64	rsvd1;
};