Re: [PATCH V6 07/24] ublk: add batch I/O dispatch infrastructure

[Jens Axboe <axboe@kernel.dk>]

Claude says:

> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index e5db5a552357..e7bd7e7b1731 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c

[ ... ]

> +static bool __ublk_batch_prep_dispatch(struct ublk_queue *ubq,
> +				       const struct ublk_batch_io_data *data,
> +				       unsigned short tag)
> +{
> +	struct ublk_device *ub = data->ub;
> +	struct ublk_io *io = &ubq->ios[tag];
> +	struct request *req = blk_mq_tag_to_rq(ub->tag_set.tags[ubq->q_id], tag);
> +	enum auto_buf_reg_res res = AUTO_BUF_REG_FALLBACK;
> +	struct io_uring_cmd *cmd = data->cmd;
> +
> +	if (!ublk_start_io(ubq, req, io))
> +		return false;
> +
> +	if (ublk_support_auto_buf_reg(ubq) && ublk_rq_has_data(req)) {
> +		res = __ublk_do_auto_buf_reg(ubq, req, io, cmd,
> +				data->issue_flags);
> +
> +		if (res == AUTO_BUF_REG_FAIL)
> +			return false;
> +	}
> +
> +	ublk_io_lock(io);
> +	ublk_prep_auto_buf_reg_io(ubq, req, io, cmd, res);
> +	ublk_io_unlock(io);
> +
> +	return true;
> +}

[ ... ]

> +#define MAX_NR_TAG 128
> +static int __ublk_batch_dispatch(struct ublk_queue *ubq,
> +				 const struct ublk_batch_io_data *data,
> +				 struct ublk_batch_fetch_cmd *fcmd)
> +{

[ ... ]

> +	sel.val = ublk_batch_copy_io_tags(fcmd, sel.addr, tag_buf, len * tag_sz);
> +	ret = ublk_batch_fetch_post_cqe(fcmd, &sel, data->issue_flags);
> +	if (unlikely(ret < 0)) {
> +		int i, res;
> +
> +		/*
> +		 * Undo prep state for all IOs since userspace never received them.
> +		 * This restores IOs to pre-prepared state so they can be cleanly
> +		 * re-prepared when tags are pulled from FIFO again.
> +		 */
> +		for (i = 0; i < len; i++) {
> +			struct ublk_io *io = &ubq->ios[tag_buf[i]];
> +			int index = -1;
> +
> +			ublk_io_lock(io);
> +			if (io->flags & UBLK_IO_FLAG_AUTO_BUF_REG)
> +				index = io->buf.auto_reg.index;
> +			io->flags &= ~(UBLK_IO_FLAG_OWNED_BY_SRV | UBLK_IO_FLAG_AUTO_BUF_REG);
> +			io->flags |= UBLK_IO_FLAG_ACTIVE;
> +			ublk_io_unlock(io);
> +
> +			if (index != -1)
> +				io_buffer_unregister_bvec(data->cmd, index,
> +						data->issue_flags);
> +		}

Should io->task_registered_buffers also be cleared here?

When __ublk_batch_prep_dispatch() succeeds with AUTO_BUF_REG_OK, the call to
ublk_prep_auto_buf_reg_io() sets io->task_registered_buffers = 1.  In this
error recovery path, the buffer is unregistered and UBLK_IO_FLAG_AUTO_BUF_REG
is cleared, but task_registered_buffers is not reset to 0.

If the tag is later re-dispatched and __ublk_do_auto_buf_reg() returns
AUTO_BUF_REG_FALLBACK (e.g., due to -ENOMEM), task_registered_buffers will
remain at 1 even though no buffer is actually registered.

This could affect ublk_sub_req_ref() which calculates:

    sub_refs = UBLK_REFCOUNT_INIT - io->task_registered_buffers

If task_registered_buffers is incorrectly 1 when it should be 0, the
refcount subtraction would leave an extra reference, potentially causing
the IO to never complete.

-- 
Jens Axboe