[PATCH 0/3] miscellaneous ZBD improvements

[PATCH 0/3] miscellaneous ZBD improvements

[Dmitry Fomichev]

This series contains three patches related to zoned block devices.

Dmitry Fomichev (3):
  configure: improve libzbc version check
  zbd: simplify zone reset code
  t/zbd: check log file for failed assertions

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

-- 
2.21.0

[PATCH 1/3] 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. Since
this process may fail because pkg-config is not installed, verify
pkg-config presence and warn the user if it was not found.

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

diff --git a/configure b/configure
index 5925e94f..0c4a96ec 100755
--- a/configure
+++ b/configure
@@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
 }
 EOF
 if compile_prog "" "-lzbc" "libzbc"; then
-  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-9]*//')
-  if test "$libzbcvermaj" -ge "5" ; then
+  if $(pkg-config --atleast-version=5 libzbc); then
     libzbc="yes"
+    LIBS="-lzbc $LIBS"
+  elif pkg-config --version > /dev/null 2>&1; then
+    print_config "libzbc engine" "libzbc version 5 or above required"
+    libzbc="no"
   else
-    print_config "libzbc engine" "Unsupported libzbc version (version 5 or above required)"
+    print_config "libzbc engine" "missing pkg-config, can't check library version"
     libzbc="no"
   fi
 else
-- 
2.21.0

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

[Damien Le Moal]

On 2020/07/27 12:16, 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. Since
> this process may fail because pkg-config is not installed, verify
> pkg-config presence and warn the user if it was not found.
> 
> Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> ---
>  configure | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 5925e94f..0c4a96ec 100755
> --- a/configure
> +++ b/configure
> @@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
>  }
>  EOF
>  if compile_prog "" "-lzbc" "libzbc"; then
> -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-9]*//')
> -  if test "$libzbcvermaj" -ge "5" ; then
> +  if $(pkg-config --atleast-version=5 libzbc); then
>      libzbc="yes"
> +    LIBS="-lzbc $LIBS"
> +  elif pkg-config --version > /dev/null 2>&1; then
> +    print_config "libzbc engine" "libzbc version 5 or above required"
> +    libzbc="no"
>    else
> -    print_config "libzbc engine" "Unsupported libzbc version (version 5 or above required)"
> +    print_config "libzbc engine" "missing pkg-config, can't check library version"
>      libzbc="no"
>    fi
>  else
> 

Looks good.

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

-- 
Damien Le Moal
Western Digital Research

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

[Sitsofe Wheeler]

On Mon, 27 Jul 2020 at 06:05, Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>
> On 2020/07/27 12:16, 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. Since
> > this process may fail because pkg-config is not installed, verify
> > pkg-config presence and warn the user if it was not found.
> >
> > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > ---
> >  configure | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/configure b/configure
> > index 5925e94f..0c4a96ec 100755
> > --- a/configure
> > +++ b/configure
> > @@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
> >  }
> >  EOF
> >  if compile_prog "" "-lzbc" "libzbc"; then
> > -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-9]*//')
> > -  if test "$libzbcvermaj" -ge "5" ; then
> > +  if $(pkg-config --atleast-version=5 libzbc); then

Should this really be being done in a subshell? If it weren't done in
a subshell wouldn't the if have failed due to a lack of pkg-config:

% if not-pkg-config --atleast-version=5 libzbc; then echo y; else echo n; fi
zsh: command not found: not-pkg-config
n

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

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

[Dmitry Fomichev]

> -----Original Message-----
> From: Sitsofe Wheeler <sitsofe@gmail.com>
> Sent: Monday, July 27, 2020 1:23 AM
> To: Damien Le Moal <Damien.LeMoal@wdc.com>
> Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> <axboe@kernel.dk>; fio@vger.kernel.org; Shinichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH 1/3] configure: improve libzbc version check
> 
> On Mon, 27 Jul 2020 at 06:05, Damien Le Moal <Damien.LeMoal@wdc.com>
> wrote:
> >
> > On 2020/07/27 12:16, 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. Since
> > > this process may fail because pkg-config is not installed, verify
> > > pkg-config presence and warn the user if it was not found.
> > >
> > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > ---
> > >  configure | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/configure b/configure
> > > index 5925e94f..0c4a96ec 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
> > >  }
> > >  EOF
> > >  if compile_prog "" "-lzbc" "libzbc"; then
> > > -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-
> 9]*//')
> > > -  if test "$libzbcvermaj" -ge "5" ; then
> > > +  if $(pkg-config --atleast-version=5 libzbc); then
> 
> Should this really be being done in a subshell? If it weren't done in
> a subshell wouldn't the if have failed due to a lack of pkg-config:
> 
> % if not-pkg-config --atleast-version=5 libzbc; then echo y; else echo n; fi
> zsh: command not found: not-pkg-config
> n

