[PATCH 0/6] Restore the check_swd() function in the ZBD code

[PATCH 0/6] Restore the check_swd() function in the ZBD code

[Bart Van Assche]

Hi Jens,

The fio changes included in this patch series are as follows:
- The check_swd() function has been restored in such a way that the compiler
  can verify the syntactical correctness of the code.
- A new function zbd_process_swd() has been introduced to avoid duplicated
  code.
- The robustness of the ZBD test scripts has been improved.

Please consider these patches for the upstream fio repository.

Thanks,

Bart.

Bart Van Assche (6):
  t/zbd/functions: Make fio_reset_count() return 0 if no resets occurred
  t/zbd/test-zbd-support: Ensure that an assertion failure causes this
    test to fail
  t/zbd/test-zbd-support: Set fio aux path and forbid file creation
  t/zbd/test-zbd-support: Report a test summary when finished
  zbd: Restore zbd_check_swd()
  zbd: Avoid duplicating the code for calculating the number of sectors
    with data

 t/zbd/functions        |  5 ++-
 t/zbd/test-zbd-support | 17 +++++++---
 zbd.c                  | 75 +++++++++++++++++++++++++++++++++++-------
 3 files changed, 80 insertions(+), 17 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

[PATCH 1/6] t/zbd/functions: Make fio_reset_count() return 0 if no resets occurred

[Bart Van Assche]

This patch avoids that certain bash versions report the following complaint:

