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

From: Tomohiro Kusumi <redacted>

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 <redacted>
---
 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

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

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/

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler [off-list ref]:

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

quoted

From: Tomohiro Kusumi <redacted>

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 <redacted>
---
 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.

On 4 September 2017 at 20:48, Tomohiro Kusumi [off-list ref] wrote:

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

quoted

From: Tomohiro Kusumi <redacted>

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.

[...]

quoted

quoted

---
 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/

2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler [off-list ref]:

On 4 September 2017 at 20:48, Tomohiro Kusumi [off-list ref] wrote:

quoted

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

quoted

From: Tomohiro Kusumi <redacted>

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.

[...]

quoted

quoted

quoted

---
 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/

On 09/05/2017 12:14 AM, Tomohiro Kusumi wrote:

2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

On 4 September 2017 at 20:48, Tomohiro Kusumi [off-list ref] wrote:

quoted

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

quoted

From: Tomohiro Kusumi <redacted>

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.

[...]

quoted

quoted

quoted

---
 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

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 [off-list ref]:

On 09/05/2017 12:14 AM, Tomohiro Kusumi wrote:

quoted

2017-09-05 8:59 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

On 4 September 2017 at 20:48, Tomohiro Kusumi [off-list ref] wrote:

quoted

2017-09-04 22:36 GMT+03:00 Sitsofe Wheeler [off-list ref]:

quoted

Hi,
On 4 September 2017 at 16:23,  [off-list ref] wrote:

quoted

From: Tomohiro Kusumi <redacted>

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.

[...]

quoted

quoted

quoted

---
 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

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

On 5 September 2017 at 15:58, Jens Axboe [off-list ref] wrote:

On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:

quoted

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.

quoted

(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/

On 09/05/2017 01:00 PM, Sitsofe Wheeler wrote:

On 5 September 2017 at 15:58, Jens Axboe [off-list ref] wrote:

quoted

On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:

quoted

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).

quoted

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

On Tue, Sep 05, 2017 at 01:45:59PM -0600, Jens Axboe wrote:

On 09/05/2017 01:00 PM, Sitsofe Wheeler wrote:

quoted

On 5 September 2017 at 15:58, Jens Axboe [off-list ref] wrote:

quoted

On 09/05/2017 08:54 AM, Tomohiro Kusumi wrote:

quoted

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).

quoted

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