Re: [PATCH 08/16] ublk: remove ublk_commit_and_fetch()

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

On Tue, Jul 08, 2025 at 09:27:57AM -0400, Caleb Sander Mateos wrote:
> On Wed, Jul 2, 2025 at 12:04 AM Ming Lei <ming.lei@redhat.com> wrote:
> >
> > Remove ublk_commit_and_fetch() and open code request completion.
> >
> > Now request reference is stored in 'ublk_io', which becomes one global
> > variable, the motivation is to centralize access 'struct ublk_io' reference,
> > then we can introduce lock to protect `ublk_io` in future for supporting
> > io batch.
> 
> I didn't follow this. What do you mean by "global variable"?

ublk server can send anything to driver with specified tag if batch io
extension is added and per-io task is relaxed, then 'ublk_io' instance can be
visible to any userpsace command, which needs protection, looks like one
global variable.

If reference is stored in request pdu, things becomes more like local
variable, since the early ublk_io flag check guarantees that concurrent
access can't reach 'request'.

> 
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/block/ublk_drv.c | 36 ++++++++++++++++++------------------
> >  1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > index 7fd2fa493d6a..13c6b1e0e1ef 100644
> > --- a/drivers/block/ublk_drv.c
> > +++ b/drivers/block/ublk_drv.c
> > @@ -711,13 +711,12 @@ static inline void ublk_put_req_ref(struct ublk_io *io, struct request *req)
> >                 __ublk_complete_rq(req);
> >  }
> >
> > -static inline void ublk_sub_req_ref(struct ublk_io *io, struct request *req)
> > +static inline bool ublk_sub_req_ref(struct ublk_io *io, struct request *req)
> >  {
> >         unsigned sub_refs = UBLK_REFCOUNT_INIT - io->task_registered_buffers;
> >
> >         io->task_registered_buffers = 0;
> > -       if (refcount_sub_and_test(sub_refs, &io->ref))
> > -               __ublk_complete_rq(req);
> > +       return refcount_sub_and_test(sub_refs, &io->ref);
> 
> The struct request *req parameter can be removed now. Looks like it
> can be removed from ublk_need_complete_req() too.

Good catch!

> 
> >  }
> >
> >  static inline bool ublk_need_get_data(const struct ublk_queue *ubq)
> > @@ -2224,21 +2223,13 @@ static int ublk_check_commit_and_fetch(const struct ublk_queue *ubq,
> >         return 0;
> >  }
> >
> > -static void ublk_commit_and_fetch(const struct ublk_queue *ubq,
> > -                                 struct ublk_io *io, struct io_uring_cmd *cmd,
> > -                                 struct request *req, unsigned int issue_flags,
> > -                                 __u64 zone_append_lba, u16 buf_idx)
> > +static bool ublk_need_complete_req(const struct ublk_queue *ubq,
> > +                                  struct ublk_io *io,
> > +                                  struct request *req)
> >  {
> > -       if (buf_idx != UBLK_INVALID_BUF_IDX)
> > -               io_buffer_unregister_bvec(cmd, buf_idx, issue_flags);
> > -
> > -       if (req_op(req) == REQ_OP_ZONE_APPEND)
> > -               req->__sector = zone_append_lba;
> > -
> >         if (ublk_need_req_ref(ubq))
> > -               ublk_sub_req_ref(io, req);
> > -       else
> > -               __ublk_complete_rq(req);
> > +               return ublk_sub_req_ref(io, req);
> > +       return true;
> >  }
> >
> >  static bool ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io,
> > @@ -2271,6 +2262,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> >         unsigned tag = ub_cmd->tag;
> >         struct request *req;
> >         int ret;
> > +       bool compl;
> >
> >         pr_devel("%s: received: cmd op %d queue %d tag %d result %d\n",
> >                         __func__, cmd->cmd_op, ub_cmd->q_id, tag,
> > @@ -2347,8 +2339,16 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> >                         goto out;
> >                 req = ublk_fill_io_cmd(io, cmd, ub_cmd->result);
> >                 ret = ublk_config_io_buf(ubq, io, cmd, ub_cmd->addr, &buf_idx);
> > -               ublk_commit_and_fetch(ubq, io, cmd, req, issue_flags,
> > -                                     ub_cmd->zone_append_lba, buf_idx);
> > +               compl = ublk_need_complete_req(ubq, io, req);
> > +
> > +               /* can't touch 'ublk_io' any more */
> > +               if (buf_idx != UBLK_INVALID_BUF_IDX)
> > +                       io_buffer_unregister_bvec(cmd, buf_idx, issue_flags);
> > +               if (req_op(req) == REQ_OP_ZONE_APPEND)
> > +                       req->__sector = ub_cmd->zone_append_lba;
> > +               if (compl)
> > +                       __ublk_complete_rq(req);
> 
> What is the benefit of separating the reference count decrement from
> the call to __ublk_complete_rq()? I can understand if you want to keep
> the code manipulating ublk_io separate from the code dispatching the
> completed request. But it seems like this could be written more simply
> as
> 
> if (ublk_need_complete_req(ubq, io, req))
>         __ublk_complete_rq(req);

ublk_need_complete_req() has to be protected, but the following code
(buffer unregister, complete req needn't and can't be covered with spin lock).

It is for putting anything needing protection in future together.


Thanks,
Ming