This line now does the same thing that is done to check libiscsi and libnbd
minimum versions in the same script. It works fine if pkg-config is installed
in the system. The version without the subshell also works, but it outputs
an additional error message if pkg-config is not installed -

./configure: line 2470: not-pkg-config: command not found
libzbc engine                 missing pkg-config, can't check library version

I could remove the explicit check for pkg-config and do the version check
without the subshell, but I think the output is less confusing with the subshell
version.

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

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

[Sitsofe Wheeler]

On Tue, 28 Jul 2020 at 20:58, Dmitry Fomichev <Dmitry.Fomichev@wdc.com> wrote:
>
> > -----Original Message-----
> > From: Sitsofe Wheeler <sitsofe@gmail.com>
> > Sent: Monday, July 27, 2020 1:23 AM
> > To: Damien Le Moal <Damien.LeMoal@wdc.com>
> > Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> > <axboe@kernel.dk>; fio@vger.kernel.org; Shinichiro Kawasaki
> > <shinichiro.kawasaki@wdc.com>
> > Subject: Re: [PATCH 1/3] configure: improve libzbc version check
> >
> > On Mon, 27 Jul 2020 at 06:05, Damien Le Moal <Damien.LeMoal@wdc.com>
> > wrote:
> > >
> > > On 2020/07/27 12:16, 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. Since
> > > > this process may fail because pkg-config is not installed, verify
> > > > pkg-config presence and warn the user if it was not found.
> > > >
> > > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > > ---
> > > >  configure | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/configure b/configure
> > > > index 5925e94f..0c4a96ec 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
> > > >  }
> > > >  EOF
> > > >  if compile_prog "" "-lzbc" "libzbc"; then
> > > > -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-
> > 9]*//')
> > > > -  if test "$libzbcvermaj" -ge "5" ; then
> > > > +  if $(pkg-config --atleast-version=5 libzbc); then
> >
> > Should this really be being done in a subshell? If it weren't done in
> > a subshell wouldn't the if have failed due to a lack of pkg-config:
> >
> > % if not-pkg-config --atleast-version=5 libzbc; then echo y; else echo n; fi
> > zsh: command not found: not-pkg-config
> > n
>
> This line now does the same thing that is done to check libiscsi and libnbd
> minimum versions in the same script. It works fine if pkg-config is installed
> in the system. The version without the subshell also works, but it outputs
> an additional error message if pkg-config is not installed -
>
> ./configure: line 2470: not-pkg-config: command not found
> libzbc engine                 missing pkg-config, can't check library version
>
> I could remove the explicit check for pkg-config and do the version check
> without the subshell, but I think the output is less confusing with the subshell
> version.

OK I see what you mean. Would it be better to have an explicit
pkg-config check for all those engines?

