On Tue, Jan 7, 2025 at 4:08 AM Ming Lei <tom.leiming@xxxxxxxxx> wrote:
> +
> +/* Return true if io cmd is queued, otherwise forward it to userspace */
> +bool ublk_run_bpf_handler(struct ublk_queue *ubq, struct request *req,
> + queue_io_cmd_t cb)
> +{
> + ublk_bpf_return_t ret;
> + struct ublk_rq_data *data = blk_mq_rq_to_pdu(req);
> + struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag);
> + struct ublk_bpf_io *bpf_io = &data->bpf_data;
> + const unsigned long total = iod->nr_sectors << 9;
> + unsigned int done = 0;
> + bool res = true;
> + int err;
> +
> + if (!test_bit(UBLK_BPF_IO_PREP, &bpf_io->flags))
> + ublk_bpf_prep_io(bpf_io, iod);
> +
> + do {
> + enum ublk_bpf_disposition rc;
> + unsigned int bytes;
> +
> + ret = cb(bpf_io, done);
High level observation...
I suspect forcing all sturct_ops callbacks to have only these
two arguments and packing args into ublk_bpf_io
will be limiting in the long term.
And this part of api would need to be redesigned,
but since it's not an uapi... not a big deal.
> + rc = ublk_bpf_get_disposition(ret);
> +
> + if (rc == UBLK_BPF_IO_QUEUED)
> + goto exit;
> +
> + if (rc == UBLK_BPF_IO_REDIRECT)
> + break;
Same point about return value processing...
Each struct_ops callback could have had its own meaning
of retvals.
I suspect it would have been more flexible and more powerful
this way.
Other than that bpf plumbing looks good.
There is an issue with leaking allocated memory in bpf_aio_alloc kfunc
(it probably should be KF_ACQUIRE)
and a few other things, but before doing any in depth review
from bpf pov I'd like to hear what block folks think.
Motivation looks useful,
but the claim of performance gains without performance numbers
is a leap of faith.
> +
> + if (unlikely(rc != UBLK_BPF_IO_CONTINUE)) {
> + printk_ratelimited(KERN_ERR "%s: unknown rc code %d\n",
> + __func__, rc);
> + err = -EINVAL;
> + goto fail;
> + }
> +
> + bytes = ublk_bpf_get_return_bytes(ret);
> + if (unlikely((bytes & 511) || !bytes)) {
> + err = -EREMOTEIO;
> + goto fail;
> + } else if (unlikely(bytes > total - done)) {
> + err = -ENOSPC;
> + goto fail;
> + } else {
> + done += bytes;
> + }
> + } while (done < total);
