Re: [PATCH 04/20] ublk: add integrity UAPI

[Ming Lei <ming.lei@redhat.com>]

On Mon, Dec 22, 2025 at 10:09:50AM -0500, Caleb Sander Mateos wrote:
> On Mon, Dec 22, 2025 at 9:26 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Tue, Dec 16, 2025 at 10:34:38PM -0700, Caleb Sander Mateos wrote:
> > > From: Stanley Zhang <stazhang@purestorage.com>
> > >
> > > Add UAPI definitions for metadata/integrity support in ublk.
> > > UBLK_PARAM_TYPE_INTEGRITY and struct ublk_param_integrity allow a ublk
> > > server to specify the integrity params of a ublk device.
> > > The ublk driver will set UBLK_IO_F_INTEGRITY in the op_flags field of
> > > struct ublksrv_io_desc for requests with integrity data.
> > > The ublk server uses user copy with UBLKSRV_IO_INTEGRITY_FLAG set in the
> > > offset parameter to access a request's integrity buffer.
> > >
> > > Signed-off-by: Stanley Zhang <stazhang@purestorage.com>
> > > [csander: drop feature flag and redundant pi_tuple_size field,
> > >  add io_desc flag, use block metadata UAPI constants]
> > > Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> > > ---
> > >  include/uapi/linux/ublk_cmd.h | 20 +++++++++++++++++++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/ublk_cmd.h b/include/uapi/linux/ublk_cmd.h
> > > index ec77dabba45b..5bfb9a0521c3 100644
> > > --- a/include/uapi/linux/ublk_cmd.h
> > > +++ b/include/uapi/linux/ublk_cmd.h
> > > @@ -129,11 +129,15 @@
> > >  #define UBLK_QID_BITS                12
> > >  #define UBLK_QID_BITS_MASK   ((1ULL << UBLK_QID_BITS) - 1)
> > >
> > >  #define UBLK_MAX_NR_QUEUES   (1U << UBLK_QID_BITS)
> > >
> > > -#define UBLKSRV_IO_BUF_TOTAL_BITS    (UBLK_QID_OFF + UBLK_QID_BITS)
> > > +/* Copy to/from request integrity buffer instead of data buffer */
> > > +#define UBLK_INTEGRITY_FLAG_OFF              (UBLK_QID_OFF + UBLK_QID_BITS)
> > > +#define UBLKSRV_IO_INTEGRITY_FLAG    (1ULL << UBLK_INTEGRITY_FLAG_OFF)
> > > +
> >
> > I feel it is more readable to move the definition into the patch which uses
> > them.
> 
> Sure, I can do that.
> 
> >
> > > +#define UBLKSRV_IO_BUF_TOTAL_BITS    (UBLK_INTEGRITY_FLAG_OFF + 1)
> >
> > It is UAPI, UBLKSRV_IO_BUF_TOTAL_BITS shouldn't be changed, or can you
> > explain this way is safe?
> 
> It's not clear to me how userspace is expected to use
> UBLKSRV_IO_BUF_TOTAL_BITS. (Our ublk server, for one, doesn't use it.)
> Can you provide an example? It looks to me like the purpose is to
> communicate the number of bits needed to represent a user copy offset
> value, in which case it makes sense to include the integrity flag now
> that that bit is being used.

Yes, it is used for this purpose.

Now one new bit(bit 53) is added for marking if it is for meta user copy,
let's keep UBLKSRV_IO_BUF_TOTAL_BITS cover its original meaning, and not
include the new bit UBLKSRV_IO_INTEGRITY_FLAG, which can be documented.

Then it can avoid some trouble, such as, break some uapi header checker at
least.

> 
> >
> > >  #define UBLKSRV_IO_BUF_TOTAL_SIZE    (1ULL << UBLKSRV_IO_BUF_TOTAL_BITS)
> > >
> > >  /*
> > >   * ublk server can register data buffers for incoming I/O requests with a sparse
> > >   * io_uring buffer table. The request buffer can then be used as the data buffer
> > > @@ -406,10 +410,12 @@ struct ublksrv_ctrl_dev_info {
> > >   *
> > >   * ublk server has to check this flag if UBLK_AUTO_BUF_REG_FALLBACK is
> > >   * passed in.
> > >   */
> > >  #define              UBLK_IO_F_NEED_REG_BUF          (1U << 17)
> > > +/* Request has an integrity data buffer */
> > > +#define              UBLK_IO_F_INTEGRITY             (1U << 18)
> > >
> > >  /*
> > >   * io cmd is described by this structure, and stored in share memory, indexed
> > >   * by request tag.
> > >   *
> > > @@ -598,10 +604,20 @@ struct ublk_param_segment {
> > >       __u32   max_segment_size;
> > >       __u16   max_segments;
> > >       __u8    pad[2];
> > >  };
> > >
> > > +struct ublk_param_integrity {
> > > +     __u32   flags; /* LBMD_PI_CAP_* from linux/fs.h */
> > > +     __u8    interval_exp;
> > > +     __u8    metadata_size;
> > > +     __u8    pi_offset;
> > > +     __u8    csum_type; /* LBMD_PI_CSUM_* from linux/fs.h */
> > > +     __u8    tag_size;
> > > +     __u8    pad[7];
> > > +};
> > > +
> >
> > Just be curious, `pi_tuple_size` isn't defined, instead it is hard-coded in
> > ublk_integrity_pi_tuple_size().
> >
> > However, both scsi and nvme sets `pi_tuple_size`, so it means that ublk PI
> > supports one `subset` or scsi/nvme `pi_tuple_size` can be removed too?
> 
> blk_validate_integrity_limits() validates that pi_tuple_size matches
> the expected PI size for each csum_type value. So it looks like these
> fields are redundant. Yes, pi_tuple_size could probably be removed
> from the scsi/nvme block drivers too. But maybe there's value in
> having the drivers explicitly specify both values?

Ok, got it, then this interface looks fine.

For both scsi and nvme, `pi_tuple_size` is not read from hardware, and
actually calculated from guard type.


Thanks,
Ming