(There is also a general issue that fio's pkg-config usage won't cope
well in cross compile environments but that's another issue...)

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

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

[Dmitry Fomichev]

> -----Original Message-----
> From: Sitsofe Wheeler <sitsofe@gmail.com>
> Sent: Tuesday, July 28, 2020 5:23 PM
> To: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> Cc: Damien Le Moal <Damien.LeMoal@wdc.com>; Jens Axboe
> <axboe@kernel.dk>; fio@vger.kernel.org; Shinichiro Kawasaki
> <shinichiro.kawasaki@wdc.com>
> Subject: Re: [PATCH 1/3] configure: improve libzbc version check
> 
> On Tue, 28 Jul 2020 at 20:58, Dmitry Fomichev <Dmitry.Fomichev@wdc.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Sitsofe Wheeler <sitsofe@gmail.com>
> > > Sent: Monday, July 27, 2020 1:23 AM
> > > To: Damien Le Moal <Damien.LeMoal@wdc.com>
> > > Cc: Dmitry Fomichev <Dmitry.Fomichev@wdc.com>; Jens Axboe
> > > <axboe@kernel.dk>; fio@vger.kernel.org; Shinichiro Kawasaki
> > > <shinichiro.kawasaki@wdc.com>
> > > Subject: Re: [PATCH 1/3] configure: improve libzbc version check
> > >
> > > On Mon, 27 Jul 2020 at 06:05, Damien Le Moal
> <Damien.LeMoal@wdc.com>
> > > wrote:
> > > >
> > > > On 2020/07/27 12:16, 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. Since
> > > > > this process may fail because pkg-config is not installed, verify
> > > > > pkg-config presence and warn the user if it was not found.
> > > > >
> > > > > Signed-off-by: Dmitry Fomichev <dmitry.fomichev@wdc.com>
> > > > > ---
> > > > >  configure | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/configure b/configure
> > > > > index 5925e94f..0c4a96ec 100755
> > > > > --- a/configure
> > > > > +++ b/configure
> > > > > @@ -2467,11 +2467,14 @@ int main(int argc, char **argv)
> > > > >  }
> > > > >  EOF
> > > > >  if compile_prog "" "-lzbc" "libzbc"; then
> > > > > -  libzbcvermaj=$(pkg-config --modversion libzbc | sed 's/\.[0-9]*\.[0-
> > > 9]*//')
> > > > > -  if test "$libzbcvermaj" -ge "5" ; then
> > > > > +  if $(pkg-config --atleast-version=5 libzbc); then
> > >
> > > Should this really be being done in a subshell? If it weren't done in
> > > a subshell wouldn't the if have failed due to a lack of pkg-config:
> > >
> > > % if not-pkg-config --atleast-version=5 libzbc; then echo y; else echo n; fi
> > > zsh: command not found: not-pkg-config
> > > n
> >
> > This line now does the same thing that is done to check libiscsi and libnbd
> > minimum versions in the same script. It works fine if pkg-config is installed
> > in the system. The version without the subshell also works, but it outputs
> > an additional error message if pkg-config is not installed -
> >
> > ./configure: line 2470: not-pkg-config: command not found
> > libzbc engine                 missing pkg-config, can't check library version
> >
> > I could remove the explicit check for pkg-config and do the version check
> > without the subshell, but I think the output is less confusing with the
> subshell
> > version.
> 
> OK I see what you mean. Would it be better to have an explicit
> pkg-config check for all those engines?
> 
> (There is also a general issue that fio's pkg-config usage won't cope
> well in cross compile environments but that's another issue...)

I've sent v2 that adds a helper function to perform the same check for all
other libs that need it.

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

[PATCH 2/3] 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.

No functional change.

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..f6ccf299 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)
 {
@@ -735,10 +687,36 @@ static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
 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(&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++;
+
+	return ret;
 }
 
 /* The caller must hold f->zbd_info->mutex */
-- 
2.21.0

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

[Damien Le Moal]

On 2020/07/27 12:16, 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.
> 
> No functional change.
> 
> 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..f6ccf299 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)
>  {
> @@ -735,10 +687,36 @@ static unsigned int zbd_zone_nr(struct zoned_block_device_info *zbd_info,
>  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(&z->mutex);

Your change is not affecting the locking model, but I wonder if it would not be
better to lock the zone before calling zbd_reset_wp() so that the device side
zone reset and the update "z->wp = z->start" are atomically executed...

> +	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++;
> +
> +	return ret;
>  }
>  
>  /* The caller must hold f->zbd_info->mutex */
> 

Anyway, since there does not seem to be any problem with the current locking
scheme, looks good.

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

-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 2/3] zbd: simplify zone reset code

[Dmitry Fomichev]

