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...
>
> > 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
