[PATCH] don't change direct I/O xfer size during initial layout setup

[PATCH] don't change direct I/O xfer size during initial layout setup

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

8c43ba62('filesetup: align layout buffer') and
6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
need to keep the valid transfer size throughout the entire writes.

The write(2) size may be truncated on the last write and break the
dio requirement. This results in td_verror() in the output.

--
 # mount | grep " on / "
 /dev/mapper/fedora-root on / type xfs (rw,relatime,attr2,inode64,noquota)
 # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MiB --unlink=1 --direct=1 | grep "err="
 xxx: (groupid=0, jobs=1): err= 0: pid=3526: Mon Sep  4 18:03:57 2017
 # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MB --unlink=1 --direct=1 | grep "err="
 fio: pid=3538, err=22/file:filesetup.c:238, func=write, error=Invalid argument
 xxx: (groupid=0, jobs=1): err=22 (file:filesetup.c:238, func=write, error=Invalid argument): pid=3538: Mon Sep  4 18:04:04 2017

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 filesetup.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/filesetup.c b/filesetup.c
index 5e8ea35..42d95db 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
 	unsigned long long left;
 	unsigned int bs, alloc_size = 0;
 	char *b = NULL;
+	bool done = false;
 
 	if (read_only) {
 		log_err("fio: refusing extend of file due to read-only\n");
@@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
 		goto err;
 	}
 
