Restore the check_swd() function in the ZBD code

PATCH In review Bart Van Assche 2 participants 8 messages Sep 28, 2018 – Sep 29, 2018

lore

Share & tools

lore

Bart Van Assche

author

2018-09-28 21:51 UTC #0 lore raw

cover Restore the check_swd() function in the ZBD code

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

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #1 lore raw

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

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 <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 t/zbd/functions | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/zbd/functions +4 -1

index 95f9bf456b9a..173f0ca64a42 100644
@@ -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}"
 }

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #2 lore raw

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

In reply to 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 <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 t/zbd/test-zbd-support | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/zbd/test-zbd-support +1 -1

index 6ee5055b57cf..afe229097ef6 100755
@@ -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'

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #3 lore raw

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

In reply to 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 <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 t/zbd/test-zbd-support | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/zbd/test-zbd-support +4 -3

index afe229097ef6..fd30f5c6ecca 100755
@@ -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() {

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #4 lore raw

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

In reply to Bart Van Assche

Signed-off-by Bart Van Assche <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 t/zbd/test-zbd-support | 8 ++++++++
 1 file changed, 8 insertions(+)

t/zbd/test-zbd-support +8 -0

index fd30f5c6ecca..2d7279109af9 100755
@@ -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

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #5 lore raw

PATCH 5/6 zbd: Restore zbd_check_swd()

In reply to 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 <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 zbd.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

zbd.c +26 -0

index aa08b8111534..7728f71d25a5 100644
@@ -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:

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

author

2018-09-28 21:51 UTC #6 lore raw

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

In reply to 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 <[email protected]>

Cc Damien Le Moal <[email protected]>

---
 zbd.c | 61 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 18 deletions(-)

zbd.c +43 ~~-18~~

index 7728f71d25a5..8acda1f62f32 100644
@@ -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

Signature · 1 line

2.19.0.605.g01d371f741-goog

Jens Axboe

2018-09-29 21:17 UTC #7 lore raw

In reply to Bart Van Assche

Quoted 10 lines · 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, applied.

Signature · 1 line

Jens Axboe