zbd: fix sequential verify with max_open_zones=1

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

If verify is off, then job runs correctly and IO is sequential, and restarted from offset 0 and remains sequential.

If verify is on, then 1 full run is done and verified correctly. At this point there is exactly 1 open zone which is the last zone.

Now IO restarts from offset 0 and pick_random_zone() picks opened zone #0 which is the last zone because offset is 0. All IO is redirected to the last zone, which is rewritten once triggering verify again.

IO pattern becomes: 1 full sequential rewrite followed by constant sequential rewrites of the last zone.

	[global]
	filename=/dev/loop0
	direct=1
	zonemode=zbd
	zonesize=1M
	bs=512K
	rw=write
	verify=xxhash
	[j]
	max_open_zones=1
	io_size=3G

Fix is to close everything knowing that "full reset" comes from verify.

max_open_zones=2 restarts from half of the device, etc.

Signed-off-by Alexey Dobriyan (SK hynix) <[email protected]>

---

 zbd.c |   53 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

zbd.c +18 -1

@@ -710,6 +710,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 	return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
 }
 
+/* The caller must hold f->zbd_info->mutex */
+static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
+			   unsigned int open_zone_idx)
+{
+	uint32_t zone_idx;
+
+	assert(open_zone_idx < f->zbd_info->num_open_zones);
+	zone_idx = f->zbd_info->open_zones[open_zone_idx];
+	memmove(f->zbd_info->open_zones + open_zone_idx,
+		f->zbd_info->open_zones + open_zone_idx + 1,
+		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+		sizeof(f->zbd_info->open_zones[0]));
+	f->zbd_info->num_open_zones--;
+	f->zbd_info->zone_info[zone_idx].open = 0;
+}
+
 /*
  * Reset a range of zones. Returns 0 upon success and 1 upon failure.
  * @td: fio thread data.

@@ -733,12 +749,27 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
 		zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
 	for (z = zb; z < ze; z++) {
+		uint32_t nz = z - f->zbd_info->zone_info;
+
 		if (!zbd_zone_swr(z))
 			continue;
 		zone_lock(td, z);
-		reset_wp = all_zones ? z->wp != z->start :

zbd.c +8 -9

+			}
+			pthread_mutex_unlock(&f->zbd_info->mutex);
+
+			reset_wp = z->wp != z->start;
+		} else {
+			reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
+					z->wp % min_bs != 0;
+		}
 		if (reset_wp) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
 			       f->file_name,

@@ -928,22 +959,6 @@ out:
 	return res;
 }
 
-/* The caller must hold f->zbd_info->mutex */
-static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
-			   unsigned int open_zone_idx)
-{
-	uint32_t zone_idx;
-
-	assert(open_zone_idx < f->zbd_info->num_open_zones);
-	zone_idx = f->zbd_info->open_zones[open_zone_idx];
-	memmove(f->zbd_info->open_zones + open_zone_idx,

zbd.c static int zbd_reset_zones(struct thread_data *td, struct fio_file *f, · line 764

Quoted 6 lines · Alexey Dobriyan

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

zbd.c +3 -0

+				if (f->zbd_info->open_zones[i] == nz) {
+					zbd_close_zone(td, f, i);
+				}

nit: you do not need the {} brackets for the if here.

With the nits above fixed, this looks OK to me.

It would be good to add a test case for this to t/zbd/test-zbd-support. Can you send something please ?

Quoted 3 lines · Damien Le Moal

Quoted 2 lines · Alexey Dobriyan

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

Quoted 5 lines

zbd.c static int zbd_reset_zones(struct thread_data *td, struct fio_file *f, · line 764

Quoted 3 lines

zbd.c +3 -0

+				if (f->zbd_info->open_zones[i] == nz) {
+					zbd_close_zone(td, f, i);
+				}

nit: you do not need the {} brackets for the if here.

I started to use {} everywhere, it is less thinking that way.

Quoted 5 lines

With the nits above fixed, this looks OK to me.

It would be good to add a test case for this to t/zbd/test-zbd-support. Can you send something please ?

I'll try. This needs a test with blktrace parsing and eveything.

Quoted 6 lines · Alexey Dobriyan

Quoted 2 lines · Damien Le Moal

It would be good to add a test case for this to t/zbd/test-zbd-support. Can you send something please ?

I'll try. This needs a test with blktrace parsing and eveything.

That sounds like an overkill.

Can't we use the debug output (debug=zbd) for
checking ? If that does not work, let's not add a test that complicates the test setup.

Quoted 11 lines · Alexey Dobriyan

Quoted 9 lines · Damien Le Moal

zbd.c static int zbd_reset_zones(struct thread_data *td, struct fio_file *f, · line 764

Quoted 6 lines · Alexey Dobriyan

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

zbd.c +3 -0

+				if (f->zbd_info->open_zones[i] == nz) {
+					zbd_close_zone(td, f, i);
+				}

nit: you do not need the {} brackets for the if here.

Quoted 1 line

I started to use {} everywhere, it is less thinking that way.

Sure, but that is not the coding style normally used in fio and many other open source C projects. I will let Jens decide here.

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

If verify is off, then job runs correctly and IO is sequential, and restarted from offset 0 and remains sequential.

If verify is on, then 1 full run is done and verified correctly. At this point there is exactly 1 open zone which is the last zone.

Now IO restarts from offset 0 and pick_random_zone() picks opened zone #0 which is the last zone because offset is 0. All IO is redirected to the last zone, which is rewritten once triggering verify again.

IO pattern becomes: 1 full sequential rewrite followed by constant sequential rewrites of the last zone.

	[global]
	filename=/dev/loop0
	direct=1
	zonemode=zbd
	zonesize=1M
	bs=512K
	rw=write
	verify=xxhash
	[j]
	max_open_zones=1
	io_size=3G

Fix is to close every zone given that verification acts as a barrier between jobs.

max_open_zones=2 can restart from half of the device, etc.

Signed-off-by Alexey Dobriyan (SK hynix) <[email protected]>

---

 zbd.c |   52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

zbd.c +33 -19

@@ -687,6 +687,22 @@ static int zbd_reset_zone(struct thread_data *td, struct fio_file *f,
 	return zbd_reset_range(td, f, z->start, (z+1)->start - z->start);
 }
 
+/* The caller must hold f->zbd_info->mutex */
+static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
+			   unsigned int open_zone_idx)
+{
+	uint32_t zone_idx;
+
+	assert(open_zone_idx < f->zbd_info->num_open_zones);
+	zone_idx = f->zbd_info->open_zones[open_zone_idx];
+	memmove(f->zbd_info->open_zones + open_zone_idx,
+		f->zbd_info->open_zones + open_zone_idx + 1,
+		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
+		sizeof(f->zbd_info->open_zones[0]));
+	f->zbd_info->num_open_zones--;
+	f->zbd_info->zone_info[zone_idx].open = 0;
+}
+
 /*
  * Reset a range of zones. Returns 0 upon success and 1 upon failure.
  * @td: fio thread data.

@@ -710,12 +726,26 @@ static int zbd_reset_zones(struct thread_data *td, struct fio_file *f,
 	dprint(FD_ZBD, "%s: examining zones %u .. %u\n", f->file_name,
 		zbd_zone_nr(f->zbd_info, zb), zbd_zone_nr(f->zbd_info, ze));
 	for (z = zb; z < ze; z++) {
+		uint32_t nz = z - f->zbd_info->zone_info;
+
 		if (!zbd_zone_swr(z))
 			continue;
 		zone_lock(td, z);
-		reset_wp = all_zones ? z->wp != z->start :
-				(td->o.td_ddir & TD_DDIR_WRITE) &&
-				z->wp % min_bs != 0;
+		if (all_zones) {
+			unsigned int i;
+
+			pthread_mutex_lock(&f->zbd_info->mutex);
+			for (i = 0; i < f->zbd_info->num_open_zones; i++) {
+				if (f->zbd_info->open_zones[i] == nz)
+					zbd_close_zone(td, f, i);
+			}
+			pthread_mutex_unlock(&f->zbd_info->mutex);
+
+			reset_wp = z->wp != z->start;
+		} else {
+			reset_wp = (td->o.td_ddir & TD_DDIR_WRITE) &&
+					z->wp % min_bs != 0;
+		}
 		if (reset_wp) {
 			dprint(FD_ZBD, "%s: resetting zone %u\n",
 			       f->file_name,

@@ -905,22 +935,6 @@ out:
 	return res;
 }
 
-/* The caller must hold f->zbd_info->mutex */
-static void zbd_close_zone(struct thread_data *td, const struct fio_file *f,
-			   unsigned int open_zone_idx)
-{
-	uint32_t zone_idx;
-
-	assert(open_zone_idx < f->zbd_info->num_open_zones);
-	zone_idx = f->zbd_info->open_zones[open_zone_idx];
-	memmove(f->zbd_info->open_zones + open_zone_idx,
-		f->zbd_info->open_zones + open_zone_idx + 1,
-		(ZBD_MAX_OPEN_ZONES - (open_zone_idx + 1)) *
-		sizeof(f->zbd_info->open_zones[0]));
-	f->zbd_info->num_open_zones--;
-	f->zbd_info->zone_info[zone_idx].open = 0;
-}
-
 /* Anything goes as long as it is not a constant. */
 static uint32_t pick_random_zone_idx(const struct fio_file *f,
 				     const struct io_u *io_u)

Sequential write with max_open_zones=1 has interesting (read: buggy) interaction with verify=.

If verify is off, then job runs correctly and IO is sequential, and restarted from offset 0 and remains sequential.

If verify is on, then 1 full run is done and verified correctly. At this point there is exactly 1 open zone which is the last zone.

Now IO restarts from offset 0 and pick_random_zone() picks opened zone #0 which is the last zone because offset is 0. All IO is redirected to the last zone, which is rewritten once triggering verify again.

IO pattern becomes: 1 full sequential rewrite followed by constant sequential rewrites of the last zone.

	[global]
	filename=/dev/loop0
	direct=1
	zonemode=zbd
	zonesize=1M
	bs=512K
	rw=write
	verify=xxhash
	[j]
	max_open_zones=1
	io_size=3G

Fix is to close every zone given that verification acts as a barrier between jobs.

max_open_zones=2 can restart from half of the device, etc.