[PATCH 1/2] Adds check for numberio during verify phase.

[PATCH 1/2] Adds check for numberio during verify phase.

[Juan Casse]

We check numberio to detect stale blocks.

Signed-off-by: Juan Casse <jcasse@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
 HOWTO            |  7 +++++++
 init.c           | 33 +++++++++++++++++++++++++++++++++
 ioengine.h       |  1 +
 iolog.c          |  1 +
 iolog.h          |  1 +
 libfio.c         |  3 ++-
 options.c        |  9 +++++++++
 thread_options.h |  2 ++
 verify.c         | 10 ++++++++--
 9 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/HOWTO b/HOWTO
index 005dac2..14ff8b5 100644
--- a/HOWTO
+++ b/HOWTO
@@ -657,6 +657,13 @@ ioengine=str	Defines how the job issues io to the file. The following
 				filename, eg ioengine=external:/tmp/foo.o
 				to load ioengine foo.o in /tmp.
 
+data_integrity_check
+		If this option is given, fio will check the i/o number of
+		each block read back during the verification phase. Fio
+		checks numberio to detect stale blocks. Currently, this
+		option requires synchronous i/o, and equal-sized read and
+		write blocks. This option requires workloads that write data.
+
 iodepth=int	This defines how many io units to keep in flight against
 		the file. The default is 1 for each file defined in this
 		job, can be overridden with a larger value for higher
diff --git a/init.c b/init.c
index 1afc341..8e2e1ae 100644
--- a/init.c
+++ b/init.c
@@ -915,6 +915,35 @@ static char *make_filename(char *buf, struct thread_options *o,
 
 	return buf;
 }
+
+int data_integrity_ini(struct thread_data *td)
+{
+	if (td->o.td_ddir != TD_DDIR_WRITE &&
+		td->o.td_ddir != TD_DDIR_RW &&
+		td->o.td_ddir != TD_DDIR_RANDRW &&
+		td->o.td_ddir != TD_DDIR_RANDWRITE) {
+		fprintf(stderr,
+			"Option data_integrity_check requires a workload that "
+			"writes data\n");
+		return 1;
+	}
+
+	if (td->o.bs[DDIR_READ] != td->o.bs[DDIR_WRITE]) {
+		fprintf(stderr,
+			"Currently, option data_integrity_check requires "
+			"equal-sized read and write blocks\n");
+		return 1;
+	}
+
+	if (strcmp(td->o.ioengine, "sync")) {
+		log_err("Currently, option data_integrity_check requires "
+			"synchronous i/o\n");
+		return 1;
+	}
+
+	return 0;
+}
+
 /*
  * Adds a job to the list of things todo. Sanitizes the various options
  * to make sure we don't have conflicts, and initializes various
@@ -944,6 +973,10 @@ static int add_job(struct thread_data *td, const char *jobname, int job_add_num,
 		return 0;
 	}
 
+	if (o->data_integrity_check)
+		if (data_integrity_ini(td))
+			goto err;
+
 	td->client_type = client_type;
 
 	if (profile_td_init(td))
diff --git a/ioengine.h b/ioengine.h
index 31662eb..812febd 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -50,6 +50,7 @@ struct io_u {
 	 */
 	unsigned long buflen;
 	unsigned long long offset;
