[PATCH 0/2] Fix percentage random workload and add test case

[PATCH 0/2] Fix percentage random workload and add test case

[Ankit Kumar]

Sometimes while running a mix of sequential and random I/O's the
generated sequential offset is outside the I/O region. This usually
happens if the previous I/O is the last block in the region which
results in this sequential I/O offset to be just outside the region.
This results in fio finishing without doing the specified amount of
I/O's.
With this fix fio now generates a random offset within the region.

Added a test case for the same (test_id: 28)
As I see there is a pending patch with test_id: 27
so I decided to skip it to avoid conflict.

This now fixes the issue: https://github.com/axboe/fio/issues/1486

Ankit Kumar (2):
  io_u: fix offset generation for mix of random and sequential workload
  test: add test for mix random and sequential workload

 io_u.c                |  5 ++++-
 t/jobs/t0028-post.fio | 27 +++++++++++++++++++++++++++
 t/jobs/t0028-pre.fio  | 21 +++++++++++++++++++++
 t/run-fio-tests.py    | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 t/jobs/t0028-post.fio
 create mode 100644 t/jobs/t0028-pre.fio

-- 
2.17.1

[PATCH 1/2] io_u: fix offset generation for mix of random and sequential workload

[Ankit Kumar]

Sometimes while running a mix of sequential and random I/O's the
generated sequential offset is outside the I/O region. This usually
happens if the previous I/O is the last block in the region and
thus results in this sequential I/O's offset to be just outside the
region.
With this fix, fio generates a random offset within the I/O region.

