Re: [PATCH 1/2] blktests/src: add mini ublk source code

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

Hi Ming, thanks for the patches. It sounds a good idea to extend blktests
coverage to ublk.

Regarding the commit title, I suggest this:

   src: add mini ublk source codes

The word "blktests" can be placed after the word "PATCH" as follows:

   [PATCH blktests] src: add mini ublk source codes

Please try --subject-prefix="PATCH blktests" option for git format-patch.

On Feb 16, 2023 / 11:01, Ming Lei wrote:
> Prepare for adding ublk related test:
> 
> 1) ublk delete is sync removal, this way is convenient to
>    blkg/queue/disk instance leak issue
> 
> 2) mini ublk has two builtin target(null, loop), and loop IO is
> handled by io_uring, so we can use ublk to cover part of io_uring
> workloads
> 
> 3) not like loop/nbd, ublk won't pre-allocate/add disk, and always
> add/delete disk dynamically, this way may cover disk plug & unplug
> tests
> 
> 4) ublk specific test given people starts to use it, so better to
> let blktest cover ublk related tests
> 
> Add mini ublk source for test purpose only, which is easy to use:
> 
> ./miniublk add -t {null|loop} [-q nr_queues] [-d depth] [-n dev_id]
> 	 default: nr_queues=2(max 4), depth=128(max 128), dev_id=-1(auto allocation)
> 	 -t loop -f backing_file
> 	 -t null
> ./miniublk del [-n dev_id] [--disk/-d disk_path ] -a
> 	 -a delete all devices, -n delete specified device
> ./miniublk list [-n dev_id] -a
> 	 -a list all devices, -n list specified device, default -a
> 
> ublk depends on liburing 2.2, so allow to ignore ublk build failure
> in case of missing liburing, and we will check if ublk program exits
> inside test.
> 
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  Makefile            |    2 +-
>  src/Makefile        |   13 +-
>  src/ublk/miniublk.c | 1385 +++++++++++++++++++++++++++++++++++++++++++
>  src/ublk/ublk_cmd.h |  278 +++++++++
>  4 files changed, 1674 insertions(+), 4 deletions(-)
>  create mode 100644 src/ublk/miniublk.c
>  create mode 100644 src/ublk/ublk_cmd.h
> 
> diff --git a/Makefile b/Makefile
> index 5a04479..b9bbade 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2,7 +2,7 @@ prefix ?= /usr/local
>  dest = $(DESTDIR)$(prefix)/blktests
>  
>  all:
> -	$(MAKE) -C src all
> +	$(MAKE) -i -C src all

This -i option to ignore errors is applied to all build targets, so it will hide
errors. Instead os ignoring the error, how about checking the liburing version
with pkg-config command? I roughly implemented it as the patch below on top of
your patch. It looks working.

diff --git a/src/Makefile b/src/Makefile
index 9bb8da6..c600099 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -2,6 +2,11 @@ HAVE_C_HEADER = $(shell if echo "\#include <$(1)>" |		\
 		$(CC) -E - > /dev/null 2>&1; then echo "$(2)";	\
 		else echo "$(3)"; fi)
 
+HAVE_PACKAGE_VER = $(shell if pkg-config --atleast-version="$(2)" "$(1)"; \
+			then echo 1; else echo 0; fi)
+
+HAVE_LIBURING := $(call HAVE_PACKAGE_VER,liburing,2.2)
+
 C_TARGETS := \
 	loblksize \
 	loop_change_fd \
@@ -18,8 +23,11 @@ C_MINIUBLK := ublk/miniublk
 CXX_TARGETS := \
 	discontiguous-io
 
+ifeq ($(HAVE_LIBURING), 1)
+TARGETS := $(C_TARGETS) $(CXX_TARGETS) $(C_MINIUBLK)
+else
 TARGETS := $(C_TARGETS) $(CXX_TARGETS)
-ALL_TARGETS := $(TARGETS) $(C_MINIUBLK)
+endif
 
 CONFIG_DEFS := $(call HAVE_C_HEADER,linux/blkzoned.h,-DHAVE_LINUX_BLKZONED_H)
 
@@ -28,12 +36,12 @@ override CXXFLAGS := -O2 -std=c++11 -Wall -Wextra -Wshadow -Wno-sign-compare \
 		     -Werror $(CXXFLAGS) $(CONFIG_DEFS)
 MINIUBLK_FLAGS :=  -D_GNU_SOURCE -lpthread -luring -Iublk
 
-all: $(ALL_TARGETS)
+all: $(TARGETS)
 
 clean:
-	rm -f $(ALL_TARGETS)
+	rm -f $(TARGETS)
 
-install: $(ALL_TARGETS)
+install: $(TARGETS)
 	install -m755 -d $(dest)
 	install $(TARGETS) $(dest)
 