t/zbd/test-zbd-support: line 69: [: : integer expression expected

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/functions | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/zbd/functions b/t/zbd/functions
index 95f9bf456b9a..173f0ca64a42 100644
--- a/t/zbd/functions
+++ b/t/zbd/functions
@@ -102,5 +102,8 @@ fio_written() {
 }
 
 fio_reset_count() {
-    sed -n 's/^.*write:[^;]*; \([0-9]*\) zone resets$/\1/p'
+    local count
+
+    count=$(sed -n 's/^.*write:[^;]*; \([0-9]*\) zone resets$/\1/p')
+    echo "${count:-0}"
 }
-- 
2.19.0.605.g01d371f741-goog

[PATCH 2/6] t/zbd/test-zbd-support: Ensure that an assertion failure causes this test to fail

[Bart Van Assche]

Switch to threaded mode to make sure that an assertion failure results in
a non-zero exit status.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index 6ee5055b57cf..afe229097ef6 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -113,7 +113,7 @@ run_fio_on_seq() {
 # Check whether buffered writes are refused.
 test1() {
     run_fio --name=job1 --filename="$dev" --rw=write --direct=0 --bs=4K	\
-	    --size="${zone_size}"					\
+	    --size="${zone_size}" --thread=1				\
 	    --zonemode=zbd --zonesize="${zone_size}" 2>&1 |
 	tee -a "${logfile}.${test_number}" |
 	grep -q 'Using direct I/O is mandatory for writing to ZBD drives'
-- 
2.19.0.605.g01d371f741-goog

[PATCH 3/6] t/zbd/test-zbd-support: Set fio aux path and forbid file creation

[Bart Van Assche]

Set the fio aux path to /tmp to avoid cluttering the directory from which the
test scripts are run. Disallow file creation to make tests fail if the block
device against which these are run does not exist.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index afe229097ef6..fd30f5c6ecca 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -81,13 +81,14 @@ is_scsi_device() {
 }
 
 run_fio() {
-    local fio
+    local fio opts
 
     fio=$(dirname "$0")/../../fio
 
-    { echo; echo "fio $*"; echo; } >>"${logfile}.${test_number}"
+    opts=("--aux-path=/tmp" "--allow_file_create=0" "$@")
+    { echo; echo "fio ${opts[*]}"; echo; } >>"${logfile}.${test_number}"
 
-    "${dynamic_analyzer[@]}" "$fio" "$@"
+    "${dynamic_analyzer[@]}" "$fio" "${opts[@]}"
 }
 
 run_one_fio_job() {
-- 
2.19.0.605.g01d371f741-goog

[PATCH 4/6] t/zbd/test-zbd-support: Report a test summary when finished

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 t/zbd/test-zbd-support | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/zbd/test-zbd-support b/t/zbd/test-zbd-support
index fd30f5c6ecca..2d7279109af9 100755
--- a/t/zbd/test-zbd-support
+++ b/t/zbd/test-zbd-support
@@ -801,18 +801,26 @@ fi
 
 logfile=$0.log
 
+passed=0
+failed=0
 rc=0
 for test_number in "${tests[@]}"; do
     rm -f "${logfile}.${test_number}"
     echo -n "Running test $test_number ... "
     if eval "test$test_number"; then
 	status="PASS"
+	((passed++))
     else
 	status="FAIL"
+	((failed++))
 	rc=1
     fi
     echo "$status"
     echo "$status" >> "${logfile}.${test_number}"
 done
 
+echo "$passed tests passed"
+if [ $failed -gt 0 ]; then
+    echo " and $failed tests failed"
+fi
 exit $rc
-- 
2.19.0.605.g01d371f741-goog

[PATCH 5/6] zbd: Restore zbd_check_swd()

[Bart Van Assche]

A recent patch removed the check_swd() function. Restore it since this function
is useful when debugging fio. Rename the function into zbd_check_swd(). Insert
the calls to these function where no locks are held to avoid triggering a
deadlock. See also commit d60be7d51cbb ("zbd: Remove unused function and
variable").

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/zbd.c b/zbd.c
index aa08b8111534..7728f71d25a5 100644
--- a/zbd.c
+++ b/zbd.c
@@ -726,6 +726,28 @@ static bool zbd_dec_and_reset_write_cnt(const struct thread_data *td,
 	return write_cnt == 0;
 }
 
+/* Check whether the value of zbd_info.sectors_with_data is correct. */
+static void zbd_check_swd(const struct fio_file *f)
+{
+#if 0
+	struct fio_zone_info *zb, *ze, *z;
+	uint64_t swd = 0;
+
+	zb = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset)];
+	ze = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset +
+						  f->io_size)];
+	for (z = zb; z < ze; z++) {
+		pthread_mutex_lock(&z->mutex);
+		swd += z->wp - z->start;
+	}
+	pthread_mutex_lock(&f->zbd_info->mutex);
+	assert(f->zbd_info->sectors_with_data == swd);
+	pthread_mutex_unlock(&f->zbd_info->mutex);
+	for (z = zb; z < ze; z++)
+		pthread_mutex_unlock(&z->mutex);
+#endif
+}
+
 void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 {
 	struct fio_zone_info *zb, *ze, *z;
@@ -1077,6 +1099,8 @@ static void zbd_post_submit(const struct io_u *io_u, bool success)
 	}
 unlock:
 	pthread_mutex_unlock(&z->mutex);
+
+	zbd_check_swd(io_u->file);
 }
 
 bool zbd_unaligned_write(int error_code)
@@ -1129,6 +1153,8 @@ enum io_u_action zbd_adjust_block(struct thread_data *td, struct io_u *io_u)
 	    io_u->ddir == DDIR_READ && td->o.read_beyond_wp)
 		return io_u_accept;
 