-	while (left && !td->terminate) {
+	while (!done && !td->terminate) {
 		ssize_t r;
 
-		if (bs > left)
-			bs = left;
+		/* If bs >= left this is the last write */
+		if (bs > left) {
+			done = true;
+			if (!td->o.odirect)
+				bs = left;
+		}
 
 		fill_io_buffer(td, b, bs, bs);
 
@@ -223,6 +228,8 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
 
 		if (r > 0) {
 			left -= r;
+			if (!left)
+				done = true;
 			continue;
 		} else {
 			if (r < 0) {
-- 
2.9.5

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Sitsofe Wheeler]

Hi,
On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>
> 8c43ba62('filesetup: align layout buffer') and
> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
> need to keep the valid transfer size throughout the entire writes.
>
> The write(2) size may be truncated on the last write and break the
> dio requirement. This results in td_verror() in the output.
>
> --
>  # mount | grep " on / "
>  /dev/mapper/fedora-root on / type xfs (rw,relatime,attr2,inode64,noquota)
>  # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MiB --unlink=1 --direct=1 | grep "err="
>  xxx: (groupid=0, jobs=1): err= 0: pid=3526: Mon Sep  4 18:03:57 2017
>  # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MB --unlink=1 --direct=1 | grep "err="
>  fio: pid=3538, err=22/file:filesetup.c:238, func=write, error=Invalid argument
>  xxx: (groupid=0, jobs=1): err=22 (file:filesetup.c:238, func=write, error=Invalid argument): pid=3538: Mon Sep  4 18:04:04 2017
>
> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
> ---
>  filesetup.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/filesetup.c b/filesetup.c
> index 5e8ea35..42d95db 100644
> --- a/filesetup.c
> +++ b/filesetup.c
> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>         unsigned long long left;
>         unsigned int bs, alloc_size = 0;
>         char *b = NULL;
> +       bool done = false;
>
>         if (read_only) {
>                 log_err("fio: refusing extend of file due to read-only\n");
> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>                 goto err;
>         }
>
> -       while (left && !td->terminate) {
> +       while (!done && !td->terminate) {
>                 ssize_t r;
>
> -               if (bs > left)
> -                       bs = left;
> +               /* If bs >= left this is the last write */
> +               if (bs > left) {
> +                       done = true;
> +                       if (!td->o.odirect)
> +                               bs = left;
> +               }
>
>                 fill_io_buffer(td, b, bs, bs);
>

Hmm. Won't this result in a file that is bigger than requested when
direct=1 and the maximum bs isn't an exact multiple of the extended
filesize? Should it start trying the minimum block size at this stage
with direct=1? If that goes on to not fit wouldn't it be better to do
less given that the main fio job won't be able to fill the gap either?

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Tomohiro Kusumi]

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
> Hi,
> On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>
>> 8c43ba62('filesetup: align layout buffer') and
>> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
>> need to keep the valid transfer size throughout the entire writes.
>>
>> The write(2) size may be truncated on the last write and break the
>> dio requirement. This results in td_verror() in the output.
>>
>> --
>>  # mount | grep " on / "
>>  /dev/mapper/fedora-root on / type xfs (rw,relatime,attr2,inode64,noquota)
>>  # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MiB --unlink=1 --direct=1 | grep "err="
>>  xxx: (groupid=0, jobs=1): err= 0: pid=3526: Mon Sep  4 18:03:57 2017
>>  # ./fio --name=xxx --kb_base=1000 --ioengine=sync --rw=read --bs=4KiB --size=10MB --unlink=1 --direct=1 | grep "err="
>>  fio: pid=3538, err=22/file:filesetup.c:238, func=write, error=Invalid argument
>>  xxx: (groupid=0, jobs=1): err=22 (file:filesetup.c:238, func=write, error=Invalid argument): pid=3538: Mon Sep  4 18:04:04 2017
>>
>> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
>> ---
>>  filesetup.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/filesetup.c b/filesetup.c
>> index 5e8ea35..42d95db 100644
>> --- a/filesetup.c
>> +++ b/filesetup.c
>> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>         unsigned long long left;
>>         unsigned int bs, alloc_size = 0;
>>         char *b = NULL;
>> +       bool done = false;
>>
>>         if (read_only) {
>>                 log_err("fio: refusing extend of file due to read-only\n");
>> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>                 goto err;
>>         }
>>
>> -       while (left && !td->terminate) {
>> +       while (!done && !td->terminate) {
>>                 ssize_t r;
>>
>> -               if (bs > left)
>> -                       bs = left;
>> +               /* If bs >= left this is the last write */
>> +               if (bs > left) {
>> +                       done = true;
>> +                       if (!td->o.odirect)
>> +                               bs = left;
>> +               }
>>
>>                 fill_io_buffer(td, b, bs, bs);
>>
>
> Hmm. Won't this result in a file that is bigger than requested when
> direct=1 and the maximum bs isn't an exact multiple of the extended
> filesize? Should it start trying the minimum block size at this stage
> with direct=1? If that goes on to not fit wouldn't it be better to do
> less given that the main fio job won't be able to fill the gap either?
>
> --
> Sitsofe | http://sucs.org/~sits/

The td's are also under the dio requirements.
If you really want the exact size, you can truncate(2) it.

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Sitsofe Wheeler]

On 4 September 2017 at 20:48, Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
> 2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>> Hi,
>> On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>
>>> 8c43ba62('filesetup: align layout buffer') and
>>> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
>>> need to keep the valid transfer size throughout the entire writes.
>>>
>>> The write(2) size may be truncated on the last write and break the
>>> dio requirement. This results in td_verror() in the output.
>>>
[...]
>>> ---
>>>  filesetup.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/filesetup.c b/filesetup.c
>>> index 5e8ea35..42d95db 100644
>>> --- a/filesetup.c
>>> +++ b/filesetup.c
>>> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>         unsigned long long left;
>>>         unsigned int bs, alloc_size = 0;
>>>         char *b = NULL;
>>> +       bool done = false;
>>>
>>>         if (read_only) {
>>>                 log_err("fio: refusing extend of file due to read-only\n");
>>> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>                 goto err;
>>>         }
>>>
>>> -       while (left && !td->terminate) {
>>> +       while (!done && !td->terminate) {
>>>                 ssize_t r;
>>>
>>> -               if (bs > left)
>>> -                       bs = left;
>>> +               /* If bs >= left this is the last write */
>>> +               if (bs > left) {
>>> +                       done = true;
>>> +                       if (!td->o.odirect)
>>> +                               bs = left;
>>> +               }
>>>
>>>                 fill_io_buffer(td, b, bs, bs);
>>>
>>
>> Hmm. Won't this result in a file that is bigger than requested when
>> direct=1 and the maximum bs isn't an exact multiple of the extended
>> filesize? Should it start trying the minimum block size at this stage
>> with direct=1? If that goes on to not fit wouldn't it be better to do
>> less given that the main fio job won't be able to fill the gap either?
>
> The td's are also under the dio requirements.
> If you really want the exact size, you can truncate(2) it.

Technically didn't fio already truncate the file to the correct size
prior to starting direct layout over on
https://github.com/axboe/fio/blob/d33db728d79386d544be93c24f4e3383f2a47143/filesetup.c#L191
?

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Tomohiro Kusumi]

2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
> On 4 September 2017 at 20:48, Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
>> 2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>> Hi,
>>> On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>
>>>> 8c43ba62('filesetup: align layout buffer') and
>>>> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
>>>> need to keep the valid transfer size throughout the entire writes.
>>>>
>>>> The write(2) size may be truncated on the last write and break the
>>>> dio requirement. This results in td_verror() in the output.
>>>>
> [...]
>>>> ---
>>>>  filesetup.c | 13 ++++++++++---
>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/filesetup.c b/filesetup.c
>>>> index 5e8ea35..42d95db 100644
>>>> --- a/filesetup.c
>>>> +++ b/filesetup.c
>>>> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>         unsigned long long left;
>>>>         unsigned int bs, alloc_size = 0;
>>>>         char *b = NULL;
>>>> +       bool done = false;
>>>>
>>>>         if (read_only) {
>>>>                 log_err("fio: refusing extend of file due to read-only\n");
>>>> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>                 goto err;
>>>>         }
>>>>
>>>> -       while (left && !td->terminate) {
>>>> +       while (!done && !td->terminate) {
>>>>                 ssize_t r;
>>>>
>>>> -               if (bs > left)
>>>> -                       bs = left;
>>>> +               /* If bs >= left this is the last write */
>>>> +               if (bs > left) {
>>>> +                       done = true;
>>>> +                       if (!td->o.odirect)
>>>> +                               bs = left;
>>>> +               }
>>>>
>>>>                 fill_io_buffer(td, b, bs, bs);
>>>>
>>>
>>> Hmm. Won't this result in a file that is bigger than requested when
>>> direct=1 and the maximum bs isn't an exact multiple of the extended
>>> filesize? Should it start trying the minimum block size at this stage
>>> with direct=1? If that goes on to not fit wouldn't it be better to do
>>> less given that the main fio job won't be able to fill the gap either?
>>
>> The td's are also under the dio requirements.
>> If you really want the exact size, you can truncate(2) it.
>
> Technically didn't fio already truncate the file to the correct size
> prior to starting direct layout over on
> https://github.com/axboe/fio/blob/d33db728d79386d544be93c24f4e3383f2a47143/filesetup.c#L191
> ?

You were talking about the size *after* the write....

>
> --
> Sitsofe | http://sucs.org/~sits/

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Jens Axboe]

On 09/05/2017 12:14 AM, Tomohiro Kusumi wrote:
> 2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>> On 4 September 2017 at 20:48, Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
>>> 2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>> Hi,
>>>> On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>
>>>>> 8c43ba62('filesetup: align layout buffer') and
>>>>> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
>>>>> need to keep the valid transfer size throughout the entire writes.
>>>>>
>>>>> The write(2) size may be truncated on the last write and break the
>>>>> dio requirement. This results in td_verror() in the output.
>>>>>
>> [...]
>>>>> ---
>>>>>  filesetup.c | 13 ++++++++++---
>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/filesetup.c b/filesetup.c
>>>>> index 5e8ea35..42d95db 100644
>>>>> --- a/filesetup.c
>>>>> +++ b/filesetup.c
>>>>> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>>         unsigned long long left;
>>>>>         unsigned int bs, alloc_size = 0;
>>>>>         char *b = NULL;
>>>>> +       bool done = false;
>>>>>
>>>>>         if (read_only) {
>>>>>                 log_err("fio: refusing extend of file due to read-only\n");
>>>>> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>>                 goto err;
>>>>>         }
>>>>>
>>>>> -       while (left && !td->terminate) {
>>>>> +       while (!done && !td->terminate) {
>>>>>                 ssize_t r;
>>>>>
>>>>> -               if (bs > left)
>>>>> -                       bs = left;
>>>>> +               /* If bs >= left this is the last write */
>>>>> +               if (bs > left) {
>>>>> +                       done = true;
>>>>> +                       if (!td->o.odirect)
>>>>> +                               bs = left;
>>>>> +               }
>>>>>
>>>>>                 fill_io_buffer(td, b, bs, bs);
>>>>>
>>>>
>>>> Hmm. Won't this result in a file that is bigger than requested when
>>>> direct=1 and the maximum bs isn't an exact multiple of the extended
>>>> filesize? Should it start trying the minimum block size at this stage
>>>> with direct=1? If that goes on to not fit wouldn't it be better to do
>>>> less given that the main fio job won't be able to fill the gap either?
>>>
>>> The td's are also under the dio requirements.
>>> If you really want the exact size, you can truncate(2) it.
>>
>> Technically didn't fio already truncate the file to the correct size
>> prior to starting direct layout over on
>> https://github.com/axboe/fio/blob/d33db728d79386d544be93c24f4e3383f2a47143/filesetup.c#L191
>> ?
> 
> You were talking about the size *after* the write....

How many fixups for a simple "use O_DIRECT for layout if the job file
has direct=1 set" have we done now? I'm going to revert the whole thing,
and if someone can convince me that a simple clean patch that gets all
of it right exists, then please do send it. But until that happens,
we're going back to the old behavior of just using buffered IO to lay it
out.

-- 
Jens Axboe

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Tomohiro Kusumi]

I think I've made three fixes over the original two commits.

1. non O_DIRECT dio support -> committed -> now reverted
2. segfault on fio_memfree() -> committed -> now reverted
3. this one

As far as I've seen this is it.

(I think pre read can do the same as this function, to avoid
irrelevant errors, though pre read on dio is contradictory)


2017-09-05 17:39 GMT+03:00 Jens Axboe <axboe@kernel.dk>:
> On 09/05/2017 12:14 AM, Tomohiro Kusumi wrote:
>> 2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>> On 4 September 2017 at 20:48, Tomohiro Kusumi <kusumi.tomohiro@gmail.com> wrote:
>>>> 2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler <sitsofe@gmail.com>:
>>>>> Hi,
>>>>> On 4 September 2017 at 16:23,  <kusumi.tomohiro@gmail.com> wrote:
>>>>>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>>>>>
>>>>>> 8c43ba62('filesetup: align layout buffer') and
>>>>>> 6e344dc3('filesetup: keep OS_O_DIRECT flag when pre-allocating file')
>>>>>> need to keep the valid transfer size throughout the entire writes.
>>>>>>
>>>>>> The write(2) size may be truncated on the last write and break the
>>>>>> dio requirement. This results in td_verror() in the output.
>>>>>>
>>> [...]
>>>>>> ---
>>>>>>  filesetup.c | 13 ++++++++++---
>>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/filesetup.c b/filesetup.c
>>>>>> index 5e8ea35..42d95db 100644
>>>>>> --- a/filesetup.c
>>>>>> +++ b/filesetup.c
>>>>>> @@ -112,6 +112,7 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>>>         unsigned long long left;
>>>>>>         unsigned int bs, alloc_size = 0;
>>>>>>         char *b = NULL;
>>>>>> +       bool done = false;
>>>>>>
>>>>>>         if (read_only) {
>>>>>>                 log_err("fio: refusing extend of file due to read-only\n");
>>>>>> @@ -211,11 +212,15 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
>>>>>>                 goto err;
>>>>>>         }
>>>>>>
>>>>>> -       while (left && !td->terminate) {
>>>>>> +       while (!done && !td->terminate) {
>>>>>>                 ssize_t r;
>>>>>>
>>>>>> -               if (bs > left)
>>>>>> -                       bs = left;
>>>>>> +               /* If bs >= left this is the last write */
>>>>>> +               if (bs > left) {
>>>>>> +                       done = true;
>>>>>> +                       if (!td->o.odirect)
>>>>>> +                               bs = left;
>>>>>> +               }
>>>>>>
>>>>>>                 fill_io_buffer(td, b, bs, bs);
>>>>>>
>>>>>
>>>>> Hmm. Won't this result in a file that is bigger than requested when
>>>>> direct=1 and the maximum bs isn't an exact multiple of the extended
>>>>> filesize? Should it start trying the minimum block size at this stage
>>>>> with direct=1? If that goes on to not fit wouldn't it be better to do
>>>>> less given that the main fio job won't be able to fill the gap either?
>>>>
>>>> The td's are also under the dio requirements.
>>>> If you really want the exact size, you can truncate(2) it.
>>>
>>> Technically didn't fio already truncate the file to the correct size
>>> prior to starting direct layout over on
>>> https://github.com/axboe/fio/blob/d33db728d79386d544be93c24f4e3383f2a47143/filesetup.c#L191
>>> ?
>>
>> You were talking about the size *after* the write....
>
> How many fixups for a simple "use O_DIRECT for layout if the job file
> has direct=1 set" have we done now? I'm going to revert the whole thing,
> and if someone can convince me that a simple clean patch that gets all
> of it right exists, then please do send it. But until that happens,
> we're going back to the old behavior of just using buffered IO to lay it
> out.
>
> --
> Jens Axboe
>

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Jens Axboe]

On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:
> I think I've made three fixes over the original two commits.
> 
> 1. non O_DIRECT dio support -> committed -> now reverted
> 2. segfault on fio_memfree() -> committed -> now reverted
> 3. this one
> 
> As far as I've seen this is it.

Right, just checked (and reverted) and it's 5 changes all in
all for the change, not including the parent to this email,
which would have made it 6 in all. The original change from Weiping
clearly wasn't well tested or thought through, so it's better
to just kill it all and do it cleanly from scratch instead.

That said, I'm not even convinced we need this change. Logically
it makes sense, but there's really nothing wrong with doing
a buffered layout + cache kill as we have been doing since
the dawn of time in fio.

> (I think pre read can do the same as this function, to avoid
> irrelevant errors, though pre read on dio is contradictory)

Yeah, pre-read with O_DIRECT would be a waste of time.

-- 
Jens Axboe

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Sitsofe Wheeler]

On 5 September 2017 at 15:58, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:
>> I think I've made three fixes over the original two commits.
>>
>> 1. non O_DIRECT dio support -> committed -> now reverted
>> 2. segfault on fio_memfree() -> committed -> now reverted
>> 3. this one
>>
>> As far as I've seen this is it.
>
> Right, just checked (and reverted) and it's 5 changes all in
> all for the change, not including the parent to this email,
> which would have made it 6 in all. The original change from Weiping
> clearly wasn't well tested or thought through, so it's better
> to just kill it all and do it cleanly from scratch instead.

Unfortunately it's been a problem magnetic with stacks of extra
conditions turning up and corner cases to stumble over (see
https://github.com/sitsofe/fio/commit/8fed4ccc87e39144643b1e32caddd882e6197e58
for yet another case that was going to need fixing).

> That said, I'm not even convinced we need this change. Logically
> it makes sense, but there's really nothing wrong with doing
> a buffered layout + cache kill as we have been doing since
> the dawn of time in fio.

One of the benefits was that it stopped

rm -f /tmp/fiofile; ./fio --loops=10 --filename /tmp/fiofile --bs=4k \
 --size=1M --direct=1 --name=go

reporting cached speeds on macOS (a platform where invalidation
doesn't work). The sad thing is that the change wound up slowing
layout right down because the I/O is potentially done in such small
sizes. With the problems it attracted it need a re-think if it's to
avoid attracting a never ending stream of workarounds.

>> (I think pre read can do the same as this function, to avoid
>> irrelevant errors, though pre read on dio is contradictory)
>
> Yeah, pre-read with O_DIRECT would be a waste of time.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[Jens Axboe]

On 09/05/2017 01:00 PM, Sitsofe Wheeler wrote:
> On 5 September 2017 at 15:58, Jens Axboe <axboe@kernel.dk> wrote:
>> On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:
>>> I think I've made three fixes over the original two commits.
>>>
>>> 1. non O_DIRECT dio support -> committed -> now reverted
>>> 2. segfault on fio_memfree() -> committed -> now reverted
>>> 3. this one
>>>
>>> As far as I've seen this is it.
>>
>> Right, just checked (and reverted) and it's 5 changes all in
>> all for the change, not including the parent to this email,
>> which would have made it 6 in all. The original change from Weiping
>> clearly wasn't well tested or thought through, so it's better
>> to just kill it all and do it cleanly from scratch instead.
> 
> Unfortunately it's been a problem magnetic with stacks of extra
> conditions turning up and corner cases to stumble over (see
> https://github.com/sitsofe/fio/commit/8fed4ccc87e39144643b1e32caddd882e6197e58
> for yet another case that was going to need fixing).
> 
>> That said, I'm not even convinced we need this change. Logically
>> it makes sense, but there's really nothing wrong with doing
>> a buffered layout + cache kill as we have been doing since
>> the dawn of time in fio.
> 
> One of the benefits was that it stopped
> 
> rm -f /tmp/fiofile; ./fio --loops=10 --filename /tmp/fiofile --bs=4k \
>  --size=1M --direct=1 --name=go
> 
> reporting cached speeds on macOS (a platform where invalidation
> doesn't work). The sad thing is that the change wound up slowing
> layout right down because the I/O is potentially done in such small
> sizes. With the problems it attracted it need a re-think if it's to
> avoid attracting a never ending stream of workarounds.

Slowing down layout for everyone is a much larger issue (and a
regression), whereas the lack of cache invalidation on a 2nd tier
platform is much less interesting. Of the two, I know what I'd pick.

It'd be nice if we can get everything working nicely. Honestly, it's not
rocket science (at all) to use O_DIRECT for layouts. The performance
issue is the biggest hurdle, the only thing we can do there is use a
larger block size and hope it closes enough of the gap.

-- 
Jens Axboe

Re: [PATCH] don't change direct I/O xfer size during initial layout setup

[weiping zhang]

On Tue, Sep 05, 2017 at 01:45:59PM -0600, Jens Axboe wrote:
> On 09/05/2017 01:00 PM, Sitsofe Wheeler wrote:
> > On 5 September 2017 at 15:58, Jens Axboe <axboe@kernel.dk> wrote:
> >> On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:
> >>> I think I've made three fixes over the original two commits.
> >>>
> >>> 1. non O_DIRECT dio support -> committed -> now reverted
> >>> 2. segfault on fio_memfree() -> committed -> now reverted
> >>> 3. this one
> >>>
> >>> As far as I've seen this is it.
> >>
> >> Right, just checked (and reverted) and it's 5 changes all in
> >> all for the change, not including the parent to this email,
> >> which would have made it 6 in all. The original change from Weiping
> >> clearly wasn't well tested or thought through, so it's better
> >> to just kill it all and do it cleanly from scratch instead.
> > 
> > Unfortunately it's been a problem magnetic with stacks of extra
> > conditions turning up and corner cases to stumble over (see
> > https://github.com/sitsofe/fio/commit/8fed4ccc87e39144643b1e32caddd882e6197e58
> > for yet another case that was going to need fixing).
> > 
> >> That said, I'm not even convinced we need this change. Logically
> >> it makes sense, but there's really nothing wrong with doing
> >> a buffered layout + cache kill as we have been doing since
> >> the dawn of time in fio.
> > 
> > One of the benefits was that it stopped
> > 
> > rm -f /tmp/fiofile; ./fio --loops=10 --filename /tmp/fiofile --bs=4k \
> >  --size=1M --direct=1 --name=go
> > 
> > reporting cached speeds on macOS (a platform where invalidation
> > doesn't work). The sad thing is that the change wound up slowing
> > layout right down because the I/O is potentially done in such small
> > sizes. With the problems it attracted it need a re-think if it's to
> > avoid attracting a never ending stream of workarounds.
> 
> Slowing down layout for everyone is a much larger issue (and a
> regression), whereas the lack of cache invalidation on a 2nd tier
> platform is much less interesting. Of the two, I know what I'd pick.
> 
> It'd be nice if we can get everything working nicely. Honestly, it's not
> rocket science (at all) to use O_DIRECT for layouts. The performance
> issue is the biggest hurdle, the only thing we can do there is use a
> larger block size and hope it closes enough of the gap.
> 

Hi Jens,

How about add a slight check, open a temp file with O_DIRECT but
pre-allocate using buffer io, like following:


diff --git a/filesetup.c b/filesetup.c
index b51ab35..7b86938 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -107,7 +107,7 @@ static void fallocate_file(struct thread_data *td, struct fio_file *f)
  */
 static int extend_file(struct thread_data *td, struct fio_file *f)
 {
-	int new_layout = 0, unlink_file = 0, flags;
+	int new_layout = 0, unlink_file = 0, flags, tmp_fd;
 	unsigned long long left;
 	unsigned int bs;
 	char *b = NULL;
@@ -152,6 +152,16 @@ static int extend_file(struct thread_data *td, struct fio_file *f)
 #endif
 
 	dprint(FD_FILE, "open file %s, flags %x\n", f->file_name, flags);
+
+	if (td->o.odirect) {
+		tmp_fd = open(f->file_name, flags | OS_O_DIRECT, 0644);
+		if (tmp_fd < 0 && errno == EINVAL)
+			log_err("fio: seems like filesystem does not support " \
+				"direct=1/buffered=0\n");
+		if (tmp_fd < 0)
+			return 1;
+		close(tmp_fd);
+	}
+
 	f->fd = open(f->file_name, flags, 0644);
 	if (f->fd < 0) {
 		int err = errno;

Thanks