+	unsigned short numberio;
 	void *buf;
 
 	/*
diff --git a/iolog.c b/iolog.c
index 9bcf0d8..6459d3b 100644
--- a/iolog.c
+++ b/iolog.c
@@ -188,6 +188,7 @@ void log_io_piece(struct thread_data *td, struct io_u *io_u)
 	ipo->file = io_u->file;
 	ipo->offset = io_u->offset;
 	ipo->len = io_u->buflen;
+	ipo->numberio = io_u->numberio;
 
 	if (io_u_should_trim(td, io_u)) {
 		flist_add_tail(&ipo->trim_list, &td->trim_list);
diff --git a/iolog.h b/iolog.h
index 8fedc19..94e0fc1 100644
--- a/iolog.h
+++ b/iolog.h
@@ -78,6 +78,7 @@ struct io_piece {
 		struct fio_file *file;
 	};
 	unsigned long long offset;
+	unsigned short numberio;
 	unsigned long len;
 	unsigned int flags;
 	enum fio_ddir ddir;
diff --git a/libfio.c b/libfio.c
index c26d6a3..f003945 100644
--- a/libfio.c
+++ b/libfio.c
@@ -83,7 +83,8 @@ static void reset_io_counters(struct thread_data *td)
 		td->this_io_blocks[ddir] = 0;
 		td->rate_bytes[ddir] = 0;
 		td->rate_blocks[ddir] = 0;
-		td->io_issues[ddir] = 0;
+		if (!td->o.data_integrity_check)
+			td->io_issues[ddir] = 0;
 	}
 	td->zone_bytes = 0;
 
diff --git a/options.c b/options.c
index caf89d3..569cdf7 100644
--- a/options.c
+++ b/options.c
@@ -1935,6 +1935,15 @@ struct fio_option fio_options[FIO_MAX_OPTS] = {
 		.group	= FIO_OPT_G_RUNTIME,
 	},
 	{
+		.name	= "data_integrity_check",
+		.lname	= "Data integrity check",
+		.type	= FIO_OPT_STR_SET,
+		.off1	= td_var_offset(data_integrity_check),
+		.help	= "Checks numberio during verify phase",
+		.category = FIO_OPT_C_GENERAL,
+		.group	= FIO_OPT_G_RUNTIME,
+	},
+	{
 		.name	= "ramp_time",
 		.lname	= "Ramp time",
 		.type	= FIO_OPT_STR_VAL_TIME,
diff --git a/thread_options.h b/thread_options.h
index 3f345c5..8666cd1 100644
--- a/thread_options.h
+++ b/thread_options.h
@@ -107,6 +107,8 @@ struct thread_options {
 	unsigned int fsync_on_close;
 	unsigned int bs_is_seq_rand;
 
+	unsigned int data_integrity_check;
+
 	unsigned int random_distribution;
 
 	fio_fp64_t zipf_theta;
diff --git a/verify.c b/verify.c
index 9e88d61..83c5735 100644
--- a/verify.c
+++ b/verify.c
@@ -369,6 +369,10 @@ static int verify_io_u_meta(struct verify_header *hdr, struct vcont *vc)
 	if (td->o.verify_pattern_bytes)
 		ret |= verify_io_u_pattern(hdr, vc);
 
+	if (td->o.data_integrity_check)
+		if (vh->numberio != io_u->numberio)
+			ret = EILSEQ;
+
 	if (!ret)
 		return 0;
 
@@ -768,8 +772,7 @@ static void fill_meta(struct verify_header *hdr, struct thread_data *td,
 	vh->time_sec = io_u->start_time.tv_sec;
 	vh->time_usec = io_u->start_time.tv_usec;
 
-	vh->numberio = td->io_issues[DDIR_WRITE];
-
+	vh->numberio = io_u->numberio;
 	vh->offset = io_u->offset + header_num * td->o.verify_interval;
 }
 
@@ -942,6 +945,8 @@ void populate_verify_io_u(struct thread_data *td, struct io_u *io_u)
 	if (td->o.verify == VERIFY_NULL)
 		return;
 
+	io_u->numberio = td->io_issues[DDIR_WRITE];
+
 	fill_pattern_headers(td, io_u, 0, 0);
 }
 
@@ -974,6 +979,7 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
 
 		io_u->offset = ipo->offset;
 		io_u->buflen = ipo->len;
+		io_u->numberio = ipo->numberio;
 		io_u->file = ipo->file;
 		io_u->flags |= IO_U_F_VER_LIST;
 
-- 
1.7.12.4

[PATCH 2/2] Adds check for rand_seed during verify phase.

[Juan Casse]

Improve data integrity checking and detect stale blocks from previous
runs.

Signed-off-by: Juan Casse <jcasse@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
 verify.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/verify.c b/verify.c
index 83c5735..deabf76 100644
--- a/verify.c
+++ b/verify.c
@@ -648,18 +648,17 @@ static int verify_header(struct io_u *io_u, struct verify_header *hdr)
 	uint32_t crc;
 
 	if (hdr->magic != FIO_HDR_MAGIC)
-		return 0;
-	if (hdr->len > io_u->buflen) {
-		log_err("fio: verify header exceeds buffer length (%u > %lu)\n", hdr->len, io_u->buflen);
-		return 0;
-	}
+		return 1;
+	if (hdr->len > io_u->buflen)
+		return 2;
+	if (hdr->rand_seed != io_u->rand_seed)
+		return 3;
 
 	crc = fio_crc32c(p, offsetof(struct verify_header, crc32));
 	if (crc == hdr->crc32)
-		return 1;
-
+		return 0;
 	log_err("fio: verify header crc %x, calculated %x\n", hdr->crc32, crc);
-	return 0;
+	return 4;
 }
 
 int verify_io_u(struct thread_data *td, struct io_u *io_u)
@@ -696,13 +695,41 @@ int verify_io_u(struct thread_data *td, struct io_u *io_u)
 			memswp(p, p + td->o.verify_offset, header_size);
 		hdr = p;
 
-		if (!verify_header(io_u, hdr)) {
+		ret = verify_header(io_u, hdr);
+		switch (ret) {
+		case 0:
+			break;
+		case 1:
 			log_err("verify: bad magic header %x, wanted %x at "
 				"file %s offset %llu, length %u\n",
 				hdr->magic, FIO_HDR_MAGIC,
 				io_u->file->file_name,
 				io_u->offset + hdr_num * hdr->len, hdr->len);
 			return EILSEQ;
+			break;
+		case 2:
+			log_err("fio: verify header exceeds buffer length (%u "
+				"> %lu)\n", hdr->len, io_u->buflen);
+			return EILSEQ;
+			break;
+		case 3:
+			log_err("verify: bad header rand_seed %"PRIu64
+				", wanted %"PRIu64" at file %s offset %llu, "
+				"length %u\n",
+				hdr->rand_seed, io_u->rand_seed,
+				io_u->file->file_name,
+				io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
+			break;
+		case 4:
+			return EILSEQ;
+			break;
+		default:
+			log_err("verify: unknown header error at file %s "
+			"offset %llu, length %u\n",
+			io_u->file->file_name,
+			io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
 		}
 
 		if (td->o.verify != VERIFY_NONE)
-- 
1.7.12.4

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Grant Grundler]

Jens, et al,

In general I'm ok with this patch...just one issue I'd like to get an
opinion on...

On Mon, Aug 26, 2013 at 2:22 PM, Juan Casse <jcasse@chromium.org> wrote:
> Improve data integrity checking and detect stale blocks from previous
> runs.
>
> Signed-off-by: Juan Casse <jcasse@chromium.org>
> Reviewed-by: Grant Grundler <grundler@chromium.org>
> ---
>  verify.c | 45 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/verify.c b/verify.c
> index 83c5735..deabf76 100644
> --- a/verify.c
> +++ b/verify.c
> @@ -648,18 +648,17 @@ static int verify_header(struct io_u *io_u, struct verify_header *hdr)
>         uint32_t crc;
>
>         if (hdr->magic != FIO_HDR_MAGIC)
> -               return 0;
> -       if (hdr->len > io_u->buflen) {
> -               log_err("fio: verify header exceeds buffer length (%u > %lu)\n", hdr->len, io_u->buflen);
> -               return 0;
> -       }
> +               return 1;
> +       if (hdr->len > io_u->buflen)
> +               return 2;
> +       if (hdr->rand_seed != io_u->rand_seed)
> +               return 3;
>
>         crc = fio_crc32c(p, offsetof(struct verify_header, crc32));
>         if (crc == hdr->crc32)
> -               return 1;
> -
> +               return 0;
>         log_err("fio: verify header crc %x, calculated %x\n", hdr->crc32, crc);
> -       return 0;
> +       return 4;
>  }
>
>  int verify_io_u(struct thread_data *td, struct io_u *io_u)
> @@ -696,13 +695,41 @@ int verify_io_u(struct thread_data *td, struct io_u *io_u)
>                         memswp(p, p + td->o.verify_offset, header_size);
>                 hdr = p;
>
> -               if (!verify_header(io_u, hdr)) {
> +               ret = verify_header(io_u, hdr);
> +               switch (ret) {
> +               case 0:
> +                       break;
> +               case 1:
>                         log_err("verify: bad magic header %x, wanted %x at "
>                                 "file %s offset %llu, length %u\n",
>                                 hdr->magic, FIO_HDR_MAGIC,
>                                 io_u->file->file_name,
>                                 io_u->offset + hdr_num * hdr->len, hdr->len);
>                         return EILSEQ;
> +                       break;
> +               case 2:
> +                       log_err("fio: verify header exceeds buffer length (%u "
> +                               "> %lu)\n", hdr->len, io_u->buflen);
> +                       return EILSEQ;

Should we return different error values for each different possible mismatch?

I think we should but don't know which error values would be preferred here.

The reason is automated testing will log a failure and I want a
different failure code for each possible failure mode. That way, I can
easily tell if a different data integrity check fails and which
systems are reporting a particular error code.

I understand, some error codes may have many different root causes.
But I should be able to tell the difference between "magic number is
wrong", "IO Timed out", and "generation number is wrong" at the first
cut.

thanks,
grant

> +                       break;
> +               case 3:
> +                       log_err("verify: bad header rand_seed %"PRIu64
> +                               ", wanted %"PRIu64" at file %s offset %llu, "
> +                               "length %u\n",
> +                               hdr->rand_seed, io_u->rand_seed,
> +                               io_u->file->file_name,
> +                               io_u->offset + hdr_num * hdr->len, hdr->len);
> +                       return EILSEQ;
> +                       break;
> +               case 4:
> +                       return EILSEQ;
> +                       break;
> +               default:
> +                       log_err("verify: unknown header error at file %s "
> +                       "offset %llu, length %u\n",
> +                       io_u->file->file_name,
> +                       io_u->offset + hdr_num * hdr->len, hdr->len);
> +                       return EILSEQ;
>                 }
>
>                 if (td->o.verify != VERIFY_NONE)
> --
> 1.7.12.4
>

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Jens Axboe]

On 08/26/2013 04:11 PM, Grant Grundler wrote:
> Jens, et al,
> 
> In general I'm ok with this patch...just one issue I'd like to get an
> opinion on...
> 
> On Mon, Aug 26, 2013 at 2:22 PM, Juan Casse <jcasse@chromium.org> wrote:
>> Improve data integrity checking and detect stale blocks from previous
>> runs.
>>
>> Signed-off-by: Juan Casse <jcasse@chromium.org>
>> Reviewed-by: Grant Grundler <grundler@chromium.org>
>> ---
>>  verify.c | 45 ++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 36 insertions(+), 9 deletions(-)
>>
>> diff --git a/verify.c b/verify.c
>> index 83c5735..deabf76 100644
>> --- a/verify.c
>> +++ b/verify.c
>> @@ -648,18 +648,17 @@ static int verify_header(struct io_u *io_u, struct verify_header *hdr)
>>         uint32_t crc;
>>
>>         if (hdr->magic != FIO_HDR_MAGIC)
>> -               return 0;
>> -       if (hdr->len > io_u->buflen) {
>> -               log_err("fio: verify header exceeds buffer length (%u > %lu)\n", hdr->len, io_u->buflen);
>> -               return 0;
>> -       }
>> +               return 1;
>> +       if (hdr->len > io_u->buflen)
>> +               return 2;
>> +       if (hdr->rand_seed != io_u->rand_seed)
>> +               return 3;
>>
>>         crc = fio_crc32c(p, offsetof(struct verify_header, crc32));
>>         if (crc == hdr->crc32)
>> -               return 1;
>> -
>> +               return 0;
>>         log_err("fio: verify header crc %x, calculated %x\n", hdr->crc32, crc);
>> -       return 0;
>> +       return 4;
>>  }
>>
>>  int verify_io_u(struct thread_data *td, struct io_u *io_u)
>> @@ -696,13 +695,41 @@ int verify_io_u(struct thread_data *td, struct io_u *io_u)
>>                         memswp(p, p + td->o.verify_offset, header_size);
>>                 hdr = p;
>>
>> -               if (!verify_header(io_u, hdr)) {
>> +               ret = verify_header(io_u, hdr);
>> +               switch (ret) {
>> +               case 0:
>> +                       break;
>> +               case 1:
>>                         log_err("verify: bad magic header %x, wanted %x at "
>>                                 "file %s offset %llu, length %u\n",
>>                                 hdr->magic, FIO_HDR_MAGIC,
>>                                 io_u->file->file_name,
>>                                 io_u->offset + hdr_num * hdr->len, hdr->len);
>>                         return EILSEQ;
>> +                       break;
>> +               case 2:
>> +                       log_err("fio: verify header exceeds buffer length (%u "
>> +                               "> %lu)\n", hdr->len, io_u->buflen);
>> +                       return EILSEQ;
> 
> Should we return different error values for each different possible mismatch?
> 
> I think we should but don't know which error values would be preferred here.
> 
> The reason is automated testing will log a failure and I want a
> different failure code for each possible failure mode. That way, I can
> easily tell if a different data integrity check fails and which
> systems are reporting a particular error code.
> 
> I understand, some error codes may have many different root causes.
> But I should be able to tell the difference between "magic number is
> wrong", "IO Timed out", and "generation number is wrong" at the first
> cut.

Right now fio will return EILSEQ for any verification failure. If you
time out, then no, you should not get that. If we do more fine grained
errors for verify failures, then we would have to get creative on what
to use. And not sure I'm a huge fan of that. If you do get a
verification failure, though, at least the runtime log should tell you
EXACTLY what it was.

-- 
Jens Axboe

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Grant Grundler]

On Tue, Aug 27, 2013 at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
...
> Right now fio will return EILSEQ for any verification failure. If you
> time out, then no, you should not get that. If we do more fine grained
> errors for verify failures, then we would have to get creative on what
> to use. And not sure I'm a huge fan of that.

I'm not a fan of that either since it's "open ended". Using EILSEQ is
probably fine.

> If you do get a
> verification failure, though, at least the runtime log should tell you
> EXACTLY what it was.

Ah ok - I didn't think of that and the test can look there to
disambiguate the return code.

thanks!
grant

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Erwan Velu]

On 27/08/2013 00:11, Grant Grundler wrote:
> Jens, et al,
>
> In general I'm ok with this patch...just one issue I'd like to get an
> opinion on...
>
I don't have the skill to make a technical review of it.

At least, I'm a great fan of well detailed commits.
At my taste, I do like when a commit does explain what was the problem 
and what is the solution.

A single line for such commit seems a little bit light for me.

Cheers,

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Jens Axboe]

On Wed, Aug 28 2013, Erwan Velu wrote:
> On 27/08/2013 00:11, Grant Grundler wrote:
> >Jens, et al,
> >
> >In general I'm ok with this patch...just one issue I'd like to get an
> >opinion on...
> >
> I don't have the skill to make a technical review of it.
> 
> At least, I'm a great fan of well detailed commits.
> At my taste, I do like when a commit does explain what was the problem and
> what is the solution.
> 
> A single line for such commit seems a little bit light for me.

Agree. A good commit message describes the need for the change. It needs
to justify its own inclusion, so to speak. Basically it answers the key
question of why this change is needed.

-- 
Jens Axboe

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Erwan Velu]

On 31/08/2013 06:31, Jens Axboe wrote:
> [...]
> Agree. A good commit message describes the need for the change. It needs
> to justify its own inclusion, so to speak. Basically it answers the key
> question of why this change is needed.
>

I don't know how you play with fio, but on my project I'm working on, 
that's a good reason to refuse a patch even if the code is correct :o)

Cheers,
Erwan

Re: [PATCH 2/2] Adds check for rand_seed during verify phase.

[Jens Axboe]

On Mon, Sep 02 2013, Erwan Velu wrote:
> On 31/08/2013 06:31, Jens Axboe wrote:
> >[...]
> >Agree. A good commit message describes the need for the change. It needs
> >to justify its own inclusion, so to speak. Basically it answers the key
> >question of why this change is needed.
> >
> 
> I don't know how you play with fio, but on my project I'm working on, that's
> a good reason to refuse a patch even if the code is correct :o)

I tend to be (too) nice and provide the missing commit log, especially
if it's for a feature and needs a description. But I have been known to
push back as well and just ask for a description. Usually has not been a
problem.

-- 
Jens Axboe

Re: [PATCH 1/2] Adds check for numberio during verify phase.

[Jens Axboe]

On 08/26/2013 03:22 PM, Juan Casse wrote:
> We check numberio to detect stale blocks.
> 
> Signed-off-by: Juan Casse <jcasse@chromium.org>
> Reviewed-by: Grant Grundler <grundler@chromium.org>
> ---
>  HOWTO            |  7 +++++++
>  init.c           | 33 +++++++++++++++++++++++++++++++++
>  ioengine.h       |  1 +
>  iolog.c          |  1 +
>  iolog.h          |  1 +
>  libfio.c         |  3 ++-
>  options.c        |  9 +++++++++
>  thread_options.h |  2 ++
>  verify.c         | 10 ++++++++--
>  9 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/HOWTO b/HOWTO
> index 005dac2..14ff8b5 100644
> --- a/HOWTO
> +++ b/HOWTO
> @@ -657,6 +657,13 @@ ioengine=str	Defines how the job issues io to the file. The following
>  				filename, eg ioengine=external:/tmp/foo.o
>  				to load ioengine foo.o in /tmp.
>  
> +data_integrity_check
> +		If this option is given, fio will check the i/o number of
> +		each block read back during the verification phase. Fio
> +		checks numberio to detect stale blocks. Currently, this
> +		option requires synchronous i/o, and equal-sized read and
> +		write blocks. This option requires workloads that write data.

I think this use case is just too narrow. Why does it require sync IO
and equal read/write sizes? Can't you just replay and re-generate and
compare?

-- 
Jens Axboe

Re: [PATCH 1/2] Adds check for numberio during verify phase.

[Grant Grundler]

On Tue, Aug 27, 2013 at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
...
>> +data_integrity_check
>> +             If this option is given, fio will check the i/o number of
>> +             each block read back during the verification phase. Fio
>> +             checks numberio to detect stale blocks. Currently, this
>> +             option requires synchronous i/o, and equal-sized read and
>> +             write blocks. This option requires workloads that write data.
>
> I think this use case is just too narrow.

Jens.
This behavior should be standard behavior for the data integrity
checking. To debug data corruption problems we need to know if it's
stale data or corrupted data. Huge difference in how to track it down
and potential causes. I'm arguing it shouldn't even be an option.

> Why does it require sync IO and equal read/write sizes?
> Can't you just replay and re-generate and compare?

I think it's just limitations of this implementation and testing. In
principle I think you are right. Perhaps Juan can explain in more
detail.

cheers,
grant

Re: [PATCH 1/2] Adds check for numberio during verify phase.

[Juan Casse]

Jens,

sync IO:
You're right, this should work. The code change simply adds the check
for numberio to the existing fio infrastructure. I think my first
attempt had some problems and I did not test asynchronous io on my
finished code. I will run some tests to make sure that it works with
asynchronous io.

equal size read/writes:
I just realized after running a quick test that there is a problem.
With different read/write sizes, fio exited with an error:
fio: verify.c:960: populate_hdr: Assertion '0' failed.
I think this has to do with the offsets of the read and write blocks
being different when they're of different sizes. I need to figure out
what is going on. Any ideas Jens?

Here is the job used:
readwrite=randrw
randrepeat=1
size=1m
bs=4k,8k
ioengine=libaio
direct=1
buffered=0
rwmixread=30
rwmixwrite=70
norandommap
loops=2
verify=meta
verify_pattern=0xffffffffffffffff
verify_dump=1
continue_on_error=verify

On Tue, Aug 27, 2013 at 10:22 AM, Grant Grundler <grundler@chromium.org> wrote:
> On Tue, Aug 27, 2013 at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
> ...
>>> +data_integrity_check
>>> +             If this option is given, fio will check the i/o number of
>>> +             each block read back during the verification phase. Fio
>>> +             checks numberio to detect stale blocks. Currently, this
>>> +             option requires synchronous i/o, and equal-sized read and
>>> +             write blocks. This option requires workloads that write data.
>>
>> I think this use case is just too narrow.
>
> Jens.
> This behavior should be standard behavior for the data integrity
> checking. To debug data corruption problems we need to know if it's
> stale data or corrupted data. Huge difference in how to track it down
> and potential causes. I'm arguing it shouldn't even be an option.
>
>> Why does it require sync IO and equal read/write sizes?
>> Can't you just replay and re-generate and compare?
>
> I think it's just limitations of this implementation and testing. In
> principle I think you are right. Perhaps Juan can explain in more
> detail.
>
> cheers,
> grant

Re: [PATCH 1/2] Adds check for numberio during verify phase.

[Juan Casse]

Jens,

I found an fio bug or am i not understanding how it works?

I just ran the latest fio version without my changes and it failed
with different block sizes for read and writes.

fio --version
=========
fio-2.1.2-13-g3e10

error message
============
fio: bad verify type: 0
fio: verify.c:930: populate_hdr: Assertion `0' failed.
fio: pid=18551, got signal=6

job file
=====
readwrite=rw
randrepeat=1
size=64k
bs=4k,8k
ioengine=sync
direct=1
buffered=0
rwmixread=50
rwmixwrite=50
norandommap
loops=1
verify=meta
verify_pattern=0xffffffffffffffff
verify_dump=1
continue_on_error=verify


On Tue, Aug 27, 2013 at 10:50 AM, Juan Casse <jcasse@google.com> wrote:
> Jens,
>
> sync IO:
> You're right, this should work. The code change simply adds the check
> for numberio to the existing fio infrastructure. I think my first
> attempt had some problems and I did not test asynchronous io on my
> finished code. I will run some tests to make sure that it works with
> asynchronous io.
>
> equal size read/writes:
> I just realized after running a quick test that there is a problem.
> With different read/write sizes, fio exited with an error:
> fio: verify.c:960: populate_hdr: Assertion '0' failed.
> I think this has to do with the offsets of the read and write blocks
> being different when they're of different sizes. I need to figure out
> what is going on. Any ideas Jens?
>
> Here is the job used:
> readwrite=randrw
> randrepeat=1
> size=1m
> bs=4k,8k
> ioengine=libaio
> direct=1
> buffered=0
> rwmixread=30
> rwmixwrite=70
> norandommap
> loops=2
> verify=meta
> verify_pattern=0xffffffffffffffff
> verify_dump=1
> continue_on_error=verify
>
> On Tue, Aug 27, 2013 at 10:22 AM, Grant Grundler <grundler@chromium.org> wrote:
>> On Tue, Aug 27, 2013 at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> ...
>>>> +data_integrity_check
>>>> +             If this option is given, fio will check the i/o number of
>>>> +             each block read back during the verification phase. Fio
>>>> +             checks numberio to detect stale blocks. Currently, this
>>>> +             option requires synchronous i/o, and equal-sized read and
>>>> +             write blocks. This option requires workloads that write data.
>>>
>>> I think this use case is just too narrow.
>>
>> Jens.
>> This behavior should be standard behavior for the data integrity
>> checking. To debug data corruption problems we need to know if it's
>> stale data or corrupted data. Huge difference in how to track it down
>> and potential causes. I'm arguing it shouldn't even be an option.
>>
>>> Why does it require sync IO and equal read/write sizes?
>>> Can't you just replay and re-generate and compare?
>>
>> I think it's just limitations of this implementation and testing. In
>> principle I think you are right. Perhaps Juan can explain in more
>> detail.
>>
>> cheers,
>> grant

Re: [PATCH 1/2] Adds check for numberio during verify phase.

[Juan Casse]

Hi Jens:

Update on sync IO:

I ran the code (with my changes) using asynchronous engine libaio with
mixed read/write and found no problems. So, yes, I can eliminate the
restriction of synchronous io.

This makes sense since I'm not changing the behavior of fio besides
checking numberio.

To summarize my change:
When the data_integrity_check option is selected, fio will:
1. not reset the numberio after each iteration
2. check numberio during the verify phase

On Tue, Aug 27, 2013 at 10:50 AM, Juan Casse <jcasse@google.com> wrote:
> Jens,
>
> sync IO:
> You're right, this should work. The code change simply adds the check
> for numberio to the existing fio infrastructure. I think my first
> attempt had some problems and I did not test asynchronous io on my
> finished code. I will run some tests to make sure that it works with
> asynchronous io.
>
> equal size read/writes:
> I just realized after running a quick test that there is a problem.
> With different read/write sizes, fio exited with an error:
> fio: verify.c:960: populate_hdr: Assertion '0' failed.
> I think this has to do with the offsets of the read and write blocks
> being different when they're of different sizes. I need to figure out
> what is going on. Any ideas Jens?
>
> Here is the job used:
> readwrite=randrw
> randrepeat=1
> size=1m
> bs=4k,8k
> ioengine=libaio
> direct=1
> buffered=0
> rwmixread=30
> rwmixwrite=70
> norandommap
> loops=2
> verify=meta
> verify_pattern=0xffffffffffffffff
> verify_dump=1
> continue_on_error=verify
>
> On Tue, Aug 27, 2013 at 10:22 AM, Grant Grundler <grundler@chromium.org> wrote:
>> On Tue, Aug 27, 2013 at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
>> ...
>>>> +data_integrity_check
>>>> +             If this option is given, fio will check the i/o number of
>>>> +             each block read back during the verification phase. Fio
>>>> +             checks numberio to detect stale blocks. Currently, this
>>>> +             option requires synchronous i/o, and equal-sized read and
>>>> +             write blocks. This option requires workloads that write data.
>>>
>>> I think this use case is just too narrow.
>>
>> Jens.
>> This behavior should be standard behavior for the data integrity
>> checking. To debug data corruption problems we need to know if it's
>> stale data or corrupted data. Huge difference in how to track it down
>> and potential causes. I'm arguing it shouldn't even be an option.
>>
>>> Why does it require sync IO and equal read/write sizes?
>>> Can't you just replay and re-generate and compare?
>>
>> I think it's just limitations of this implementation and testing. In
>> principle I think you are right. Perhaps Juan can explain in more
>> detail.
>>
>> cheers,
>> grant

[PATCH 2/2] Adds check for rand_seed during verify phase.

[Juan Casse]

Improve data integrity checking and detect stale blocks from previous
runs.

Signed-off-by: Juan Casse <jcasse@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
 verify.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/verify.c b/verify.c
index 83c5735..deabf76 100644
--- a/verify.c
+++ b/verify.c
@@ -648,18 +648,17 @@ static int verify_header(struct io_u *io_u, struct verify_header *hdr)
 	uint32_t crc;
 
 	if (hdr->magic != FIO_HDR_MAGIC)
-		return 0;
-	if (hdr->len > io_u->buflen) {
-		log_err("fio: verify header exceeds buffer length (%u > %lu)\n", hdr->len, io_u->buflen);
-		return 0;
-	}
+		return 1;
+	if (hdr->len > io_u->buflen)
+		return 2;
+	if (hdr->rand_seed != io_u->rand_seed)
+		return 3;
 
 	crc = fio_crc32c(p, offsetof(struct verify_header, crc32));
 	if (crc == hdr->crc32)
-		return 1;
-
+		return 0;
 	log_err("fio: verify header crc %x, calculated %x\n", hdr->crc32, crc);
-	return 0;
+	return 4;
 }
 
 int verify_io_u(struct thread_data *td, struct io_u *io_u)
@@ -696,13 +695,41 @@ int verify_io_u(struct thread_data *td, struct io_u *io_u)
 			memswp(p, p + td->o.verify_offset, header_size);
 		hdr = p;
 
-		if (!verify_header(io_u, hdr)) {
+		ret = verify_header(io_u, hdr);
+		switch (ret) {
+		case 0:
+			break;
+		case 1:
 			log_err("verify: bad magic header %x, wanted %x at "
 				"file %s offset %llu, length %u\n",
 				hdr->magic, FIO_HDR_MAGIC,
 				io_u->file->file_name,
 				io_u->offset + hdr_num * hdr->len, hdr->len);
 			return EILSEQ;
+			break;
+		case 2:
+			log_err("fio: verify header exceeds buffer length (%u "
+				"> %lu)\n", hdr->len, io_u->buflen);
+			return EILSEQ;
+			break;
+		case 3:
+			log_err("verify: bad header rand_seed %"PRIu64
+				", wanted %"PRIu64" at file %s offset %llu, "
+				"length %u\n",
+				hdr->rand_seed, io_u->rand_seed,
+				io_u->file->file_name,
+				io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
+			break;
+		case 4:
+			return EILSEQ;
+			break;
+		default:
+			log_err("verify: unknown header error at file %s "
+			"offset %llu, length %u\n",
+			io_u->file->file_name,
+			io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
 		}
 
 		if (td->o.verify != VERIFY_NONE)
-- 
1.7.12.4

[PATCH 1/2] Adds check for numberio during verify phase.

[Juan Casse]

Currently, fio checks the block offset number in a block's header during
the verify phase.
We add a check for the io number (numberio) to detect stale blocks. This
check is performed only on workloads that write data, as those workloads
know what numberio was written to the blocks.
This is separate from the verify_only option. The verify_only does a dry-run
of the specified workload and then verifies once at the lst iteration. The
implementation of verify_only will be submitted in a separate patch.

Signed-off-by: Juan Casse <jcasse@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
 ioengine.h |  1 +
 iolog.c    |  1 +
 iolog.h    |  1 +
 libfio.c   |  1 -
 verify.c   | 14 +++++++++++++-
 5 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/ioengine.h b/ioengine.h
index 31662eb..812febd 100644
--- a/ioengine.h
+++ b/ioengine.h
@@ -50,6 +50,7 @@ struct io_u {
 	 */
 	unsigned long buflen;
 	unsigned long long offset;
