On Tue, Sep 9, 2025 at 7:23 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Tue, Sep 02, 2025 at 09:41:55PM -0700, Caleb Sander Mateos wrote:
> > On Mon, Sep 1, 2025 at 3:03 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> > >
> > > Refactor auto buffer register code and prepare for supporting batch IO
> > > feature, and the main motivation is to put 'ublk_io' operation code
> > > together, so that per-io lock can be applied for the code block.
> > >
> > > The key changes are:
> > > - Rename ublk_auto_buf_reg() as ublk_do_auto_buf_reg()
> >
> > Thanks, the type and the function having the same name was a minor annoyance.
> >
> > > - Introduce an enum `auto_buf_reg_res` to represent the result of
> > > the buffer registration attempt (FAIL, FALLBACK, OK).
> > > - Split the existing `ublk_do_auto_buf_reg` function into two:
> > > - `__ublk_do_auto_buf_reg`: Performs the actual buffer registration
> > > and returns the `auto_buf_reg_res` status.
> > > - `ublk_do_auto_buf_reg`: A wrapper that calls the internal function
> > > and handles the I/O preparation based on the result.
> > > - Introduce `ublk_prep_auto_buf_reg_io` to encapsulate the logic for
> > > preparing the I/O for completion after buffer registration.
> > > - Pass the `tag` directly to `ublk_auto_buf_reg_fallback` to avoid
> > > recalculating it.
> > >
> > > This refactoring makes the control flow clearer and isolates the different
> > > stages of the auto buffer registration process.
> > >
> > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > > ---
> > > drivers/block/ublk_drv.c | 65 +++++++++++++++++++++++++++-------------
> > > 1 file changed, 44 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index 9185978abeb7..e53f623b0efe 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1205,17 +1205,36 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> > > }
> > >
> > > static void
> > > -ublk_auto_buf_reg_fallback(const struct ublk_queue *ubq, struct ublk_io *io)
> > > +ublk_auto_buf_reg_fallback(const struct ublk_queue *ubq, unsigned tag)
> > > {
> > > - unsigned tag = io - ubq->ios;
> >
> > The reason to calculate the tag like this was to avoid the pointer
> > dereference in req->tag. But req->tag is already accessed just prior
> > in ublk_dispatch_req(), so it should be cached and not too expensive
> > to load again.
>
> Ok, one thing is that ublk_auto_buf_reg_fallback() should be called in slow
> path...
What you have seems fine. Just providing some background on why I
wrote it like this.
Best,
Caleb
>
> >
> > > struct ublksrv_io_desc *iod = ublk_get_iod(ubq, tag);
> > >
> > > iod->op_flags |= UBLK_IO_F_NEED_REG_BUF;
> > > }
> > >
> > > -static bool ublk_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> > > - struct ublk_io *io, struct io_uring_cmd *cmd,
> > > - unsigned int issue_flags)
> > > +enum auto_buf_reg_res {
> > > + AUTO_BUF_REG_FAIL,
> > > + AUTO_BUF_REG_FALLBACK,
> > > + AUTO_BUF_REG_OK,
> > > +};
> >
> > nit: move this enum definition next to the function that returns it?
>
> Yeah, good point.
>
> >
> > > +
> > > +static void ublk_prep_auto_buf_reg_io(const struct ublk_queue *ubq,
> > > + struct request *req, struct ublk_io *io,
> > > + struct io_uring_cmd *cmd, bool registered)
> >
> > How about passing enum auto_buf_reg_res instead of bool registered to
> > avoid the duplicated == AUTO_BUF_REG_OK in the callers?
>
> OK, either way is fine for me.
>
> >
> > > +{
> > > + if (registered) {
> > > + io->task_registered_buffers = 1;
> > > + io->buf_ctx_handle = io_uring_cmd_ctx_handle(cmd);
> > > + io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> > > + }
> > > + ublk_init_req_ref(ubq, io);
> > > + __ublk_prep_compl_io_cmd(io, req);
> > > +}
> > > +
> > > +static enum auto_buf_reg_res
> > > +__ublk_do_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> > > + struct ublk_io *io, struct io_uring_cmd *cmd,
> > > + unsigned int issue_flags)
> > > {
> > > int ret;
> > >
> > > @@ -1223,29 +1242,27 @@ static bool ublk_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> > > io->buf.auto_reg.index, issue_flags);
> > > if (ret) {
> > > if (io->buf.auto_reg.flags & UBLK_AUTO_BUF_REG_FALLBACK) {
> > > - ublk_auto_buf_reg_fallback(ubq, io);
> > > - return true;
> > > + ublk_auto_buf_reg_fallback(ubq, req->tag);
> > > + return AUTO_BUF_REG_FALLBACK;
> > > }
> > > blk_mq_end_request(req, BLK_STS_IOERR);
> > > - return false;
> > > + return AUTO_BUF_REG_FAIL;
> > > }
> > >
> > > - io->task_registered_buffers = 1;
> > > - io->buf_ctx_handle = io_uring_cmd_ctx_handle(cmd);
> > > - io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> > > - return true;
> > > + return AUTO_BUF_REG_OK;
> > > }
> > >
> > > -static bool ublk_prep_auto_buf_reg(struct ublk_queue *ubq,
> > > - struct request *req, struct ublk_io *io,
> > > - struct io_uring_cmd *cmd,
> > > - unsigned int issue_flags)
> > > +static void ublk_do_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> > > + struct ublk_io *io, struct io_uring_cmd *cmd,
> > > + unsigned int issue_flags)
> > > {
> > > - ublk_init_req_ref(ubq, io);
> > > - if (ublk_support_auto_buf_reg(ubq) && ublk_rq_has_data(req))
> > > - return ublk_auto_buf_reg(ubq, req, io, cmd, issue_flags);
> > > + enum auto_buf_reg_res res = __ublk_do_auto_buf_reg(ubq, req, io, cmd,
> > > + issue_flags);
> > >
> > > - return true;
> > > + if (res != AUTO_BUF_REG_FAIL) {
> > > + ublk_prep_auto_buf_reg_io(ubq, req, io, cmd, res == AUTO_BUF_REG_OK);
> > > + io_uring_cmd_done(cmd, UBLK_IO_RES_OK, 0, issue_flags);
> > > + }
> > > }
> > >
> > > static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> > > @@ -1318,8 +1335,14 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> > > if (!ublk_start_io(ubq, req, io))
> > > return;
> > >
> > > - if (ublk_prep_auto_buf_reg(ubq, req, io, io->cmd, issue_flags))
> > > + if (ublk_support_auto_buf_reg(ubq) && ublk_rq_has_data(req)) {
> > > + struct io_uring_cmd *cmd = io->cmd;
> >
> > Don't really see the need for this intermediate variable
>
> Yes, will remove it, but the big thing is that there isn't io->cmd for BATCH_IO
> any more.
>
>
> Thanks,
> Ming
>