> -----Original Message-----
> From: Damien Le Moal <Damien.LeMoal@wdc.com>
> Sent: Monday, July 27, 2020 1:07 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 2/3] zbd: simplify zone reset code
> 
> On 2020/07/27 12:16, 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.
> >
> > No functional change.
> >
> > 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..f6ccf299 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)
> >  {
> > @@ -735,10 +687,36 @@ static unsigned int zbd_zone_nr(struct
> zoned_block_device_info *zbd_info,
> >  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(&z->mutex);
> 
> Your change is not affecting the locking model, but I wonder if it would not
> be
> better to lock the zone before calling zbd_reset_wp() so that the device side
> zone reset and the update "z->wp = z->start" are atomically executed...

I am seeing that zbd_reset_zone() is always called with the zone already locked
and we can remove that locking inside this function (need to add a note in the
description that the caller must hold z->mutex). Overall, the additional change is

--- a/zbd.c
+++ b/zbd.c
@@ -691,24 +691,26 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,

        dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
                zbd_zone_nr(f->zbd_info, z));
+
+       pthread_mutex_lock(&f->zbd_info->mutex);
+
        switch (f->zbd_info->model) {
        case ZBD_HOST_AWARE:
        case ZBD_HOST_MANAGED:
                ret = zbd_reset_wp(td, f, offset, length);
-               if (ret < 0)
+               if (ret < 0) {
+                       pthread_mutex_unlock(&f->zbd_info->mutex);
                        return ret;
+               }
                break;
        default:
                break;
        }

-       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++;

I tried this now and I don't see any problems. This probably needs to be a separate
patch. I can add it to this series. What do you suggest?

> 
> > +	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++;
> > +
> > +	return ret;
> >  }
> >
> >  /* The caller must hold f->zbd_info->mutex */
> >
> 
> Anyway, since there does not seem to be any problem with the current
> locking
> scheme, looks good.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
> 
> --
> Damien Le Moal
> Western Digital Research

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

[Damien Le Moal]

On 2020/07/29 8:01, Dmitry Fomichev wrote:
> 
>> -----Original Message-----
>> From: Damien Le Moal <Damien.LeMoal@wdc.com>
>> Sent: Monday, July 27, 2020 1:07 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 2/3] zbd: simplify zone reset code
>>
>> On 2020/07/27 12:16, 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.
>>>
>>> No functional change.
>>>
>>> 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..f6ccf299 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)
>>>  {
>>> @@ -735,10 +687,36 @@ static unsigned int zbd_zone_nr(struct
>> zoned_block_device_info *zbd_info,
>>>  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(&z->mutex);
>>
>> Your change is not affecting the locking model, but I wonder if it would not
>> be
>> better to lock the zone before calling zbd_reset_wp() so that the device side
>> zone reset and the update "z->wp = z->start" are atomically executed...
> 
> I am seeing that zbd_reset_zone() is always called with the zone already locked
> and we can remove that locking inside this function (need to add a note in the
> description that the caller must hold z->mutex). Overall, the additional change is
> 
> --- a/zbd.c
> +++ b/zbd.c
> @@ -691,24 +691,26 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
> 
>         dprint(FD_ZBD, "%s: resetting wp of zone %u.\n", f->file_name,
>                 zbd_zone_nr(f->zbd_info, z));
> +
> +       pthread_mutex_lock(&f->zbd_info->mutex);

I do not think this is needed.

> +
>         switch (f->zbd_info->model) {
>         case ZBD_HOST_AWARE:
>         case ZBD_HOST_MANAGED:
>                 ret = zbd_reset_wp(td, f, offset, length);
> -               if (ret < 0)
> +               if (ret < 0) {
> +                       pthread_mutex_unlock(&f->zbd_info->mutex);

Neither is this.

>                         return ret;
> +               }
>                 break;
>         default:
>                 break;
>         }
> 
> -       pthread_mutex_lock(&z->mutex);

Checking too, yes, the zone is already locked when zbd_reset_zone() is called.
So this can go away.

> -       pthread_mutex_lock(&f->zbd_info->mutex);

But this one needs to stay to protect sectors_with_data.


>         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++;
> 
> I tried this now and I don't see any problems. This probably needs to be a separate
> patch. I can add it to this series. What do you suggest?

I would just squash the above changes in your initial patch as a v2. Since the
original patch is already moving the mutex calls around, you may as well change
them in the same patch with a mention about it in the commit message.


-- 
Damien Le Moal
Western Digital Research

[PATCH 3/3] 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 3/3] t/zbd: check log file for failed assertions

[Damien Le Moal]

On 2020/07/27 12:16, 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.

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

-- 
Damien Le Moal
Western Digital Research