+	unsigned short numberio;
 	void *buf;
 
 	/*
diff --git a/iolog.c b/iolog.c
index 9bcf0d8..6459d3b 100644
--- a/iolog.c
+++ b/iolog.c
@@ -188,6 +188,7 @@ void log_io_piece(struct thread_data *td, struct io_u *io_u)
 	ipo->file = io_u->file;
 	ipo->offset = io_u->offset;
 	ipo->len = io_u->buflen;
+	ipo->numberio = io_u->numberio;
 
 	if (io_u_should_trim(td, io_u)) {
 		flist_add_tail(&ipo->trim_list, &td->trim_list);
diff --git a/iolog.h b/iolog.h
index 8fedc19..94e0fc1 100644
--- a/iolog.h
+++ b/iolog.h
@@ -78,6 +78,7 @@ struct io_piece {
 		struct fio_file *file;
 	};
 	unsigned long long offset;
+	unsigned short numberio;
 	unsigned long len;
 	unsigned int flags;
 	enum fio_ddir ddir;
diff --git a/libfio.c b/libfio.c
index c26d6a3..6e290f4 100644
--- a/libfio.c
+++ b/libfio.c
@@ -83,7 +83,6 @@ static void reset_io_counters(struct thread_data *td)
 		td->this_io_blocks[ddir] = 0;
 		td->rate_bytes[ddir] = 0;
 		td->rate_blocks[ddir] = 0;
-		td->io_issues[ddir] = 0;
 	}
 	td->zone_bytes = 0;
 
diff --git a/verify.c b/verify.c
index 9e88d61..70880f4 100644
--- a/verify.c
+++ b/verify.c
@@ -369,6 +369,15 @@ static int verify_io_u_meta(struct verify_header *hdr, struct vcont *vc)
 	if (td->o.verify_pattern_bytes)
 		ret |= verify_io_u_pattern(hdr, vc);
 
+	/*
+	 * For read-only workloads, the program cannot be certain of the
+	 * last numberio written to a block. Checking of numberio will be done
+	 * only for wrokloads that write data.
+	 */
+	if (td_write(td) || td_rw(td))
+		if (vh->numberio != io_u->numberio)
+			ret = EILSEQ;
+
 	if (!ret)
 		return 0;
 
