[PATCH v2 0/4] miscellaneous ZBD improvements

[PATCH v2 0/4] miscellaneous ZBD improvements

[Dmitry Fomichev]

This series contains four patches related to zoned block devices.

v1 -> v2:
  - removed recursive zone locking in zbd_reset_zone()
  - added --disable-libzbc configure option
  - moved pkg-config version check to a separate function
  - added a patch to verify that pkg-config is available to check
    the minimum supported version in all libraries that need that

Dmitry Fomichev (4):
  configure: improve libzbc version check
  configure: check if pkg-config is installed
  zbd: simplify zone reset code
  t/zbd: check log file for failed assertions

 configure              | 61 ++++++++++++++++++---------------
 t/zbd/test-zbd-support |  9 ++++-
 zbd.c                  | 76 +++++++++++++++---------------------------
 3 files changed, 69 insertions(+), 77 deletions(-)

-- 
2.21.0

[PATCH v2 1/4] configure: improve libzbc version check

[Dmitry Fomichev]

Avoid parsing pkg-config output and just use --atleast-version to
check if libzbc is present and has an up to date version.

Currently, support for libzbc ioengine is always included if libzbc is
found at the build system. Add the option to disable libzbc ioengine
support even if libzbc is found. This can be useful for
cross-compilation.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 configure | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/configure b/configure
index 5925e94f..9a5f35f2 100755
--- a/configure
+++ b/configure
@@ -213,6 +213,8 @@ for opt do
   ;;
   --enable-libnbd) libnbd="yes"
   ;;
+  --disable-libzbc) libzbc="no"
+  ;;
   --disable-tcmalloc) disable_tcmalloc="yes"
   ;;
   --enable-libaio-uring) libaio_uring="yes"
@@ -256,6 +258,7 @@ if test "$show_help" = "yes" ; then
   echo "--with-ime=             Install path for DDN's Infinite Memory Engine"
   echo "--enable-libiscsi       Enable iscsi support"
   echo "--enable-libnbd         Enable libnbd (NBD engine) support"
+  echo "--disable-libzbc        Disable libzbc even if found"
   echo "--disable-tcmalloc	Disable tcmalloc support"
   echo "--enable-libaio-uring   Enable libaio emulated over io_uring"
   echo "--dynamic-libengines	Lib-based ioengines as dynamic libraries"
@@ -2454,9 +2457,6 @@ fi
 
 ##########################################
 # libzbc probe
-if test "$libzbc" != "yes" ; then
-  libzbc="no"
-fi
 cat > $TMPC << EOF
 #include <libzbc/zbc.h>
 int main(int argc, char **argv)
@@ -2466,19 +2466,21 @@ int main(int argc, char **argv)
   return zbc_open("foo=bar", O_RDONLY, &dev);
 }
 EOF
-if compile_prog "" "-lzbc" "libzbc"; then
-  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-9]*//')
-  if test "$libzbcvermaj" -ge "5" ; then
-    libzbc="yes"
+if test "$libzbc" != "no" ; then
+  if compile_prog "" "-lzbc" "libzbc"; then
+    minimum_libzbc=5
+    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
+      libzbc="yes"
+    else
+      print_config "libzbc engine" "libzbc version $minimum_libzbc or above required"
+      libzbc="no"
+    fi
   else
-    print_config "libzbc engine" "Unsupported libzbc version (version 5 or above required)"
+    if test "$libzbc" = "yes" ; then
+      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
+    fi
     libzbc="no"
   fi
-else
-  if test "$libzbc" = "yes" ; then
-      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
-  fi
-  libzbc="no"
 fi
 print_config "libzbc engine" "$libzbc"
 
-- 
2.21.0

[PATCH v2 2/4] configure: check if pkg-config is installed

[Dmitry Fomichev]

A few libraries need to be newer than a specific version in order to be
supported by fio and pkg-config utility is used to verify the versions
of the installed libraries. Since this step may fail because pkg-config
is not installed, verify pkg-config presence and warn the user if it
could not be found.

To avoid code duplication, add a common helper function to perform
these checks.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 configure | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/configure b/configure
index 9a5f35f2..01fa5d1c 100755
--- a/configure
+++ b/configure
@@ -133,6 +133,23 @@ output_sym() {
   echo "#define $1" >> $config_host_h
 }
 
+check_min_lib_version() {
+  local feature
+
+  if ${cross_prefix}pkg-config --atleast-version=$2 $1 > /dev/null 2>&1; then
+    return 0
+  fi
+  if ${cross_prefix}pkg-config --version > /dev/null 2>&1; then
+    feature=${3:=$1}
+    if test ${!feature} = "yes" ; then
+      feature_not_found "" "$1 >= $2"
+    fi
+  else
+    print_config "$1" "missing pkg-config, can't check library version"
+  fi
+  return 1
+}
+
 targetos=""
 cpu=""
 
@@ -1520,18 +1537,17 @@ if test "$?" != "0" ; then
   echo "configure: gtk and gthread not found"
   exit 1
 fi
-if ! ${cross_prefix}pkg-config --atleast-version 2.18.0 gtk+-2.0; then
-  echo "GTK found, but need version 2.18 or higher"
-  gfio="no"
-else
+gfio="yes"
+if check_min_lib_version gtk+-2.0 2.18.0 "gfio"; then
   if compile_prog "$GTK_CFLAGS" "$GTK_LIBS" "gfio" ; then
-    gfio="yes"
     GFIO_LIBS="$LIBS $GTK_LIBS"
     CFLAGS="$CFLAGS $GTK_CFLAGS"
   else
     echo "Please install gtk and gdk libraries"
     gfio="no"
   fi
+else
+  gfio="no"
 fi
 LDFLAGS=$ORG_LDFLAGS
 fi
@@ -2181,15 +2197,11 @@ print_config "DDN's Infinite Memory Engine" "$libime"
 ##########################################
 # Check if we have libiscsi
 if test "$libiscsi" != "no" ; then
-  minimum_libiscsi=1.9.0
-  if $(pkg-config --atleast-version=$minimum_libiscsi libiscsi); then
+  if check_min_lib_version libiscsi 1.9.0; then
     libiscsi="yes"
     libiscsi_cflags=$(pkg-config --cflags libiscsi)
     libiscsi_libs=$(pkg-config --libs libiscsi)
   else
-    if test "$libiscsi" = "yes" ; then
-      feature_not_found "libiscsi" "libiscsi >= $minimum_libiscsi"
-    fi
     libiscsi="no"
   fi
 fi