This fixes #1486

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 io_u.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/io_u.c b/io_u.c
index 8035f4b7..e49b1b29 100644
--- a/io_u.c
+++ b/io_u.c
@@ -432,8 +432,11 @@ static int get_next_block(struct thread_data *td, struct io_u *io_u,
 				*is_random = false;
 				io_u_set(td, io_u, IO_U_F_BUSY_OK);
 				ret = get_next_seq_offset(td, f, ddir, &offset);
-				if (ret)
+				if (ret || offset >= f->io_size) {
 					ret = get_next_rand_block(td, f, ddir, &b);
+					offset = -1ULL;
+					*is_random = true;
+				}
 			}
 		} else {
 			*is_random = false;
-- 
2.17.1

Re: [PATCH 1/2] io_u: fix offset generation for mix of random and sequential workload

[Vincent Fu]

On 11/18/22 00:14, Ankit Kumar wrote:
 > Sometimes while running a mix of sequential and random I/O's the
 > generated sequential offset is outside the I/O region. This usually
 > happens if the previous I/O is the last block in the region and
 > thus results in this sequential I/O's offset to be just outside the
 > region.
 > With this fix, fio generates a random offset within the I/O region.
 >
 > This fixes #1486
 >
 > Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
 > ---
 >   io_u.c | 5 ++++-
 >   1 file changed, 4 insertions(+), 1 deletion(-)
 >
 > diff --git a/io_u.c b/io_u.c
 > index 8035f4b7..e49b1b29 100644
 > --- a/io_u.c
 > +++ b/io_u.c
 > @@ -432,8 +432,11 @@ static int get_next_block(struct thread_data 
*td, struct io_u *io_u,
 >   				*is_random = false;
 >   				io_u_set(td, io_u, IO_U_F_BUSY_OK);
 >   				ret = get_next_seq_offset(td, f, ddir, &offset);
 > -				if (ret)
 > +				if (ret || offset >= f->io_size) {
 >   					ret = get_next_rand_block(td, f, ddir, &b);
 > +					offset = -1ULL;
 > +					*is_random = true;
 > +				}
 >   			}
 >   		} else {
 >   			*is_random = false;

This patch causes a regression with a workload such as:

fio --name=test --bs=4k --filesize=32k --nrfiles=10 --number_ios=10 
--file_service_type=sequential --percentage_random=50 --rw=randread 
--randrepeat=0 --ioengine=null --debug=io

Only the first file will be accessed when this patch is applied. Bad 
offsets are needed in order to trigger the move to the next file.

Here is a related issue: https://github.com/axboe/fio/issues/1372

Perhaps the right thing to do is to limit the offset >= f->io_size check 
to cases where nrfiles = 1 as in the patch for issue 1372.

Also, it seems to me that if we are intending to generate a sequential 
offset for this case and succeed in doing so, we should just wrap around 
to the beginning of the file instead of choosing a new random offset.

In general we have an overwhelming set of ways to generate offsets and 
little automated testing for offset generation. In the long run we 
should develop a set of tests for offset generation that exercise the 
different options. This will help us detect issues and give us 
confidence that future code changes won't introduce regressions.

Vincent

Re: [PATCH 1/2] io_u: fix offset generation for mix of random and sequential workload

[Ankit Kumar]

[-- Attachment #1: Type: text/plain, Size: 3965 bytes --]

On Mon, Nov 28, 2022 at 04:57:28PM -0500, Vincent Fu wrote:
> On 11/18/22 00:14, Ankit Kumar wrote:
> > Sometimes while running a mix of sequential and random I/O's the
> > generated sequential offset is outside the I/O region. This usually
> > happens if the previous I/O is the last block in the region and
> > thus results in this sequential I/O's offset to be just outside the
> > region.
> > With this fix, fio generates a random offset within the I/O region.
> >
> > This fixes #1486
> >
> > Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
> > ---
> >   io_u.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/io_u.c b/io_u.c
> > index 8035f4b7..e49b1b29 100644
> > --- a/io_u.c
> > +++ b/io_u.c
> > @@ -432,8 +432,11 @@ static int get_next_block(struct thread_data *td,
> struct io_u *io_u,
> >   				*is_random = false;
> >   				io_u_set(td, io_u, IO_U_F_BUSY_OK);
> >   				ret = get_next_seq_offset(td, f, ddir, &offset);
> > -				if (ret)
> > +				if (ret || offset >= f->io_size) {
> >   					ret = get_next_rand_block(td, f, ddir, &b);
> > +					offset = -1ULL;
> > +					*is_random = true;
> > +				}
> >   			}
> >   		} else {
> >   			*is_random = false;
> 
> This patch causes a regression with a workload such as:
> 
> fio --name=test --bs=4k --filesize=32k --nrfiles=10 --number_ios=10
> --file_service_type=sequential --percentage_random=50 --rw=randread
> --randrepeat=0 --ioengine=null --debug=io
> 
> Only the first file will be accessed when this patch is applied. Bad offsets
> are needed in order to trigger the move to the next file.
> 
> Here is a related issue: https://protect2.fireeye.com/v1/url?k=9f1fdb0c-fe94ce3a-9f1e5043-74fe485fffe0-0f0dbb3b9fc8b673&q=1&e=c35fdef7-d5f7-487d-b2e8-9f36779622fe&u=https%3A%2F%2Fgithub.com%2Faxboe%2Ffio%2Fissues%2F1372
> 
> Perhaps the right thing to do is to limit the offset >= f->io_size check to
> cases where nrfiles = 1 as in the patch for issue 1372.
> 
> Also, it seems to me that if we are intending to generate a sequential
> offset for this case and succeed in doing so, we should just wrap around to
> the beginning of the file instead of choosing a new random offset.
> 
> In general we have an overwhelming set of ways to generate offsets and
> little automated testing for offset generation. In the long run we should
> develop a set of tests for offset generation that exercise the different
> options. This will help us detect issues and give us confidence that future
> code changes won't introduce regressions.
> 
> Vincent
> 
Ok thanks for pointing out, adding the nrfiles == 1 check should suffice.

I thought about changing the current sequential offset generation logic but
its used for a lot of things as you pointed out. Modifying it may create
further regression.

One such case is with zone block devices when zone capacity is less than
zone size. Currently if we don't limit size by specifying io_size or
number_ios or runtime, fio will exit early without transferring size bytes
of data as it generates a bad offset i.e. offset outside the I/O region.
This is because of holes or gaps between a zone end and start of next zone.
All the test cases in t/zbd/test-zbd-support takes account of that.

There is another case with gaps ex: with workload such as "rw=read:4K"
where fio will exit early without transferring size bytes of data. I am
not sure whether this is the correct behavior.
This will only happen if file size is bigger than f->offset + f->io_size
i.e. we never reach the end of file, else it will wrap around.

I think to handle all these cases and percentage_random workload, for
multiple files is going to be tricky.

One solution I can think of is to just update the documentation for "size" by
mentioning that runtime can also get reduced if there are holes or gaps
while doing sequential I/O's, or if we have mixture of sequential or random
workload which can generate an out of I/O region offset.

Ankit





[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] io_u: fix offset generation for mix of random and sequential workload

[Vincent Fu]

On 11/30/22 05:50, Ankit Kumar wrote:
> On Mon, Nov 28, 2022 at 04:57:28PM -0500, Vincent Fu wrote:
>> On 11/18/22 00:14, Ankit Kumar wrote:
>>> Sometimes while running a mix of sequential and random I/O's the
>>> generated sequential offset is outside the I/O region. This usually
>>> happens if the previous I/O is the last block in the region and
>>> thus results in this sequential I/O's offset to be just outside the
>>> region.
>>> With this fix, fio generates a random offset within the I/O region.
>>>
>>> This fixes #1486
>>>
>>> Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
>>> ---
>>>    io_u.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/io_u.c b/io_u.c
>>> index 8035f4b7..e49b1b29 100644
>>> --- a/io_u.c
>>> +++ b/io_u.c
>>> @@ -432,8 +432,11 @@ static int get_next_block(struct thread_data *td,
>> struct io_u *io_u,
>>>    				*is_random = false;
>>>    				io_u_set(td, io_u, IO_U_F_BUSY_OK);
>>>    				ret = get_next_seq_offset(td, f, ddir, &offset);
>>> -				if (ret)
>>> +				if (ret || offset >= f->io_size) {
>>>    					ret = get_next_rand_block(td, f, ddir, &b);
>>> +					offset = -1ULL;
>>> +					*is_random = true;
>>> +				}
>>>    			}
>>>    		} else {
>>>    			*is_random = false;
>>
>> This patch causes a regression with a workload such as:
>>
>> fio --name=test --bs=4k --filesize=32k --nrfiles=10 --number_ios=10
>> --file_service_type=sequential --percentage_random=50 --rw=randread
>> --randrepeat=0 --ioengine=null --debug=io
>>
>> Only the first file will be accessed when this patch is applied. Bad offsets
>> are needed in order to trigger the move to the next file.
>>
>> Here is a related issue: https://protect2.fireeye.com/v1/url?k=9f1fdb0c-fe94ce3a-9f1e5043-74fe485fffe0-0f0dbb3b9fc8b673&q=1&e=c35fdef7-d5f7-487d-b2e8-9f36779622fe&u=https%3A%2F%2Fgithub.com%2Faxboe%2Ffio%2Fissues%2F1372
>>
>> Perhaps the right thing to do is to limit the offset >= f->io_size check to
>> cases where nrfiles = 1 as in the patch for issue 1372.
>>
>> Also, it seems to me that if we are intending to generate a sequential
>> offset for this case and succeed in doing so, we should just wrap around to
>> the beginning of the file instead of choosing a new random offset.
>>
>> In general we have an overwhelming set of ways to generate offsets and
>> little automated testing for offset generation. In the long run we should
>> develop a set of tests for offset generation that exercise the different
>> options. This will help us detect issues and give us confidence that future
>> code changes won't introduce regressions.
>>
>> Vincent
>>
> Ok thanks for pointing out, adding the nrfiles == 1 check should suffice.
> 
> I thought about changing the current sequential offset generation logic but
> its used for a lot of things as you pointed out. Modifying it may create
> further regression.
> 
> One such case is with zone block devices when zone capacity is less than
> zone size. Currently if we don't limit size by specifying io_size or
> number_ios or runtime, fio will exit early without transferring size bytes
> of data as it generates a bad offset i.e. offset outside the I/O region.
> This is because of holes or gaps between a zone end and start of next zone.
> All the test cases in t/zbd/test-zbd-support takes account of that.
> 
> There is another case with gaps ex: with workload such as "rw=read:4K"
> where fio will exit early without transferring size bytes of data. I am
> not sure whether this is the correct behavior.
> This will only happen if file size is bigger than f->offset + f->io_size
> i.e. we never reach the end of file, else it will wrap around.
> 
> I think to handle all these cases and percentage_random workload, for
> multiple files is going to be tricky.
> 
> One solution I can think of is to just update the documentation for "size" by
> mentioning that runtime can also get reduced if there are holes or gaps
> while doing sequential I/O's, or if we have mixture of sequential or random
> workload which can generate an out of I/O region offset.
> 
> Ankit
> 

Ok. It seems like what is happening in 
https://github.com/axboe/fio/issues/1486 is that fio is doing sequential 
I/O, reaches the end of the device, and then decides that the job is 
completed. This is arguably correct behavior. I agree that the right 
approach is to improve the documentation because changing the code to 
catch this special case may be risky. I think the io_size work around 
you provided in the ticket is a reasonable resolution.

Vincent

[PATCH 2/2] test: add test for mix random and sequential workload

[Ankit Kumar]

Add a test case to confirm that with mix sequential and random workload
i.e. with fio option percentage_random set to 0 < percentage_random < 100
we are actually writing the specified size.

The test includes pre and post job files, where the former lays out 3
I/O files each of size 4MiB to be used by latter to do random write
with 1MiB size per job. This is intended to cover such cases where
fio generates offset just at the end of I/O region resulting in the
next sequential I/O to be outside the region, when the size is less than
actual filesize.

This covers #1486

Signed-off-by: Ankit Kumar <ankit.kumar@samsung.com>
---
 t/jobs/t0028-post.fio | 27 +++++++++++++++++++++++++++
 t/jobs/t0028-pre.fio  | 21 +++++++++++++++++++++
 t/run-fio-tests.py    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 82 insertions(+)
 create mode 100644 t/jobs/t0028-post.fio
 create mode 100644 t/jobs/t0028-pre.fio

diff --git a/t/jobs/t0028-post.fio b/t/jobs/t0028-post.fio
new file mode 100644
index 00000000..d5ad88ca
--- /dev/null
+++ b/t/jobs/t0028-post.fio
@@ -0,0 +1,27 @@
+# specify mixture of sequential and random workloads
+[global]
+ioengine=libaio
+size=1M
+rw=randwrite
+
+# Expected result: 	1M data written
+# Buggy result: 	Less than 1M data written
+[basic]
+bs=4K
+percentage_random=50
+
+# Expected result: 	1M data written
+# Buggy result: 	Less than 1M data written
+[rand_skewed]
+bs=4K
+randseed=47827
+percentage_random=90
+iodepth=32
+
+# Expected result: 	1M data written
+# Buggy result: 	Less than 1M data written
+[bsrange]
+bsrange=512-4K
+randseed=7847
+percentage_random=50
+iodepth=32
diff --git a/t/jobs/t0028-pre.fio b/t/jobs/t0028-pre.fio
new file mode 100644
index 00000000..dafc4b06
--- /dev/null
+++ b/t/jobs/t0028-pre.fio
@@ -0,0 +1,21 @@
+# specify mixture of sequential and random workloads
+[global]
+ioengine=libaio
+size=4M
+rw=randwrite
+
+[basic]
+bs=4K
+percentage_random=50
+
+[rand_skewed]
+bs=4K
+randseed=47827
+percentage_random=90
+iodepth=32
+
+[bsrange]
+bsrange=512-4K
+randseed=7847
+percentage_random=50
+iodepth=32
diff --git a/t/run-fio-tests.py b/t/run-fio-tests.py
index e5b307ac..500ca841 100755
--- a/t/run-fio-tests.py
+++ b/t/run-fio-tests.py
@@ -800,6 +800,30 @@ class FioJobTest_t0025(FioJobTest):
             self.passed = False
 
 
+class FioJobTest_t0028(FioJobTest):
+    """Test consists for test job t0028-post.fio
+    Confirm for all jobs that write['io_kbytes'] == 1024"""
+
+    def check_result(self):
+        super(FioJobTest_t0028, self).check_result()
+
+        if not self.passed:
+            return
+
+        if self.json_data['jobs'][0]['write']['io_kbytes'] != 1024:
+            self.passed = False
+            self.failure_reason = "{basic} data written != 1024KiB,".format(self.failure_reason)
+
+        if self.json_data['jobs'][1]['write']['io_kbytes'] != 1024:
+            self.passed = False
+            self.failure_reason = "{rand_skewed} data written != 1024KiB,".format(self.failure_reason)
+
+        kbytes = self.json_data['jobs'][2]['write']['io_kbytes']
+        if kbytes < 1021 or kbytes > 1027:
+            self.passed = False
+            self.failure_reason = "{bsrange} data written != 1024KiB,".format(self.failure_reason)
+
+
 class FioJobTest_iops_rate(FioJobTest):
     """Test consists of fio test job t0009
     Confirm that job0 iops == 1000
@@ -1214,6 +1238,16 @@ TEST_LIST = [
         'pre_success':      None,
         'requirements':     [Requirements.not_windows],
     },
+    {
+        'test_id':          28,
+        'test_class':       FioJobTest_t0028,
+        'job':              't0028-post.fio',
+        'success':          SUCCESS_DEFAULT,
+        'pre_job':          't0028-pre.fio',
+        'pre_success':      None,
+        'output_format':    'json',
+        'requirements':     [Requirements.linux, Requirements.libaio],
+    },
     {
         'test_id':          1000,
         'test_class':       FioExeTest,
-- 
2.17.1