@@ -768,7 +777,7 @@ static void fill_meta(struct verify_header *hdr, struct thread_data *td,
 	vh->time_sec = io_u->start_time.tv_sec;
 	vh->time_usec = io_u->start_time.tv_usec;
 
-	vh->numberio = td->io_issues[DDIR_WRITE];
+	vh->numberio = io_u->numberio;
 
 	vh->offset = io_u->offset + header_num * td->o.verify_interval;
 }
@@ -942,6 +951,8 @@ void populate_verify_io_u(struct thread_data *td, struct io_u *io_u)
 	if (td->o.verify == VERIFY_NULL)
 		return;
 
+	io_u->numberio = td->io_issues[io_u->ddir];
+
 	fill_pattern_headers(td, io_u, 0, 0);
 }
 
@@ -974,6 +985,7 @@ int get_next_verify(struct thread_data *td, struct io_u *io_u)
 
 		io_u->offset = ipo->offset;
 		io_u->buflen = ipo->len;
+		io_u->numberio = ipo->numberio;
 		io_u->file = ipo->file;
 		io_u->flags |= IO_U_F_VER_LIST;
 
-- 
1.7.12.4

[PATCH 2/2] Adds check for rand_seed during verify phase.

[Juan Casse]

Improve data integrity checking and detect stale blocks from previous
runs.