@@ -2198,15 +2210,11 @@ print_config "iscsi engine" "$libiscsi"
 ##########################################
 # Check if we have libnbd (for NBD support)
 if test "$libnbd" != "no" ; then
-  minimum_libnbd=0.9.8
-  if $(pkg-config --atleast-version=$minimum_libnbd libnbd); then
+  if check_min_lib_version libnbd 0.9.8; then
     libnbd="yes"
     libnbd_cflags=$(pkg-config --cflags libnbd)
     libnbd_libs=$(pkg-config --libs libnbd)
   else
-    if test "$libnbd" = "yes" ; then
-      feature_not_found "libnbd" "libnbd >= $minimum_libnbd"
-    fi
     libnbd="no"
   fi
 fi
@@ -2468,11 +2476,8 @@ int main(int argc, char **argv)
 EOF
 if test "$libzbc" != "no" ; then
   if compile_prog "" "-lzbc" "libzbc"; then
-    minimum_libzbc=5
-    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
-      libzbc="yes"
-    else
-      print_config "libzbc engine" "libzbc version $minimum_libzbc or above required"
+    libzbc="yes"
+    if ! check_min_lib_version libzbc 5; then
       libzbc="no"
     fi
   else
-- 
2.21.0

[PATCH v2 3/4] zbd: simplify zone reset code

[Dmitry Fomichev]

zbd_reset_range() function is only called once from zbd_reset_zone()
and it is always called for an individual zone, not a range.

Make zone reset flow simpler by moving all the code needed
to reset a single zone from zbd_reset_range() to zbd_reset_zone().
Therefore, zbd_reset_range() is now dropped.

zbc_reset_zone() is always called with the zone already locked. Remove
recursive zone locking inside this function and add a note in the
description of this function saying that the caller is expected to have
the zone locked when calling it.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 zbd.c | 76 +++++++++++++++++++++--------------------------------------
 1 file changed, 27 insertions(+), 49 deletions(-)

diff --git a/zbd.c b/zbd.c
index 3eac5df3..e4a480b7 100644
--- a/zbd.c
+++ b/zbd.c
@@ -670,54 +670,6 @@ int zbd_setup_files(struct thread_data *td)
 	return 0;
 }
 
-/**
- * zbd_reset_range - reset zones for a range of sectors
- * @td: FIO thread data.
- * @f: Fio file for which to reset zones
- * @sector: Starting sector in units of 512 bytes
- * @nr_sectors: Number of sectors in units of 512 bytes
- *
- * Returns 0 upon success and a negative error code upon failure.
- */
-static int zbd_reset_range(struct thread_data *td, struct fio_file *f,
-			   uint64_t offset, uint64_t length)
-{
-	uint32_t zone_idx_b, zone_idx_e;
-	struct fio_zone_info *zb, *ze, *z;
-	int ret = 0;
-
-	assert(is_valid_offset(f, offset + length - 1));
-
-	switch (f->zbd_info->model) {
-	case ZBD_HOST_AWARE:
-	case ZBD_HOST_MANAGED:
-		ret = zbd_reset_wp(td, f, offset, length);
-		if (ret < 0)
-			return ret;
-		break;
-	default:
-		break;
-	}
-
-	zone_idx_b = zbd_zone_idx(f, offset);
-	zb = &f->zbd_info->zone_info[zone_idx_b];
-	zone_idx_e = zbd_zone_idx(f, offset + length);
-	ze = &f->zbd_info->zone_info[zone_idx_e];
-	for (z = zb; z < ze; z++) {
-		pthread_mutex_lock(&z->mutex);
-		pthread_mutex_lock(&f->zbd_info->mutex);
-		f->zbd_info->sectors_with_data -= z->wp - z->start;
-		pthread_mutex_unlock(&f->zbd_info->mutex);
-		z->wp = z->start;
-		z->verify_block = 0;
-		pthread_mutex_unlock(&z->mutex);
-	}
-
-	td->ts.nr_zone_resets += ze - zb;
-
-	return ret;
-}
-
 static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
 				struct fio_zone_info *zone)
 {
@@ -731,14 +683,40 @@ static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
  * @z: Zone to reset.
  *
  * Returns 0 upon success and a negative error code upon failure.
+ *
+ * The caller must hold z->mutex.
  */
 static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 			  struct fio_zone_info *z)
 {
+	uint64_t offset = z->start;
+	uint64_t length = (z+1)->start - offset;
+	int ret = 0;
+
+	assert(is_valid_offset(f, offset + length - 1));
+
 	dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
 		zbd_zone_nr(f->zbd_info, z));
+	switch (f->zbd_info->model) {
+	case ZBD_HOST_AWARE:
+	case ZBD_HOST_MANAGED:
+		ret = zbd_reset_wp(td, f, offset, length);
+		if (ret < 0)
+			return ret;
+		break;
+	default:
+		break;
+	}
 
-	return zbd_reset_range(td, f, z->start, zbd_zone_end(z) - z->start);
+	pthread_mutex_lock(&f->zbd_info->mutex);
+	f->zbd_info->sectors_with_data -= z->wp - z->start;
+	pthread_mutex_unlock(&f->zbd_info->mutex);
+	z->wp = z->start;
+	z->verify_block = 0;
+
+	td->ts.nr_zone_resets++;
+
+	return ret;
 }
 
 /* The caller must hold f->zbd_info->mutex */
-- 
2.21.0

[PATCH v2 4/4] t/zbd: check log file for failed assertions

[Dmitry Fomichev]

Currently, a ZBD test can succeed even if an fio assertion is raised
during its run. Search every ZBD test log file for failed assertions
and fail the test if any were found.

Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
---
 t/zbd/test-zbd-support | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 471a3487..139495d3 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -77,6 +77,13 @@ check_reset_count() {
     eval "[ '$reset_count' '$1' '$2' ]"
 }
 