>  
>  clean:
>  	$(MAKE) -C src clean
> diff --git a/src/Makefile b/src/Makefile
> index 3b587f6..83e8a62 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -13,23 +13,27 @@ C_TARGETS := \
>  	sg/syzkaller1 \
>  	zbdioctl
>  
> +C_MINIUBLK := ublk/miniublk
> +
>  CXX_TARGETS := \
>  	discontiguous-io
>  
>  TARGETS := $(C_TARGETS) $(CXX_TARGETS)
> +ALL_TARGETS := $(TARGETS) $(C_MINIUBLK)
>  
>  CONFIG_DEFS := $(call HAVE_C_HEADER,linux/blkzoned.h,-DHAVE_LINUX_BLKZONED_H)
>  
>  override CFLAGS   := -O2 -Wall -Wshadow $(CFLAGS) $(CONFIG_DEFS)
>  override CXXFLAGS := -O2 -std=c++11 -Wall -Wextra -Wshadow -Wno-sign-compare \
>  		     -Werror $(CXXFLAGS) $(CONFIG_DEFS)
> +MINIUBLK_FLAGS :=  -D_GNU_SOURCE -lpthread -luring

In my envronemen, -Iublk was required also to avoid a build error.

>  
> -all: $(TARGETS)
> +all: $(ALL_TARGETS)
>  
>  clean:
> -	rm -f $(TARGETS)
> +	rm -f $(ALL_TARGETS)
>  
> -install: $(TARGETS)
> +install: $(ALL_TARGETS)
>  	install -m755 -d $(dest)
>  	install $(TARGETS) $(dest)
>  
> @@ -39,4 +43,7 @@ $(C_TARGETS): %: %.c
>  $(CXX_TARGETS): %: %.cpp
>  	$(CXX) $(CPPFLAGS) $(CXXFLAGS) -o $@ $^
>  
> +$(C_MINIUBLK): %: ublk/miniublk.c
> +	$(CC) $(CFLAGS) $(MINIUBLK_FLAGS) -o $@ ublk/miniublk.c
> +
>  .PHONY: all clean install
> diff --git a/src/ublk/miniublk.c b/src/ublk/miniublk.c
> new file mode 100644
> index 0000000..dc80246
> --- /dev/null
> +++ b/src/ublk/miniublk.c
> @@ -0,0 +1,1385 @@
> +// SPDX-License-Identifier: GPL-2.0+

Many of the blktests files have the license GPL-3.0+. GPL-2.0+ should be fine,
but is there reasoning to have GPL-2.0+?

> +// Copyright (C) 2023 Ming Lei
> +
> +/*
> + * io_uring based mini ublk implementation with null/loop target,
> + * for test purpose only.
> + *
> + * So please keep it simple & reliable.
> + */
[...]

> diff --git a/src/ublk/ublk_cmd.h b/src/ublk/ublk_cmd.h
> new file mode 100644
> index 0000000..f6238cc
> --- /dev/null
> +++ b/src/ublk/ublk_cmd.h
> @@ -0,0 +1,278 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */

This license is new to blktests. Do we need this license to this header file?
Ah, is this file copied from linux kernel source tree? If so, I would like to
avoid the copy and the duplication.

