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

[Anuj gupta <anuj1072538@gmail.com>]

On Thu, Nov 7, 2024 at 11:25 AM Christoph Hellwig <hch@lst.de> wrote:
>
> > +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.

The enum is added to keep a check on the number of flags that can
be added, and make sure that we don't overflow.

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

We are planning to document this in man/io_uring_enter.2 in the liburing
repo, right after this series goes in. Or should it go somewhere else?

> > +/* 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:

The attempt here is that if two extended capabilities are not known to
co-exist then they can be kept in the same place. Since each extended
capability is now a flag, we can check what combinations are valid and
throw an error in case of incompatibility. Do you see this differently?

>
> 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];

This space is reserved for extended capabilities that might be added down
the line. It was at the end in the earlier versions, but it is moved
to the beginning
now to maintain contiguity with the free space (18b) available in the first SQE,
based on previous discussions [1].

[1] https://lore.kernel.org/linux-block/ceb58d97-b2e3-4d36-898d-753ba69476be@samsung.com/

>
>         /*
>          * 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;
> };
>