Re: [PATCH V2 00/17] io_uring/ublk: add IORING_OP_FUSED_CMD

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

On 3/7/23 14:15, Ming Lei wrote:
> Hello,
> 
> Add IORING_OP_FUSED_CMD, it is one special URING_CMD, which has to
> be SQE128. The 1st SQE(master) is one 64byte URING_CMD, and the 2nd
> 64byte SQE(slave) is another normal 64byte OP. For any OP which needs
> to support slave OP, io_issue_defs[op].fused_slave needs to be set as 1,
> and its ->issue() can retrieve/import buffer from master request's
> fused_cmd_kbuf. The slave OP is actually submitted from kernel, part of
> this idea is from Xiaoguang's ublk ebpf patchset, but this patchset
> submits slave OP just like normal OP issued from userspace, that said,
> SQE order is kept, and batching handling is done too.

 From a quick look through patches it all looks a bit complicated
and intrusive, all over generic hot paths. I think instead we
should be able to use registered buffer table as intermediary and
reuse splicing. Let me try it out


> Please see detailed design in commit log of the 3th patch, and one big
> point is how to handle buffer ownership.
> 
> With this way, it is easy to support zero copy for ublk/fuse device.
> 
> Basically userspace can specify any sub-buffer of the ublk block request
> buffer from the fused command just by setting 'offset/len'
> in the slave SQE for running slave OP. This way is flexible to implement
> io mapping: mirror, stripped, ...
> 
> The 4th & 5th patches enable fused slave support for the following OPs:
> 
> 	OP_READ/OP_WRITE
> 	OP_SEND/OP_RECV/OP_SEND_ZC
> 
> The other ublk patches cleans ublk driver and implement fused command
> for supporting zero copy.
> 
> Follows userspace code:
> 
> https://github.com/ming1/ubdsrv/tree/fused-cmd-zc-v2
> 
> All three(loop, nbd and qcow2) ublk targets have supported zero copy by passing:
> 
> 	ublk add -t [loop|nbd|qcow2] -z ....
> 
> Basic fs mount/kernel building and builtin test are done.
> 
> Also add liburing test case for covering fused command based on miniublk
> of blktest:
> 
> https://github.com/ming1/liburing/commits/fused_cmd_miniublk
> 
> Performance improvement is obvious on memory bandwidth
> related workloads, such as, 1~2X improvement on 64K/512K BS
> IO test on loop with ramfs backing file.
> 
> Any comments are welcome!
> 
> V2:
> 	- don't resue io_mapped_ubuf (io_uring)
> 	- remove REQ_F_FUSED_MASTER_BIT (io_uring)
> 	- fix compile warning (io_uring)
> 	- rebase on v6.3-rc1 (io_uring)
> 	- grabbing io request reference when handling fused command
> 	- simplify ublk_copy_user_pages() by iov iterator
> 	- add read()/write() for userspace to read/write ublk io buffer, so
> 	that some corner cases(read zero, passthrough request(report zones)) can
> 	be handled easily in case of zero copy; this way also helps to switch to
> 	zero copy completely
> 	- misc cleanup
> 
> Ming Lei (17):
>    io_uring: add IO_URING_F_FUSED and prepare for supporting OP_FUSED_CMD
>    io_uring: increase io_kiocb->flags into 64bit
>    io_uring: add IORING_OP_FUSED_CMD
>    io_uring: support OP_READ/OP_WRITE for fused slave request
>    io_uring: support OP_SEND_ZC/OP_RECV for fused slave request
>    block: ublk_drv: mark device as LIVE before adding disk
>    block: ublk_drv: add common exit handling
>    block: ublk_drv: don't consider flush request in map/unmap io
>    block: ublk_drv: add two helpers to clean up map/unmap request
>    block: ublk_drv: clean up several helpers
>    block: ublk_drv: cleanup 'struct ublk_map_data'
>    block: ublk_drv: cleanup ublk_copy_user_pages
>    block: ublk_drv: grab request reference when the request is handled by
>      userspace
>    block: ublk_drv: support to copy any part of request pages
>    block: ublk_drv: add read()/write() support for ublk char device
>    block: ublk_drv: don't check buffer in case of zero copy
>    block: ublk_drv: apply io_uring FUSED_CMD for supporting zero copy
> 
>   drivers/block/ublk_drv.c       | 605 ++++++++++++++++++++++++++-------
>   drivers/char/mem.c             |   4 +
>   drivers/nvme/host/ioctl.c      |   9 +
>   include/linux/io_uring.h       |  49 ++-
>   include/linux/io_uring_types.h |  18 +-
>   include/uapi/linux/io_uring.h  |   1 +
>   include/uapi/linux/ublk_cmd.h  |  37 +-
>   io_uring/Makefile              |   2 +-
>   io_uring/fused_cmd.c           | 232 +++++++++++++
>   io_uring/fused_cmd.h           |  11 +
>   io_uring/io_uring.c            |  22 +-
>   io_uring/io_uring.h            |   3 +
>   io_uring/net.c                 |  23 +-
>   io_uring/opdef.c               |  17 +
>   io_uring/opdef.h               |   2 +
>   io_uring/rw.c                  |  20 ++
>   16 files changed, 926 insertions(+), 129 deletions(-)
>   create mode 100644 io_uring/fused_cmd.c
>   create mode 100644 io_uring/fused_cmd.h
> 

-- 
Pavel Begunkov