Signed-off-by: Juan Casse <jcasse@chromium.org>
Reviewed-by: Grant Grundler <grundler@chromium.org>
---
 verify.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/verify.c b/verify.c
index 70880f4..62814b9 100644
--- a/verify.c
+++ b/verify.c
@@ -653,18 +653,17 @@ static int verify_header(struct io_u *io_u, struct verify_header *hdr)
 	uint32_t crc;
 
 	if (hdr->magic != FIO_HDR_MAGIC)
-		return 0;
-	if (hdr->len > io_u->buflen) {
-		log_err("fio: verify header exceeds buffer length (%u > %lu)\n", hdr->len, io_u->buflen);
-		return 0;
-	}
+		return 1;
+	if (hdr->len > io_u->buflen)
+		return 2;
+	if (hdr->rand_seed != io_u->rand_seed)
+		return 3;
 
 	crc = fio_crc32c(p, offsetof(struct verify_header, crc32));
 	if (crc == hdr->crc32)
-		return 1;
-
+		return 0;
 	log_err("fio: verify header crc %x, calculated %x\n", hdr->crc32, crc);
-	return 0;
+	return 4;
 }
 
 int verify_io_u(struct thread_data *td, struct io_u *io_u)
@@ -701,13 +700,41 @@ int verify_io_u(struct thread_data *td, struct io_u *io_u)
 			memswp(p, p + td->o.verify_offset, header_size);
 		hdr = p;
 
