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@xxxxxxxxxx>
> ---
> 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
