Re: [PATCH V2 3/3] selftests: ublk: add ublk zero copy test

[Ming Lei <ming.lei@redhat.com>]

On Wed, Feb 26, 2025 at 10:41:43AM -0700, Keith Busch wrote:
> On Wed, Feb 26, 2025 at 11:58:38PM +0800, Ming Lei wrote:
> > +	struct io_uring_sqe *reg;
> > +	struct io_uring_sqe *rw;
> > +	struct io_uring_sqe *ureg;
> > +
> > +	if (!zc) {
> > +		rw = ublk_queue_alloc_sqe(q);
> > +		if (!rw)
> > +			return -ENOMEM;
> > +
> > +		io_uring_prep_rw(op, rw, 1 /*fds[1]*/,
> > +				(void *)iod->addr,
> > +				iod->nr_sectors << 9,
> > +				iod->start_sector << 9);
> > +		io_uring_sqe_set_flags(rw, IOSQE_FIXED_FILE);
> > +		q->io_inflight++;
> > +		/* bit63 marks us as tgt io */
> > +		rw->user_data = build_user_data(tag, op, UBLK_IO_TGT_NORMAL, 1);
> > +		return 0;
> > +	}
> > +
> > +	ublk_queue_alloc_sqe3(q, &reg, &rw, &ureg);
> > +
> > +	io_uring_prep_buf_register(reg, 0, tag, q->q_id, tag);
> > +	reg->user_data = build_user_data(tag, 0xfe, 1, 1);
> > +	reg->flags |= IOSQE_CQE_SKIP_SUCCESS;
> > +	reg->flags |= IOSQE_IO_LINK;
> > +
> > +	io_uring_prep_rw(op, rw, 1 /*fds[1]*/, 0,
> > +		iod->nr_sectors << 9,
> > +		iod->start_sector << 9);
> > +	rw->buf_index = tag;
> > +	rw->flags |= IOSQE_FIXED_FILE;
> > +	rw->flags |= IOSQE_IO_LINK;
> > +	rw->user_data = build_user_data(tag, op, UBLK_IO_TGT_ZC_OP, 1);
> > +	q->io_inflight++;
> > +
> > +	io_uring_prep_buf_unregister(ureg, 0, tag, q->q_id, tag);
> > +	ureg->user_data = build_user_data(tag, 0xff, UBLK_IO_TGT_ZC_BUF, 1);
> 
> You don't have anything handling the unregister command's completion so
> I think you want the IOSQE_CQE_SKIP_SUCCESS flag on it otherwise you get
> an unexpected CQE for it.

Please see ublk_loop_io_done() which does handle unregister command by
the flag of UBLK_IO_TGT_ZC_BUF. And it is reasonable to complete the
io command after the whole link is done.

BTW, with this way, one ublk bug is actually triggered, and Caleb's patch
fixes it.


Thanks,
Ming