Re: [PATCHv2 4/6] ublk: zc register/unregister bvec

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/11/25 00:56, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Provide new operations for the user to request mapping an active request
> to an io uring instance's buf_table. The user has to provide the index
> it wants to install the buffer.
> 
> A reference count is taken on the request to ensure it can't be
> completed while it is active in a ring's buf_table.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/block/ublk_drv.c      | 145 +++++++++++++++++++++++++---------
>   include/uapi/linux/ublk_cmd.h |   4 +
>   2 files changed, 113 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 529085181f355..ccfda7b2c24da 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -51,6 +51,9 @@
...
> +static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> +				  struct ublk_queue *ubq, int tag,
> +				  const struct ublksrv_io_cmd *ub_cmd)
> +{
> +	struct io_ring_ctx *ctx = cmd_to_io_kiocb(cmd)->ctx;
> +	struct ublk_device *ub = cmd->file->private_data;
> +	int index = (int)ub_cmd->addr;
> +	struct ublk_rq_data *data;
> +	struct request *req;
> +
> +	if (!ub)
> +		return -EPERM;
> +
> +	req = blk_mq_tag_to_rq(ub->tag_set.tags[ubq->q_id], tag);
> +	if (!req)
> +		return -EINVAL;
> +
> +	data = blk_mq_rq_to_pdu(req);
> +	if (!test_and_clear_bit(UBLK_ZC_REGISTERED, &data->flags))
> +		return -EINVAL;

Why is it cleared here but not when it's auto removed?
Do you even need it? For example, take the option of not having
UBLK_U_IO_UNREGISTER_IO_BUF and doing all unregistrations the
normal io_uring way. Then you install it, and you know it'll
be released once and no protection needed.

For ublk_unregister_io_buf() it prevents multiple unregistrations,
even though io_uring would tolerate that, and I don't see
anything else meaningful going on here on the ublk side.

Btw, if you do it via ublk like this, then I agree, it's an
additional callback, though it can be made fancier in the
future. E.g. peeking at the {release,priv} and avoid flagging
above, and so on (though maybe flagging helps with ref
overflows).

All that aside, it looks fine in general. The only concern
is ublk going away before all buffers are released, but
maybe it does waits for all its requests to compelte?

> +
> +	io_buffer_unregister_bvec(ctx, index);

Please pass issue_flags into the helper and do conditional
locking inside. I understand that you rely on IO_URING_F_UNLOCKED
checks in ublk, but those are an abuse of io_uring internal
details by ublk. ublk should've never been allowed to interpret
issue_flags.

-- 
Pavel Begunkov