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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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