-		if (!verify_header(io_u, hdr)) {
+		ret = verify_header(io_u, hdr);
+		switch (ret) {
+		case 0:
+			break;
+		case 1:
 			log_err("verify: bad magic header %x, wanted %x at "
 				"file %s offset %llu, length %u\n",
 				hdr->magic, FIO_HDR_MAGIC,
 				io_u->file->file_name,
 				io_u->offset + hdr_num * hdr->len, hdr->len);
 			return EILSEQ;
+			break;
+		case 2:
+			log_err("fio: verify header exceeds buffer length (%u "
+				"> %lu)\n", hdr->len, io_u->buflen);
+			return EILSEQ;
+			break;
+		case 3:
+			log_err("verify: bad header rand_seed %"PRIu64
+				", wanted %"PRIu64" at file %s offset %llu, "
+				"length %u\n",
+				hdr->rand_seed, io_u->rand_seed,
+				io_u->file->file_name,
+				io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
+			break;
+		case 4:
+			return EILSEQ;
+			break;
+		default:
+			log_err("verify: unknown header error at file %s "
+			"offset %llu, length %u\n",
+			io_u->file->file_name,
+			io_u->offset + hdr_num * hdr->len, hdr->len);
+			return EILSEQ;
 		}
 
 		if (td->o.verify != VERIFY_NONE)
-- 
1.7.12.4