> +#ifndef USER_BLK_DRV_CMD_INC_H
> +#define USER_BLK_DRV_CMD_INC_H
> +
> +#include <linux/types.h>
> +
> +/* ublk server command definition */
> +
> +/*
> + * Admin commands, issued by ublk server, and handled by ublk driver.
> + */
> +#define	UBLK_CMD_GET_QUEUE_AFFINITY	0x01
> +#define	UBLK_CMD_GET_DEV_INFO	0x02
> +#define	UBLK_CMD_ADD_DEV		0x04
> +#define	UBLK_CMD_DEL_DEV		0x05
> +#define	UBLK_CMD_START_DEV	0x06
> +#define	UBLK_CMD_STOP_DEV	0x07
> +#define	UBLK_CMD_SET_PARAMS	0x08
> +#define	UBLK_CMD_GET_PARAMS	0x09
> +#define	UBLK_CMD_START_USER_RECOVERY	0x10
> +#define	UBLK_CMD_END_USER_RECOVERY	0x11
> +#define	UBLK_CMD_GET_DEV_INFO2		0x12
> +
> +/*
> + * IO commands, issued by ublk server, and handled by ublk driver.
> + *
> + * FETCH_REQ: issued via sqe(URING_CMD) beforehand for fetching IO request
> + *      from ublk driver, should be issued only when starting device. After
> + *      the associated cqe is returned, request's tag can be retrieved via
> + *      cqe->userdata.
> + *
> + * COMMIT_AND_FETCH_REQ: issued via sqe(URING_CMD) after ublkserver handled
> + *      this IO request, request's handling result is committed to ublk
> + *      driver, meantime FETCH_REQ is piggyback, and FETCH_REQ has to be
> + *      handled before completing io request.
> + *
> + * NEED_GET_DATA: only used for write requests to set io addr and copy data
> + *      When NEED_GET_DATA is set, ublksrv has to issue UBLK_IO_NEED_GET_DATA
> + *      command after ublk driver returns UBLK_IO_RES_NEED_GET_DATA.
> + *
> + *      It is only used if ublksrv set UBLK_F_NEED_GET_DATA flag
> + *      while starting a ublk device.
> + */
> +#define	UBLK_IO_FETCH_REQ		0x20
> +#define	UBLK_IO_COMMIT_AND_FETCH_REQ	0x21
> +#define	UBLK_IO_NEED_GET_DATA	0x22
> +
> +/* only ABORT means that no re-fetch */
> +#define UBLK_IO_RES_OK			0
> +#define UBLK_IO_RES_NEED_GET_DATA	1
> +#define UBLK_IO_RES_ABORT		(-ENODEV)
> +
> +#define UBLKSRV_CMD_BUF_OFFSET	0
> +#define UBLKSRV_IO_BUF_OFFSET	0x80000000
> +
> +/* tag bit is 12bit, so at most 4096 IOs for each queue */
> +#define UBLK_MAX_QUEUE_DEPTH	4096
> +
> +/*
> + * zero copy requires 4k block size, and can remap ublk driver's io
> + * request into ublksrv's vm space
> + */
> +#define UBLK_F_SUPPORT_ZERO_COPY	(1ULL << 0)
> +
> +/*
> + * Force to complete io cmd via io_uring_cmd_complete_in_task so that
> + * performance comparison is done easily with using task_work_add
> + */
> +#define UBLK_F_URING_CMD_COMP_IN_TASK	(1ULL << 1)
> +
> +/*
> + * User should issue io cmd again for write requests to
> + * set io buffer address and copy data from bio vectors
> + * to the userspace io buffer.
> + *
> + * In this mode, task_work is not used.
> + */
> +#define UBLK_F_NEED_GET_DATA (1UL << 2)
> +
> +#define UBLK_F_USER_RECOVERY	(1UL << 3)
> +
> +#define UBLK_F_USER_RECOVERY_REISSUE	(1UL << 4)
> +
> +/*
> + * Unprivileged user can create /dev/ublkcN and /dev/ublkbN.
> + *
> + * /dev/ublk-control needs to be available for unprivileged user, and it
> + * can be done via udev rule to make all control commands available to
> + * unprivileged user. Except for the command of UBLK_CMD_ADD_DEV, all
> + * other commands are only allowed for the owner of the specified device.
> + *
> + * When userspace sends UBLK_CMD_ADD_DEV, the device pair's owner_uid and
> + * owner_gid are stored to ublksrv_ctrl_dev_info by kernel, so far only
> + * the current user's uid/gid is stored, that said owner of the created
> + * device is always the current user.
> + *
> + * We still need udev rule to apply OWNER/GROUP with the stored owner_uid
> + * and owner_gid.
> + *
> + * Then ublk server can be run as unprivileged user, and /dev/ublkbN can
> + * be accessed and managed by its owner represented by owner_uid/owner_gid.
> + */
> +#define UBLK_F_UNPRIVILEGED_DEV	(1UL << 5)
> +
> +/* device state */
> +#define UBLK_S_DEV_DEAD	0
> +#define UBLK_S_DEV_LIVE	1
> +#define UBLK_S_DEV_QUIESCED	2
> +
> +/* shipped via sqe->cmd of io_uring command */
> +struct ublksrv_ctrl_cmd {
> +	/* sent to which device, must be valid */
> +	__u32	dev_id;
> +
> +	/* sent to which queue, must be -1 if the cmd isn't for queue */
> +	__u16	queue_id;
> +	/*
> +	 * cmd specific buffer, can be IN or OUT.
> +	 */
> +	__u16	len;
> +	__u64	addr;
> +
> +	/* inline data */
> +	__u64	data[1];
> +
> +	/*
> +	 * Used for UBLK_F_UNPRIVILEGED_DEV and UBLK_CMD_GET_DEV_INFO2
> +	 * only, include null char
> +	 */
> +	__u16	dev_path_len;
> +	__u16	pad;
> +	__u32	reserved;
> +};
> +
> +struct ublksrv_ctrl_dev_info {
> +	__u16	nr_hw_queues;
> +	__u16	queue_depth;
> +	__u16	state;
> +	__u16	pad0;
> +
> +	__u32	max_io_buf_bytes;
> +	__u32	dev_id;
> +
> +	__s32	ublksrv_pid;
> +	__u32	pad1;
> +
> +	__u64	flags;
> +
> +	/* For ublksrv internal use, invisible to ublk driver */
> +	__u64	ublksrv_flags;
> +
> +	__u32	owner_uid;	/* store by kernel */
> +	__u32	owner_gid;	/* store by kernel */
> +	__u64	reserved1;
> +	__u64   reserved2;
> +};
> +
> +#define		UBLK_IO_OP_READ		0
> +#define		UBLK_IO_OP_WRITE		1
> +#define		UBLK_IO_OP_FLUSH		2
> +#define		UBLK_IO_OP_DISCARD	3
> +#define		UBLK_IO_OP_WRITE_SAME	4
> +#define		UBLK_IO_OP_WRITE_ZEROES	5
> +
> +#define		UBLK_IO_F_FAILFAST_DEV		(1U << 8)
> +#define		UBLK_IO_F_FAILFAST_TRANSPORT	(1U << 9)
> +#define		UBLK_IO_F_FAILFAST_DRIVER	(1U << 10)
> +#define		UBLK_IO_F_META			(1U << 11)
> +#define		UBLK_IO_F_FUA			(1U << 13)
> +#define		UBLK_IO_F_NOUNMAP		(1U << 15)
> +#define		UBLK_IO_F_SWAP			(1U << 16)
> +
> +/*
> + * io cmd is described by this structure, and stored in share memory, indexed
> + * by request tag.
> + *
> + * The data is stored by ublk driver, and read by ublksrv after one fetch command
> + * returns.
> + */
> +struct ublksrv_io_desc {
> +	/* op: bit 0-7, flags: bit 8-31 */
> +	__u32		op_flags;
> +
> +	__u32		nr_sectors;
> +
> +	/* start sector for this io */
> +	__u64		start_sector;
> +
> +	/* buffer address in ublksrv daemon vm space, from ublk driver */
> +	__u64		addr;
> +};
> +
> +static inline __u8 ublksrv_get_op(const struct ublksrv_io_desc *iod)
> +{
> +	return iod->op_flags & 0xff;
> +}
> +
> +static inline __u32 ublksrv_get_flags(const struct ublksrv_io_desc *iod)
> +{
> +	return iod->op_flags >> 8;
> +}
> +
> +/* issued to ublk driver via /dev/ublkcN */
> +struct ublksrv_io_cmd {
> +	__u16	q_id;
> +
> +	/* for fetch/commit which result */
> +	__u16	tag;
> +
> +	/* io result, it is valid for COMMIT* command only */
> +	__s32	result;
> +
> +	/*
> +	 * userspace buffer address in ublksrv daemon process, valid for
> +	 * FETCH* command only
> +	 */
> +	__u64	addr;
> +};
> +
> +struct ublk_param_basic {
> +#define UBLK_ATTR_READ_ONLY            (1 << 0)
> +#define UBLK_ATTR_ROTATIONAL           (1 << 1)
> +#define UBLK_ATTR_VOLATILE_CACHE       (1 << 2)
> +#define UBLK_ATTR_FUA                  (1 << 3)
> +	__u32	attrs;
> +	__u8	logical_bs_shift;
> +	__u8	physical_bs_shift;
> +	__u8	io_opt_shift;
> +	__u8	io_min_shift;
> +
> +	__u32	max_sectors;
> +	__u32	chunk_sectors;
> +
> +	__u64   dev_sectors;
> +	__u64   virt_boundary_mask;
> +};
> +
> +struct ublk_param_discard {
> +	__u32	discard_alignment;
> +
> +	__u32	discard_granularity;
> +	__u32	max_discard_sectors;
> +
> +	__u32	max_write_zeroes_sectors;
> +	__u16	max_discard_segments;
> +	__u16	reserved0;
> +};
> +
> +/*
> + * read-only, can't set via UBLK_CMD_SET_PARAMS, disk_devt is available
> + * after device is started
> + */
> +struct ublk_param_devt {
> +	__u32   char_major;
> +	__u32   char_minor;
> +	__u32   disk_major;
> +	__u32   disk_minor;
> +};
> +
> +struct ublk_params {
> +	/*
> +	 * Total length of parameters, userspace has to set 'len' for both
> +	 * SET_PARAMS and GET_PARAMS command, and driver may update len
> +	 * if two sides use different version of 'ublk_params', same with
> +	 * 'types' fields.
> +	 */
> +	__u32	len;
> +#define UBLK_PARAM_TYPE_BASIC           (1 << 0)
> +#define UBLK_PARAM_TYPE_DISCARD         (1 << 1)
> +#define UBLK_PARAM_TYPE_DEVT            (1 << 2)
> +	__u32	types;			/* types of parameter included */
> +
> +	struct ublk_param_basic		basic;
> +	struct ublk_param_discard	discard;
> +	struct ublk_param_devt		devt;
> +};
> +
> +#endif
> -- 
> 2.31.1
> 

-- 
Shin'ichiro Kawasaki