+# Check log for failed assertions and crashes. Without these checks,
+# a test can succeed even when these events happen, but it must fail.
+check_log() {
+     [ ! -f "${logfile}.${1}" ] && return 0
+     ! grep -q -e "Assertion " -e "Aborted " "${logfile}.${1}"
+}
+
 # Whether or not $1 (/dev/...) is a SCSI device.
 is_scsi_device() {
     local d f
@@ -1008,7 +1015,7 @@ trap 'intr=1' SIGINT
 for test_number in "${tests[@]}"; do
     rm -f "${logfile}.${test_number}"
     echo -n "Running test $(printf "%02d" $test_number) ... "
-    if eval "test$test_number"; then
+    if eval "test$test_number" && check_log $test_number; then
 	status="PASS"
 	cc_status="${green}${status}${end}"
 	((passed++))
-- 
2.21.0

Re: [PATCH v2 1/4] configure: improve libzbc version check

[Damien Le Moal]

On 2020/07/30 6:04, Dmitry Fomichev wrote:
> Avoid parsing pkg-config output and just use --atleast-version to
> check if libzbc is present and has an up to date version.
> 
> Currently, support for libzbc ioengine is always included if libzbc is
> found at the build system. Add the option to disable libzbc ioengine
> support even if libzbc is found. This can be useful for
> cross-compilation.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  configure | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/configure b/configure
> index 5925e94f..9a5f35f2 100755
> --- a/configure
> +++ b/configure
> @@ -213,6 +213,8 @@ for opt do
>    ;;
>    --enable-libnbd) libnbd="yes"
>    ;;
> +  --disable-libzbc) libzbc="no"
> +  ;;
>    --disable-tcmalloc) disable_tcmalloc="yes"
>    ;;
>    --enable-libaio-uring) libaio_uring="yes"
> @@ -256,6 +258,7 @@ if test "$show_help" = "yes" ; then
>    echo "--with-ime=             Install path for DDN's Infinite Memory Engine"
>    echo "--enable-libiscsi       Enable iscsi support"
>    echo "--enable-libnbd         Enable libnbd (NBD engine) support"
> +  echo "--disable-libzbc        Disable libzbc even if found"

The message could simply be "Disable libzbc support".

>    echo "--disable-tcmalloc	Disable tcmalloc support"
>    echo "--enable-libaio-uring   Enable libaio emulated over io_uring"
>    echo "--dynamic-libengines	Lib-based ioengines as dynamic libraries"
> @@ -2454,9 +2457,6 @@ fi
>  
>  ##########################################
>  # libzbc probe
> -if test "$libzbc" != "yes" ; then
> -  libzbc="no"
> -fi
>  cat > $TMPC << EOF
>  #include <libzbc/zbc.h>
>  int main(int argc, char **argv)
> @@ -2466,19 +2466,21 @@ int main(int argc, char **argv)
>    return zbc_open("foo=bar", O_RDONLY, &dev);
>  }
>  EOF
> -if compile_prog "" "-lzbc" "libzbc"; then
> -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-9]*//')
> -  if test "$libzbcvermaj" -ge "5" ; then
> -    libzbc="yes"
> +if test "$libzbc" != "no" ; then

If you make this:

if test "$libzbc" = "yes" ; then

> +  if compile_prog "" "-lzbc" "libzbc"; then
> +    minimum_libzbc=5
> +    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
> +      libzbc="yes"
> +    else
> +      print_config "libzbc engine" "libzbc version $minimum_libzbc or above required"
> +      libzbc="no"
> +    fi

then you can simplify the above by reversing the if condition,

>    else
> -    print_config "libzbc engine" "Unsupported libzbc version (version 5 or above required)"
> +    if test "$libzbc" = "yes" ; then

and you do not need this if. Note that I think you need to initialize the libzbc
variable to "yes" by default for this to work.

> +      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
> +    fi
>      libzbc="no"
>    fi
> -else
> -  if test "$libzbc" = "yes" ; then
> -      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
> -  fi
> -  libzbc="no"
>  fi
>  print_config "libzbc engine" "$libzbc"
>  
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/4] configure: check if pkg-config is installed

[Damien Le Moal]

On 2020/07/30 6:04, Dmitry Fomichev wrote:
> A few libraries need to be newer than a specific version in order to be
> supported by fio and pkg-config utility is used to verify the versions
> of the installed libraries. Since this step may fail because pkg-config
> is not installed, verify pkg-config presence and warn the user if it
> could not be found.
> 
> To avoid code duplication, add a common helper function to perform
> these checks.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  configure | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/configure b/configure
> index 9a5f35f2..01fa5d1c 100755
> --- a/configure
> +++ b/configure
> @@ -133,6 +133,23 @@ output_sym() {
>    echo "#define $1" >> $config_host_h
>  }
>  
> +check_min_lib_version() {
> +  local feature
> +
> +  if ${cross_prefix}pkg-config --atleast-version=$2 $1 > /dev/null 2>&1; then
> +    return 0
> +  fi
> +  if ${cross_prefix}pkg-config --version > /dev/null 2>&1; then
> +    feature=${3:=$1}
> +    if test ${!feature} = "yes" ; then
> +      feature_not_found "" "$1 >= $2"
> +    fi
> +  else
> +    print_config "$1" "missing pkg-config, can't check library version"
> +  fi
> +  return 1
> +}
> +
>  targetos=""
>  cpu=""
>  
> @@ -1520,18 +1537,17 @@ if test "$?" != "0" ; then
>    echo "configure: gtk and gthread not found"
>    exit 1
>  fi
> -if ! ${cross_prefix}pkg-config --atleast-version 2.18.0 gtk+-2.0; then
> -  echo "GTK found, but need version 2.18 or higher"
> -  gfio="no"
> -else
> +gfio="yes"

I do not think adding this line is necessary. You can just keep the below
gfio="yes" assignment, no ?

> +if check_min_lib_version gtk+-2.0 2.18.0 "gfio"; then
>    if compile_prog "$GTK_CFLAGS" "$GTK_LIBS" "gfio" ; then
> -    gfio="yes"
>      GFIO_LIBS="$LIBS $GTK_LIBS"
>      CFLAGS="$CFLAGS $GTK_CFLAGS"
>    else
>      echo "Please install gtk and gdk libraries"
>      gfio="no"
>    fi
> +else
> +  gfio="no"
>  fi
>  LDFLAGS=$ORG_LDFLAGS
>  fi
> @@ -2181,15 +2197,11 @@ print_config "DDN's Infinite Memory Engine" "$libime"
>  ##########################################
>  # Check if we have libiscsi
>  if test "$libiscsi" != "no" ; then
> -  minimum_libiscsi=1.9.0
> -  if $(pkg-config --atleast-version=$minimum_libiscsi libiscsi); then
> +  if check_min_lib_version libiscsi 1.9.0; then
>      libiscsi="yes"
>      libiscsi_cflags=$(pkg-config --cflags libiscsi)
>      libiscsi_libs=$(pkg-config --libs libiscsi)
>    else
> -    if test "$libiscsi" = "yes" ; then
> -      feature_not_found "libiscsi" "libiscsi >= $minimum_libiscsi"
> -    fi

