Re: [RFC PATCH 6/7] ublk: register buffer to specified io_uring & buf index via UBLK_F_AUTO_BUF_REG

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

On Wed, Apr 30, 2025 at 09:30:16AM -0700, Caleb Sander Mateos wrote:
> On Wed, Apr 30, 2025 at 8:45 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > On Mon, Apr 28, 2025 at 05:52:28PM -0700, Caleb Sander Mateos wrote:
> > > On Mon, Apr 28, 2025 at 2:45 AM Ming Lei <ming.lei@redhat.com> wrote:
> > > >
> > > > Add UBLK_F_AUTO_BUF_REG for supporting to register buffer automatically
> > > > to specified io_uring context and buffer index.
> > > >
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >  drivers/block/ublk_drv.c      | 56 ++++++++++++++++++++++++++++-------
> > > >  include/uapi/linux/ublk_cmd.h | 38 ++++++++++++++++++++++++
> > > >  2 files changed, 84 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > > index 1fd20e481a60..e82618442749 100644
> > > > --- a/drivers/block/ublk_drv.c
> > > > +++ b/drivers/block/ublk_drv.c
> > > > @@ -66,7 +66,8 @@
> > > >                 | UBLK_F_USER_COPY \
> > > >                 | UBLK_F_ZONED \
> > > >                 | UBLK_F_USER_RECOVERY_FAIL_IO \
> > > > -               | UBLK_F_UPDATE_SIZE)
> > > > +               | UBLK_F_UPDATE_SIZE \
> > > > +               | UBLK_F_AUTO_BUF_REG)
> > > >
> > > >  #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> > > >                 | UBLK_F_USER_RECOVERY_REISSUE \
> > > > @@ -146,7 +147,10 @@ struct ublk_uring_cmd_pdu {
> > > >
> > > >  struct ublk_io {
> > > >         /* userspace buffer address from io cmd */
> > > > -       __u64   addr;
> > > > +       union {
> > > > +               __u64   addr;
> > > > +               struct ublk_auto_buf_reg buf;
> > >
> > > Maybe add a comment justifying why these fields can overlap? From my
> > > understanding, buf is valid iff UBLK_F_AUTO_BUF_REG is set on the
> > > ublk_queue and addr is valid iff neither UBLK_F_USER_COPY,
> > > UBLK_F_SUPPORT_ZERO_COPY, nor UBLK_F_AUTO_BUF_REG is set.
> >
> > ->addr is for storing the userspace buffer, which is only used in
> > non-zc cases(zc, auto_buf_reg) or user copy case.
> 
> Right, could you add a comment to that effect? I think using

Sure.

> overlapping fields is subtle and has the potential to break in the
> future if the usage of the fields changes. Documenting the assumptions
> clearly would go a long way.

The usage is actually reliable and can be well documented 

- ->addr is used if data copy is required 

- ->zone_append_lba is for zoned, which requires UBLK_F_USER_COPY or 
UBLK_F_ZERO_COPY

- 'ublk_auto_buf_reg' is for UBLK_F_AUTO_BUF_REG which is actually one
special(automatic) zero copy, meantime zoned can't be supported for this
feature



Thanks, 
Ming