Re: [PATCH v6.15 2/3] ublk: decouple zero copy from user copy

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

On Tue, Apr 29, 2025 at 08:55:48AM +0800, Ming Lei wrote:
> On Mon, Apr 28, 2025 at 09:01:04AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Apr 27, 2025 at 6:49 AM Ming Lei <ming.lei@redhat.com> wrote:
> > >
> > > UBLK_F_USER_COPY and UBLK_F_SUPPORT_ZERO_COPY are two different
> > > features, and shouldn't be coupled together.
> > >
> > > Commit 1f6540e2aabb ("ublk: zc register/unregister bvec") enables
> > > user copy automatically in case of UBLK_F_SUPPORT_ZERO_COPY, this way
> > > isn't correct.
> > >
> > > So decouple zero copy from user copy, and use independent helper to
> > > check each one.
> > 
> > I agree this makes sense.
> > 
> > >
> > > Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > ---
> > >  drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++------------
> > >  1 file changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 40f971a66d3e..0a3a3c64316d 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -205,11 +205,6 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> > >  static inline unsigned int ublk_req_build_flags(struct request *req);
> > >  static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> > >                                                    int tag);
> > > -static inline bool ublk_dev_is_user_copy(const struct ublk_device *ub)
> > > -{
> > > -       return ub->dev_info.flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > -}
> > > -
> > >  static inline bool ublk_dev_is_zoned(const struct ublk_device *ub)
> > >  {
> > >         return ub->dev_info.flags & UBLK_F_ZONED;
> > > @@ -609,14 +604,19 @@ static void ublk_apply_params(struct ublk_device *ub)
> > >                 ublk_dev_param_zoned_apply(ub);
> > >  }
> > >
> > > +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> > > +{
> > > +       return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> > > +}
> > > +
> > >  static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> > >  {
> > > -       return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> > > +       return ubq->flags & UBLK_F_USER_COPY;
> > >  }
> > >
> > >  static inline bool ublk_need_map_io(const struct ublk_queue *ubq)
> > >  {
> > > -       return !ublk_support_user_copy(ubq);
> > > +       return !ublk_support_user_copy(ubq) && !ublk_support_zero_copy(ubq);
> > >  }
> > >
> > >  static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > > @@ -624,8 +624,11 @@ static inline bool ublk_need_req_ref(const struct ublk_queue *ubq)
> > >         /*
> > >          * read()/write() is involved in user copy, so request reference
> > >          * has to be grabbed
> > > +        *
> > > +        * for zero copy, request buffer need to be registered to io_uring
> > > +        * buffer table, so reference is needed
> > >          */
> > > -       return ublk_support_user_copy(ubq);
> > > +       return ublk_support_user_copy(ubq) || ublk_support_zero_copy(ubq);
> > >  }
> > >
> > >  static inline void ublk_init_req_ref(const struct ublk_queue *ubq,
> > > @@ -2245,6 +2248,9 @@ static struct request *ublk_check_and_get_req(struct kiocb *iocb,
> > >         if (!ubq)
> > >                 return ERR_PTR(-EINVAL);
> > >
> > > +       if (!ublk_support_user_copy(ubq))
> > > +               return ERR_PTR(-EACCES);
> > 
> > This partly overlaps with the existing ublk_need_req_ref() check in
> > __ublk_check_and_get_req() (although that allows
> > UBLK_F_SUPPORT_ZERO_COPY too). Can that check be removed now that the
> > callers explicitly check ublk_support_user_copy() or
> > ublk_support_zero_copy()?
> 
> Yeah, it can be removed.

Actually the removal can only be done after the 3rd patch is applied with
zero copy check is added.



Thanks,
Ming