Why do you remove this ? You only need to remove the "if" but should keep the
feature_not_found call.

>      libiscsi="no"
>    fi
>  fi
> @@ -2198,15 +2210,11 @@ print_config "iscsi engine" "$libiscsi"
>  ##########################################
>  # Check if we have libnbd (for NBD support)
>  if test "$libnbd" != "no" ; then
> -  minimum_libnbd=0.9.8
> -  if $(pkg-config --atleast-version=$minimum_libnbd libnbd); then
> +  if check_min_lib_version libnbd 0.9.8; then
>      libnbd="yes"
>      libnbd_cflags=$(pkg-config --cflags libnbd)
>      libnbd_libs=$(pkg-config --libs libnbd)
>    else
> -    if test "$libnbd" = "yes" ; then
> -      feature_not_found "libnbd" "libnbd >= $minimum_libnbd"
> -    fi

Same here.

>      libnbd="no"
>    fi
>  fi
> @@ -2468,11 +2476,8 @@ int main(int argc, char **argv)
>  EOF
>  if test "$libzbc" != "no" ; then
>    if compile_prog "" "-lzbc" "libzbc"; then
> -    minimum_libzbc=5
> -    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
> -      libzbc="yes"
> -    else
> -      print_config "libzbc engine" "libzbc version $minimum_libzbc or above required"
> +    libzbc="yes"
> +    if ! check_min_lib_version libzbc 5; then

OK. So my comment on the previous patch is addressed here. But I still do not
see an initialization of libzbc to "yes".

>        libzbc="no"
>      fi
>    else
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/4] zbd: simplify zone reset code

[Damien Le Moal]

On 2020/07/30 6:04, Dmitry Fomichev wrote:
> zbd_reset_range() function is only called once from zbd_reset_zone()
> and it is always called for an individual zone, not a range.
> 
> Make zone reset flow simpler by moving all the code needed
> to reset a single zone from zbd_reset_range() to zbd_reset_zone().
> Therefore, zbd_reset_range() is now dropped.
> 
> zbc_reset_zone() is always called with the zone already locked. Remove
> recursive zone locking inside this function and add a note in the
> description of this function saying that the caller is expected to have
> the zone locked when calling it.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  zbd.c | 76 +++++++++++++++++++++--------------------------------------
>  1 file changed, 27 insertions(+), 49 deletions(-)
> 
> diff --git a/zbd.c b/zbd.c
> index 3eac5df3..e4a480b7 100644
> --- a/zbd.c
> +++ b/zbd.c
> @@ -670,54 +670,6 @@ int zbd_setup_files(struct thread_data *td)
>  	return 0;
>  }
>  
> -/**
> - * zbd_reset_range - reset zones for a range of sectors
> - * @td: FIO thread data.
> - * @f: Fio file for which to reset zones
> - * @sector: Starting sector in units of 512 bytes
> - * @nr_sectors: Number of sectors in units of 512 bytes
> - *
> - * Returns 0 upon success and a negative error code upon failure.
> - */
> -static int zbd_reset_range(struct thread_data *td, struct fio_file *f,
> -			   uint64_t offset, uint64_t length)
> -{
> -	uint32_t zone_idx_b, zone_idx_e;
> -	struct fio_zone_info *zb, *ze, *z;
> -	int ret = 0;
> -
> -	assert(is_valid_offset(f, offset + length - 1));
> -
> -	switch (f->zbd_info->model) {
> -	case ZBD_HOST_AWARE:
> -	case ZBD_HOST_MANAGED:
> -		ret = zbd_reset_wp(td, f, offset, length);
> -		if (ret < 0)
> -			return ret;
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	zone_idx_b = zbd_zone_idx(f, offset);
> -	zb = &f->zbd_info->zone_info[zone_idx_b];
> -	zone_idx_e = zbd_zone_idx(f, offset + length);
> -	ze = &f->zbd_info->zone_info[zone_idx_e];
> -	for (z = zb; z < ze; z++) {
> -		pthread_mutex_lock(&z->mutex);
> -		pthread_mutex_lock(&f->zbd_info->mutex);
> -		f->zbd_info->sectors_with_data -= z->wp - z->start;
> -		pthread_mutex_unlock(&f->zbd_info->mutex);
> -		z->wp = z->start;
> -		z->verify_block = 0;
> -		pthread_mutex_unlock(&z->mutex);
> -	}
> -
> -	td->ts.nr_zone_resets += ze - zb;
> -
> -	return ret;
> -}
> -
>  static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
>  				struct fio_zone_info *zone)
>  {
> @@ -731,14 +683,40 @@ static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
>   * @z: Zone to reset.
>   *
>   * Returns 0 upon success and a negative error code upon failure.
> + *
> + * The caller must hold z->mutex.
>   */
>  static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
>  			  struct fio_zone_info *z)
>  {
> +	uint64_t offset = z->start;
> +	uint64_t length = (z+1)->start - offset;
> +	int ret = 0;
> +
> +	assert(is_valid_offset(f, offset + length - 1));
> +
>  	dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
>  		zbd_zone_nr(f->zbd_info, z));
> +	switch (f->zbd_info->model) {
> +	case ZBD_HOST_AWARE:
> +	case ZBD_HOST_MANAGED:
> +		ret = zbd_reset_wp(td, f, offset, length);
> +		if (ret < 0)
> +			return ret;
> +		break;
> +	default:
> +		break;
> +	}
>  
> -	return zbd_reset_range(td, f, z->start, zbd_zone_end(z) - z->start);
> +	pthread_mutex_lock(&f->zbd_info->mutex);
> +	f->zbd_info->sectors_with_data -= z->wp - z->start;
> +	pthread_mutex_unlock(&f->zbd_info->mutex);
> +	z->wp = z->start;
> +	z->verify_block = 0;
> +
> +	td->ts.nr_zone_resets++;
> +
> +	return ret;
>  }
>  
>  /* The caller must hold f->zbd_info->mutex */
> 

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 4/4] t/zbd: check log file for failed assertions

[Damien Le Moal]

