On Thu, Feb 16, 2023 at 04:58:00PM +0800, Ming Lei wrote:

Hi Shinichiro,

Thanks for the review!

On Thu, Feb 16, 2023 at 08:19:39AM +0000, Shinichiro Kawasaki wrote:

quoted

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.

OK.

quoted

On Feb 16, 2023 / 11:01, Ming Lei wrote:

quoted

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 <redacted>
---
 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)

I tried this way, and it fails in case that liburing is built
against source tree directly. And liburing2.2 is still a bit new, and even
some distributions doesn't package it. I will think about other way
for the check.

Looks the following way works:

diff --git a/src/Makefile b/src/Makefile
index 83e8a62..adfd27a 100644
--- a/src/Makefile
+++ b/src/Makefile

@@ -2,6 +2,10 @@ HAVE_C_HEADER = $(shell if echo "\#include <$(1)>" |		\
 		$(CC) -E - > /dev/null 2>&1; then echo "$(2)";	\
 		else echo "$(3)"; fi)
 
+HAVE_C_MACRO = $(shell if echo "#include <$(1)>" | \
+		$(CC) -E - | grep $(2) > /dev/null 2>&1; then echo 1;	\
+		else echo 0; fi)
+
 C_TARGETS := \
 	loblksize \
 	loop_change_fd \

@@ -15,25 +19,30 @@ C_TARGETS := \
 
 C_MINIUBLK := ublk/miniublk
 
+HAVE_LIBURING := $(call HAVE_C_MACRO,liburing.h,IORING_OP_URING_CMD)
+
 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)
 
 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
+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)
 

Thanks, 
Ming