* [PATCH] don't change direct I/O xfer size during initial layout setup
@ 2017-09-04 15:23 kusumi.tomohiro
2017-09-04 19:36 ` Sitsofe Wheeler
0 siblings, 1 reply; 11+ messages in thread
From: kusumi.tomohiro @ 2017-09-04 15:23 UTC (permalink / raw)
To: axboe, fio; +Cc: Tomohiro Kusumi
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-04 15:23 [PATCH] don't change direct I/O xfer size during initial layout setup kusumi.tomohiro
@ 2017-09-04 19:36 ` Sitsofe Wheeler
2017-09-04 19:48 ` Tomohiro Kusumi
0 siblings, 1 reply; 11+ messages in thread
From: Sitsofe Wheeler @ 2017-09-04 19:36 UTC (permalink / raw)
To: Tomohiro Kusumi; +Cc: Jens Axboe, fio@vger.kernel.org, Tomohiro Kusumi
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-04 19:36 ` Sitsofe Wheeler
@ 2017-09-04 19:48 ` Tomohiro Kusumi
2017-09-05 5:59 ` Sitsofe Wheeler
0 siblings, 1 reply; 11+ messages in thread
From: Tomohiro Kusumi @ 2017-09-04 19:48 UTC (permalink / raw)
To: Sitsofe Wheeler; +Cc: Jens Axboe, fio@vger.kernel.org, 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-04 19:48 ` Tomohiro Kusumi
@ 2017-09-05 5:59 ` Sitsofe Wheeler
2017-09-05 6:14 ` Tomohiro Kusumi
0 siblings, 1 reply; 11+ messages in thread
From: Sitsofe Wheeler @ 2017-09-05 5:59 UTC (permalink / raw)
To: Tomohiro Kusumi; +Cc: Jens Axboe, fio@vger.kernel.org, Tomohiro Kusumi
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 5:59 ` Sitsofe Wheeler
@ 2017-09-05 6:14 ` Tomohiro Kusumi
2017-09-05 14:39 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Tomohiro Kusumi @ 2017-09-05 6:14 UTC (permalink / raw)
To: Sitsofe Wheeler; +Cc: Jens Axboe, fio@vger.kernel.org, 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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 6:14 ` Tomohiro Kusumi
@ 2017-09-05 14:39 ` Jens Axboe
2017-09-05 14:54 ` Tomohiro Kusumi
0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2017-09-05 14:39 UTC (permalink / raw)
To: Tomohiro Kusumi, Sitsofe Wheeler; +Cc: fio@vger.kernel.org, Tomohiro Kusumi
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 14:39 ` Jens Axboe
@ 2017-09-05 14:54 ` Tomohiro Kusumi
2017-09-05 14:58 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Tomohiro Kusumi @ 2017-09-05 14:54 UTC (permalink / raw)
To: Jens Axboe; +Cc: Sitsofe Wheeler, fio@vger.kernel.org, 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 14:54 ` Tomohiro Kusumi
@ 2017-09-05 14:58 ` Jens Axboe
2017-09-05 19:00 ` Sitsofe Wheeler
0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2017-09-05 14:58 UTC (permalink / raw)
To: Tomohiro Kusumi; +Cc: Sitsofe Wheeler, fio@vger.kernel.org, Tomohiro Kusumi
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 14:58 ` Jens Axboe
@ 2017-09-05 19:00 ` Sitsofe Wheeler
2017-09-05 19:45 ` Jens Axboe
0 siblings, 1 reply; 11+ messages in thread
From: Sitsofe Wheeler @ 2017-09-05 19:00 UTC (permalink / raw)
To: Jens Axboe
Cc: Tomohiro Kusumi, fio@vger.kernel.org, Tomohiro Kusumi,
weiping zhang
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/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 19:00 ` Sitsofe Wheeler
@ 2017-09-05 19:45 ` Jens Axboe
2017-09-06 10:13 ` weiping zhang
0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2017-09-05 19:45 UTC (permalink / raw)
To: Sitsofe Wheeler
Cc: Tomohiro Kusumi, fio@vger.kernel.org, Tomohiro Kusumi,
weiping zhang
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] don't change direct I/O xfer size during initial layout setup
2017-09-05 19:45 ` Jens Axboe
@ 2017-09-06 10:13 ` weiping zhang
0 siblings, 0 replies; 11+ messages in thread
From: weiping zhang @ 2017-09-06 10:13 UTC (permalink / raw)
To: Jens Axboe
Cc: Sitsofe Wheeler, Tomohiro Kusumi, fio@vger.kernel.org,
Tomohiro Kusumi
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-09-06 10:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-04 15:23 [PATCH] don't change direct I/O xfer size during initial layout setup kusumi.tomohiro
2017-09-04 19:36 ` Sitsofe Wheeler
2017-09-04 19:48 ` Tomohiro Kusumi
2017-09-05 5:59 ` Sitsofe Wheeler
2017-09-05 6:14 ` Tomohiro Kusumi
2017-09-05 14:39 ` Jens Axboe
2017-09-05 14:54 ` Tomohiro Kusumi
2017-09-05 14:58 ` Jens Axboe
2017-09-05 19:00 ` Sitsofe Wheeler
2017-09-05 19:45 ` Jens Axboe
2017-09-06 10:13 ` weiping zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).