On 2020/07/30 6:04, Dmitry Fomichev wrote:
> Currently, a ZBD test can succeed even if an fio assertion is raised
> during its run. Search every ZBD test log file for failed assertions
> and fail the test if any were found.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  t/zbd/test-zbd-support | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
> index 471a3487..139495d3 100755
> --- a/t/zbd/test-zbd-support
> +++ b/t/zbd/test-zbd-support
> @@ -77,6 +77,13 @@ check_reset_count() {
>      eval "[ '$reset_count' '$1' '$2' ]"
>  }
>  
> +# Check log for failed assertions and crashes. Without these checks,
> +# a test can succeed even when these events happen, but it must fail.
> +check_log() {
> +     [ ! -f "${logfile}.${1}" ] && return 0
> +     ! grep -q -e "Assertion " -e "Aborted " "${logfile}.${1}"
> +}
> +
>  # Whether or not $1 (/dev/...) is a SCSI device.
>  is_scsi_device() {
>      local d f
> @@ -1008,7 +1015,7 @@ trap 'intr=1' SIGINT
>  for test_number in "${tests[@]}"; do
>      rm -f "${logfile}.${test_number}"
>      echo -n "Running test $(printf "%02d" $test_number) ... "
> -    if eval "test$test_number"; then
> +    if eval "test$test_number" && check_log $test_number; then
>  	status="PASS"
>  	cc_status="${green}${status}${end}"
>  	((passed++))
> 

Looks good to me.

Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>

-- 
Damien Le Moal
Western Digital Research

RE: [PATCH v2 1/4] configure: improve libzbc version check

[Dmitry Fomichev]

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: Thursday, July 30, 2020 12:54 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> <axboe@kernel.dk>
> Cc: fio@vger.kernel.org; Shinichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH v2 1/4] configure: improve libzbc version check
> 
> On 2020/07/30 6:04, Dmitry Fomichev wrote:
> > Avoid parsing pkg-config output and just use --atleast-version to
> > check if libzbc is present and has an up to date version.
> >
> > Currently, support for libzbc ioengine is always included if libzbc is
> > found at the build system. Add the option to disable libzbc ioengine
> > support even if libzbc is found. This can be useful for
> > cross-compilation.
> >
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  configure | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 5925e94f..9a5f35f2 100755
> > --- a/configure
> > +++ b/configure
> > @@ -213,6 +213,8 @@ for opt do
> >    ;;
> >    --enable-libnbd) libnbd="yes"
> >    ;;
> > +  --disable-libzbc) libzbc="no"
> > +  ;;
> >    --disable-tcmalloc) disable_tcmalloc="yes"
> >    ;;
> >    --enable-libaio-uring) libaio_uring="yes"
> > @@ -256,6 +258,7 @@ if test "$show_help" = "yes" ; then
> >    echo "--with-ime=             Install path for DDN's Infinite Memory Engine"
> >    echo "--enable-libiscsi       Enable iscsi support"
> >    echo "--enable-libnbd         Enable libnbd (NBD engine) support"
> > +  echo "--disable-libzbc        Disable libzbc even if found"
> 
> The message could simply be "Disable libzbc support".
> 
> >    echo "--disable-tcmalloc	Disable tcmalloc support"
> >    echo "--enable-libaio-uring   Enable libaio emulated over io_uring"
> >    echo "--dynamic-libengines	Lib-based ioengines as dynamic libraries"
> > @@ -2454,9 +2457,6 @@ fi
> >
> >  ##########################################
> >  # libzbc probe
> > -if test "$libzbc" != "yes" ; then
> > -  libzbc="no"
> > -fi
> >  cat > $TMPC << EOF
> >  #include <libzbc/zbc.h>
> >  int main(int argc, char **argv)
> > @@ -2466,19 +2466,21 @@ int main(int argc, char **argv)
> >    return zbc_open("foo=bar", O_RDONLY, &dev);
> >  }
> >  EOF
> > -if compile_prog "" "-lzbc" "libzbc"; then
> > -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-
> 9]*//')
> > -  if test "$libzbcvermaj" -ge "5" ; then
> > -    libzbc="yes"
> > +if test "$libzbc" != "no" ; then
> 
> If you make this:
> 
> if test "$libzbc" = "yes" ; then
> 
> > +  if compile_prog "" "-lzbc" "libzbc"; then
> > +    minimum_libzbc=5
> > +    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
> > +      libzbc="yes"
> > +    else
> > +      print_config "libzbc engine" "libzbc version $minimum_libzbc or above
> required"
> > +      libzbc="no"
> > +    fi
> 
> then you can simplify the above by reversing the if condition,
> 
> >    else
> > -    print_config "libzbc engine" "Unsupported libzbc version (version 5 or
> above required)"
> > +    if test "$libzbc" = "yes" ; then
> 
> and you do not need this if. Note that I think you need to initialize the libzbc
> variable to "yes" by default for this to work.
> 

Right now, there is no default for libzbc ioengine support. If libzbc is found by
configure, then libzbc ioengine will always be added. The new flag, --disable-libzbc,
prevents adding support for libzbc ioengine in this case. If libzbc is not present,
fio configure is not failed with "feature not found".

Currently, no other feature in fio configure has the default option of "yes" and all
features that can be explicitly set to yes are set so by script command line options.
If a feature is set to yes and is not available, configure fails with "feature not found"
error. Do we really want configure to fail on any system that doesn't have libzbc
unless --disable-libzbc is added to the command line?

There is one feature option, "disable_lex",  with the default value, "". I guess we
could define libzbc option the same way to improve code clarity.

> > +      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
> > +    fi
> >      libzbc="no"
> >    fi
> > -else
> > -  if test "$libzbc" = "yes" ; then
> > -      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
> > -  fi
> > -  libzbc="no"
> >  fi
> >  print_config "libzbc engine" "$libzbc"
> >
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research

RE: [PATCH v2 2/4] configure: check if pkg-config is installed