+	zbd_check_swd(f);
+
 	pthread_mutex_lock(&zb->mutex);
 	switch (io_u->ddir) {
 	case DDIR_READ:
-- 
2.19.0.605.g01d371f741-goog

[PATCH 6/6] zbd: Avoid duplicating the code for calculating the number of sectors with data

[Bart Van Assche]

Introduce a new function process_swd() to avoid that almost identical code
occurs in check_swd() and zbd_file_reset(). Change the #if 0 / #endif pair
in check_swd() into if (false) to make sure that the compiler verifies the
syntactical correctness of that code.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <damien.lemoal@wdc.com>
---
 zbd.c | 61 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/zbd.c b/zbd.c
index 7728f71d25a5..8acda1f62f32 100644
--- a/zbd.c
+++ b/zbd.c
@@ -726,10 +726,14 @@ static bool zbd_dec_and_reset_write_cnt(const struct thread_data *td,
 	return write_cnt == 0;
 }
 
-/* Check whether the value of zbd_info.sectors_with_data is correct. */
-static void zbd_check_swd(const struct fio_file *f)
+enum swd_action {
+	CHECK_SWD,
+	SET_SWD,
+};
+
+/* Calculate the number of sectors with data (swd) and perform action 'a' */
+static uint64_t zbd_process_swd(const struct fio_file *f, enum swd_action a)
 {
-#if 0
 	struct fio_zone_info *zb, *ze, *z;
 	uint64_t swd = 0;
 
@@ -741,18 +745,49 @@ static void zbd_check_swd(const struct fio_file *f)
 		swd += z->wp - z->start;
 	}
 	pthread_mutex_lock(&f->zbd_info->mutex);
-	assert(f->zbd_info->sectors_with_data == swd);
+	switch (a) {
+	case CHECK_SWD:
+		assert(f->zbd_info->sectors_with_data == swd);
+		break;
+	case SET_SWD:
+		f->zbd_info->sectors_with_data = swd;
+		break;
+	}
 	pthread_mutex_unlock(&f->zbd_info->mutex);
 	for (z = zb; z < ze; z++)
 		pthread_mutex_unlock(&z->mutex);
-#endif
+
+	return swd;
+}
+
+/*
+ * The swd check is useful for debugging but takes too much time to leave
+ * it enabled all the time. Hence it is disabled by default.
+ */
+static const bool enable_check_swd = false;
+
+/* Check whether the value of zbd_info.sectors_with_data is correct. */
+static void zbd_check_swd(const struct fio_file *f)
+{
+	if (!enable_check_swd)
+		return;
+
+	zbd_process_swd(f, CHECK_SWD);
+}
+
+static void zbd_init_swd(struct fio_file *f)
+{
+	uint64_t swd;
+
+	swd = zbd_process_swd(f, SET_SWD);
+	dprint(FD_ZBD, "%s(%s): swd = %" PRIu64 "\n", __func__, f->file_name,
+	       swd);
 }
 
 void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 {
-	struct fio_zone_info *zb, *ze, *z;
+	struct fio_zone_info *zb, *ze;
 	uint32_t zone_idx_e;
-	uint64_t swd = 0;
 
 	if (!f->zbd_info)
 		return;
@@ -760,17 +795,7 @@ void zbd_file_reset(struct thread_data *td, struct fio_file *f)
 	zb = &f->zbd_info->zone_info[zbd_zone_idx(f, f->file_offset)];
 	zone_idx_e = zbd_zone_idx(f, f->file_offset + f->io_size);
 	ze = &f->zbd_info->zone_info[zone_idx_e];
-	for (z = zb ; z < ze; z++) {
-		pthread_mutex_lock(&z->mutex);
-		swd += z->wp - z->start;
-	}
-	pthread_mutex_lock(&f->zbd_info->mutex);
-	f->zbd_info->sectors_with_data = swd;
-	pthread_mutex_unlock(&f->zbd_info->mutex);
-	for (z = zb ; z < ze; z++)
-		pthread_mutex_unlock(&z->mutex);
-	dprint(FD_ZBD, "%s(%s): swd = %llu\n", __func__, f->file_name,
-		(unsigned long long) swd);
+	zbd_init_swd(f);
 	/*
 	 * If data verification is enabled reset the affected zones before
 	 * writing any data to avoid that a zone reset has to be issued while
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH 0/6] Restore the check_swd() function in the ZBD code

[Jens Axboe]

On 9/28/18 3:51 PM, Bart Van Assche wrote:
> Hi Jens,
> 
> The fio changes included in this patch series are as follows:
> - The check_swd() function has been restored in such a way that the compiler
>   can verify the syntactical correctness of the code.
> - A new function zbd_process_swd() has been introduced to avoid duplicated
>   code.
> - The robustness of the ZBD test scripts has been improved.
> 
> Please consider these patches for the upstream fio repository.

Thanks Bart, applied.

-- 
Jens Axboe