[Dmitry Fomichev]

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: Thursday, July 30, 2020 1:03 AM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> <axboe@kernel.dk>
> Cc: fio@vger.kernel.org; Shinichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH v2 2/4] configure: check if pkg-config is installed
> 
> On 2020/07/30 6:04, Dmitry Fomichev wrote:
> > A few libraries need to be newer than a specific version in order to be
> > supported by fio and pkg-config utility is used to verify the versions
> > of the installed libraries. Since this step may fail because pkg-config
> > is not installed, verify pkg-config presence and warn the user if it
> > could not be found.
> >
> > To avoid code duplication, add a common helper function to perform
> > these checks.
> >
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  configure | 45 +++++++++++++++++++++++++--------------------
> >  1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 9a5f35f2..01fa5d1c 100755
> > --- a/configure
> > +++ b/configure
> > @@ -133,6 +133,23 @@ output_sym() {
> >    echo "#define $1" >> $config_host_h
> >  }
> >
> > +check_min_lib_version() {
> > +  local feature
> > +
> > +  if ${cross_prefix}pkg-config --atleast-version=$2 $1 > /dev/null 2>&1;
> then
> > +    return 0
> > +  fi
> > +  if ${cross_prefix}pkg-config --version > /dev/null 2>&1; then
> > +    feature=${3:=$1}
> > +    if test ${!feature} = "yes" ; then
> > +      feature_not_found "" "$1 >= $2"
> > +    fi
> > +  else
> > +    print_config "$1" "missing pkg-config, can't check library version"
> > +  fi
> > +  return 1
> > +}
> > +
> >  targetos=""
> >  cpu=""
> >
> > @@ -1520,18 +1537,17 @@ if test "$?" != "0" ; then
> >    echo "configure: gtk and gthread not found"
> >    exit 1
> >  fi
> > -if ! ${cross_prefix}pkg-config --atleast-version 2.18.0 gtk+-2.0; then
> > -  echo "GTK found, but need version 2.18 or higher"
> > -  gfio="no"
> > -else
> > +gfio="yes"
> 
> I do not think adding this line is necessary. You can just keep the below
> gfio="yes" assignment, no ?
> 
> > +if check_min_lib_version gtk+-2.0 2.18.0 "gfio"; then
> >    if compile_prog "$GTK_CFLAGS" "$GTK_LIBS" "gfio" ; then
> > -    gfio="yes"
> >      GFIO_LIBS="$LIBS $GTK_LIBS"
> >      CFLAGS="$CFLAGS $GTK_CFLAGS"
> >    else
> >      echo "Please install gtk and gdk libraries"
> >      gfio="no"
> >    fi
> > +else
> > +  gfio="no"
> >  fi
> >  LDFLAGS=$ORG_LDFLAGS
> >  fi
> > @@ -2181,15 +2197,11 @@ print_config "DDN's Infinite Memory Engine"
> "$libime"
> >  ##########################################
> >  # Check if we have libiscsi
> >  if test "$libiscsi" != "no" ; then
> > -  minimum_libiscsi=1.9.0
> > -  if $(pkg-config --atleast-version=$minimum_libiscsi libiscsi); then
> > +  if check_min_lib_version libiscsi 1.9.0; then
> >      libiscsi="yes"
> >      libiscsi_cflags=$(pkg-config --cflags libiscsi)
> >      libiscsi_libs=$(pkg-config --libs libiscsi)
> >    else
> > -    if test "$libiscsi" = "yes" ; then
> > -      feature_not_found "libiscsi" "libiscsi >= $minimum_libiscsi"
> > -    fi
> 
> Why do you remove this ? You only need to remove the "if" but should keep
> the
> feature_not_found call.

The feature_not_found call is now part of check_min_lib_version() function code.
I think it is the right thing to do to avoid code duplication. Also, this unifies the
script behavior when an out of date library is found - the message in libzbc case
will be similar to libiscsi and libnbd.

> 
> >      libiscsi="no"
> >    fi
> >  fi
> > @@ -2198,15 +2210,11 @@ print_config "iscsi engine" "$libiscsi"
> >  ##########################################
> >  # Check if we have libnbd (for NBD support)
> >  if test "$libnbd" != "no" ; then
> > -  minimum_libnbd=0.9.8
> > -  if $(pkg-config --atleast-version=$minimum_libnbd libnbd); then
> > +  if check_min_lib_version libnbd 0.9.8; then
> >      libnbd="yes"
> >      libnbd_cflags=$(pkg-config --cflags libnbd)
> >      libnbd_libs=$(pkg-config --libs libnbd)
> >    else
> > -    if test "$libnbd" = "yes" ; then
> > -      feature_not_found "libnbd" "libnbd >= $minimum_libnbd"
> > -    fi
> 
> Same here.

I need to mention that this patch has been tested against all the four libraries affected.

> 
> >      libnbd="no"
> >    fi
> >  fi
> > @@ -2468,11 +2476,8 @@ int main(int argc, char **argv)
> >  EOF
> >  if test "$libzbc" != "no" ; then
> >    if compile_prog "" "-lzbc" "libzbc"; then
> > -    minimum_libzbc=5
> > -    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
> > -      libzbc="yes"
> > -    else
> > -      print_config "libzbc engine" "libzbc version $minimum_libzbc or above
> required"
> > +    libzbc="yes"
> > +    if ! check_min_lib_version libzbc 5; then
> 
> OK. So my comment on the previous patch is addressed here. But I still do
> not
> see an initialization of libzbc to "yes"

Replied in the email for the other patch, the way it works now might actually be ok...

> 
> >        libzbc="no"
> >      fi
> >    else
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH v2 1/4] configure: improve libzbc version check

[Sitsofe Wheeler]

On Fri, 31 Jul 2020 at 00:09, Dmitry Fomichev <Dmitry.Fomichev@wdc.com> wrote:
>
> Right now, there is no default for libzbc ioengine support. If libzbc is found by
> configure, then libzbc ioengine will always be added. The new flag, --disable-libzbc,
> prevents adding support for libzbc ioengine in this case. If libzbc is not present,
> fio configure is not failed with "feature not found".
>
> Currently, no other feature in fio configure has the default option of "yes" and all
> features that can be explicitly set to yes are set so by script command line options.
> If a feature is set to yes and is not available, configure fails with "feature not found"
> error. Do we really want configure to fail on any system that doesn't have libzbc
> unless --disable-libzbc is added to the command line?
>
> There is one feature option, "disable_lex",  with the default value, "". I guess we
> could define libzbc option the same way to improve code clarity.

That's right:
- "yes": hard fail if probe fails
- "": probe but don't hard fail
- "no": don't probe and disable feature

So your suggestion of a default of "" is right because I can't believe
we want to hard fail when libzbc is unavailable.

I learned of this pattern while reading
https://github.com/qemu/qemu/blob/1448629751871c4924c234c2faaa968fc26890e1/configure#L348
which I think was the basis of fio's configure script. The desire to
show this pattern led to some of the refactoring in
https://github.com/axboe/fio/commit/d490af7f8c0ee7a992f332bb6bcffb30313d6488
even though it added some unncessary lines for those options...

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH v2 1/4] configure: improve libzbc version check

[Damien Le Moal]

On 2020/07/31 8:09, Dmitry Fomichev wrote:
> 
> 
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: Thursday, July 30, 2020 12:54 AM
>> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
>> <axboe@kernel.dk>
>> Cc: fio@vger.kernel.org; Shinichiro Kawasaki
>> <shinichiro.kawasaki@wdc.com>
>> Subject: Re: [PATCH v2 1/4] configure: improve libzbc version check
>>
>> On 2020/07/30 6:04, Dmitry Fomichev wrote:
>>> Avoid parsing pkg-config output and just use --atleast-version to
>>> check if libzbc is present and has an up to date version.
>>>
>>> Currently, support for libzbc ioengine is always included if libzbc is
>>> found at the build system. Add the option to disable libzbc ioengine
>>> support even if libzbc is found. This can be useful for
>>> cross-compilation.
>>>
>>> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>> ---
>>>  configure | 28 +++++++++++++++-------------
>>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 5925e94f..9a5f35f2 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -213,6 +213,8 @@ for opt do
>>>    ;;
>>>    --enable-libnbd) libnbd="yes"
>>>    ;;
>>> +  --disable-libzbc) libzbc="no"
>>> +  ;;
>>>    --disable-tcmalloc) disable_tcmalloc="yes"
>>>    ;;
>>>    --enable-libaio-uring) libaio_uring="yes"
>>> @@ -256,6 +258,7 @@ if test "$show_help" = "yes" ; then
>>>    echo "--with-ime=             Install path for DDN's Infinite Memory Engine"
>>>    echo "--enable-libiscsi       Enable iscsi support"
>>>    echo "--enable-libnbd         Enable libnbd (NBD engine) support"
>>> +  echo "--disable-libzbc        Disable libzbc even if found"
>>
>> The message could simply be "Disable libzbc support".
>>
>>>    echo "--disable-tcmalloc	Disable tcmalloc support"
>>>    echo "--enable-libaio-uring   Enable libaio emulated over io_uring"
>>>    echo "--dynamic-libengines	Lib-based ioengines as dynamic libraries"
>>> @@ -2454,9 +2457,6 @@ fi
>>>
>>>  ##########################################
>>>  # libzbc probe
>>> -if test "$libzbc" != "yes" ; then
>>> -  libzbc="no"
>>> -fi
>>>  cat > $TMPC << EOF
>>>  #include <libzbc/zbc.h>
>>>  int main(int argc, char **argv)
>>> @@ -2466,19 +2466,21 @@ int main(int argc, char **argv)
>>>    return zbc_open("foo=bar", O_RDONLY, &dev);
>>>  }
>>>  EOF
>>> -if compile_prog "" "-lzbc" "libzbc"; then
>>> -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-
>> 9]*//')
>>> -  if test "$libzbcvermaj" -ge "5" ; then
>>> -    libzbc="yes"
>>> +if test "$libzbc" != "no" ; then
>>
>> If you make this:
>>
>> if test "$libzbc" = "yes" ; then
>>
>>> +  if compile_prog "" "-lzbc" "libzbc"; then
>>> +    minimum_libzbc=5
>>> +    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
>>> +      libzbc="yes"
>>> +    else
>>> +      print_config "libzbc engine" "libzbc version $minimum_libzbc or above
>> required"
>>> +      libzbc="no"
>>> +    fi
>>
>> then you can simplify the above by reversing the if condition,
>>
>>>    else
>>> -    print_config "libzbc engine" "Unsupported libzbc version (version 5 or
>> above required)"
>>> +    if test "$libzbc" = "yes" ; then
>>
>> and you do not need this if. Note that I think you need to initialize the libzbc
>> variable to "yes" by default for this to work.
>>
> 
> Right now, there is no default for libzbc ioengine support. If libzbc is found by
> configure, then libzbc ioengine will always be added. The new flag, --disable-libzbc,
> prevents adding support for libzbc ioengine in this case. If libzbc is not present,
> fio configure is not failed with "feature not found".
> 
> Currently, no other feature in fio configure has the default option of "yes" and all
> features that can be explicitly set to yes are set so by script command line options.
> If a feature is set to yes and is not available, configure fails with "feature not found"
> error. Do we really want configure to fail on any system that doesn't have libzbc
> unless --disable-libzbc is added to the command line?
> 
> There is one feature option, "disable_lex",  with the default value, "". I guess we
> could define libzbc option the same way to improve code clarity.

Yes. That matches what Sitsofe commented too. That would mean, enable is found
if --disable-libzbc is not set, but do not fail configure if not found.

> 
>>> +      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
>>> +    fi
>>>      libzbc="no"
>>>    fi
>>> -else
>>> -  if test "$libzbc" = "yes" ; then
>>> -      feature_not_found "libzbc" "libzbc or libzbc/zbc.h"
>>> -  fi
>>> -  libzbc="no"
>>>  fi
>>>  print_config "libzbc engine" "$libzbc"
>>>
>>>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 2/4] configure: check if pkg-config is installed

[Damien Le Moal]

On 2020/07/31 8:10, Dmitry Fomichev wrote:
> 
> 
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: Thursday, July 30, 2020 1:03 AM
>> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
>> <axboe@kernel.dk>
>> Cc: fio@vger.kernel.org; Shinichiro Kawasaki
>> <shinichiro.kawasaki@wdc.com>
>> Subject: Re: [PATCH v2 2/4] configure: check if pkg-config is installed
>>
>> On 2020/07/30 6:04, Dmitry Fomichev wrote:
>>> A few libraries need to be newer than a specific version in order to be
>>> supported by fio and pkg-config utility is used to verify the versions
>>> of the installed libraries. Since this step may fail because pkg-config
>>> is not installed, verify pkg-config presence and warn the user if it
>>> could not be found.
>>>
>>> To avoid code duplication, add a common helper function to perform
>>> these checks.
>>>
>>> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
>>> ---
>>>  configure | 45 +++++++++++++++++++++++++--------------------
>>>  1 file changed, 25 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/configure b/configure
>>> index 9a5f35f2..01fa5d1c 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -133,6 +133,23 @@ output_sym() {
>>>    echo "#define $1" >> $config_host_h
>>>  }
>>>
>>> +check_min_lib_version() {
>>> +  local feature
>>> +
>>> +  if ${cross_prefix}pkg-config --atleast-version=$2 $1 > /dev/null 2>&1;
>> then
>>> +    return 0
>>> +  fi
>>> +  if ${cross_prefix}pkg-config --version > /dev/null 2>&1; then
>>> +    feature=${3:=$1}
>>> +    if test ${!feature} = "yes" ; then
>>> +      feature_not_found "" "$1 >= $2"
>>> +    fi
>>> +  else
>>> +    print_config "$1" "missing pkg-config, can't check library version"
>>> +  fi
>>> +  return 1
>>> +}
>>> +
>>>  targetos=""
>>>  cpu=""
>>>
>>> @@ -1520,18 +1537,17 @@ if test "$?" != "0" ; then
>>>    echo "configure: gtk and gthread not found"
>>>    exit 1
>>>  fi
>>> -if ! ${cross_prefix}pkg-config --atleast-version 2.18.0 gtk+-2.0; then
>>> -  echo "GTK found, but need version 2.18 or higher"
>>> -  gfio="no"
>>> -else
>>> +gfio="yes"
>>
>> I do not think adding this line is necessary. You can just keep the below
>> gfio="yes" assignment, no ?
>>
>>> +if check_min_lib_version gtk+-2.0 2.18.0 "gfio"; then
>>>    if compile_prog "$GTK_CFLAGS" "$GTK_LIBS" "gfio" ; then
>>> -    gfio="yes"
>>>      GFIO_LIBS="$LIBS $GTK_LIBS"
>>>      CFLAGS="$CFLAGS $GTK_CFLAGS"
>>>    else
>>>      echo "Please install gtk and gdk libraries"
>>>      gfio="no"
>>>    fi
>>> +else
>>> +  gfio="no"
>>>  fi
>>>  LDFLAGS=$ORG_LDFLAGS
>>>  fi
>>> @@ -2181,15 +2197,11 @@ print_config "DDN's Infinite Memory Engine"
>> "$libime"
>>>  ##########################################
>>>  # Check if we have libiscsi
>>>  if test "$libiscsi" != "no" ; then
>>> -  minimum_libiscsi=1.9.0
>>> -  if $(pkg-config --atleast-version=$minimum_libiscsi libiscsi); then
>>> +  if check_min_lib_version libiscsi 1.9.0; then
>>>      libiscsi="yes"
>>>      libiscsi_cflags=$(pkg-config --cflags libiscsi)
>>>      libiscsi_libs=$(pkg-config --libs libiscsi)
>>>    else
>>> -    if test "$libiscsi" = "yes" ; then
>>> -      feature_not_found "libiscsi" "libiscsi >= $minimum_libiscsi"
>>> -    fi
>>
>> Why do you remove this ? You only need to remove the "if" but should keep
>> the
>> feature_not_found call.
> 
> The feature_not_found call is now part of check_min_lib_version() function code.
> I think it is the right thing to do to avoid code duplication. Also, this unifies the
> script behavior when an out of date library is found - the message in libzbc case
> will be similar to libiscsi and libnbd.

Missed that. You are right.

> 
>>
>>>      libiscsi="no"
>>>    fi
>>>  fi
>>> @@ -2198,15 +2210,11 @@ print_config "iscsi engine" "$libiscsi"
>>>  ##########################################
>>>  # Check if we have libnbd (for NBD support)
>>>  if test "$libnbd" != "no" ; then
>>> -  minimum_libnbd=0.9.8
>>> -  if $(pkg-config --atleast-version=$minimum_libnbd libnbd); then
>>> +  if check_min_lib_version libnbd 0.9.8; then
>>>      libnbd="yes"
>>>      libnbd_cflags=$(pkg-config --cflags libnbd)
>>>      libnbd_libs=$(pkg-config --libs libnbd)
>>>    else
>>> -    if test "$libnbd" = "yes" ; then
>>> -      feature_not_found "libnbd" "libnbd >= $minimum_libnbd"
>>> -    fi
>>
>> Same here.
> 
> I need to mention that this patch has been tested against all the four libraries affected.
> 
>>
>>>      libnbd="no"
>>>    fi
>>>  fi
>>> @@ -2468,11 +2476,8 @@ int main(int argc, char **argv)
>>>  EOF
>>>  if test "$libzbc" != "no" ; then
>>>    if compile_prog "" "-lzbc" "libzbc"; then
>>> -    minimum_libzbc=5
>>> -    if $(pkg-config --atleast-version=$minimum_libzbc libzbc); then
>>> -      libzbc="yes"
>>> -    else
>>> -      print_config "libzbc engine" "libzbc version $minimum_libzbc or above
>> required"
>>> +    libzbc="yes"
>>> +    if ! check_min_lib_version libzbc 5; then
>>
>> OK. So my comment on the previous patch is addressed here. But I still do
>> not
>> see an initialization of libzbc to "yes"
> 
> Replied in the email for the other patch, the way it works now might actually be ok...

Yes. Got it. But it may still be good to add the initialization libzbc="".

> 
>>
>>>        libzbc="no"
>>>      fi
>>>    else
>>>
>>
>>
>> --
>> Damien Le Moal
>> Western Digital Research
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/4] configure: improve libzbc version check

[Damien Le Moal]

On 2020/07/31 14:54, Sitsofe Wheeler wrote:
> On Fri, 31 Jul 2020 at 00:09, Dmitry Fomichev <Dmitry.Fomichev@wdc.com> wrote:
>>
>> Right now, there is no default for libzbc ioengine support. If libzbc is found by
>> configure, then libzbc ioengine will always be added. The new flag, --disable-libzbc,
>> prevents adding support for libzbc ioengine in this case. If libzbc is not present,
>> fio configure is not failed with "feature not found".
>>
>> Currently, no other feature in fio configure has the default option of "yes" and all
>> features that can be explicitly set to yes are set so by script command line options.
>> If a feature is set to yes and is not available, configure fails with "feature not found"
>> error. Do we really want configure to fail on any system that doesn't have libzbc
>> unless --disable-libzbc is added to the command line?
>>
>> There is one feature option, "disable_lex",  with the default value, "". I guess we
>> could define libzbc option the same way to improve code clarity.
> 
> That's right:
> - "yes": hard fail if probe fails
> - "": probe but don't hard fail
> - "no": don't probe and disable feature
> 
> So your suggestion of a default of "" is right because I can't believe
> we want to hard fail when libzbc is unavailable.
> 
> I learned of this pattern while reading
> https://github.com/qemu/qemu/blob/1448629751871c4924c234c2faaa968fc26890e1/configure#L348
> which I think was the basis of fio's configure script. The desire to
> show this pattern led to some of the refactoring in
> https://github.com/axboe/fio/commit/d490af7f8c0ee7a992f332bb6bcffb30313d6488
> even though it added some unncessary lines for those options...
> 

Thanks. That indeed clarifies everything !

-- 
Damien Le Moal
Western Digital Research