[PATCH v2 0/7]

[PATCH v2 0/7]

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

configure: Use x86 instead of i386 for $cpu for IA32
  Added "This change should have been a part of e12f4ede in 2016.".

Acquire file ->lock while the lock itself is being copied
  Rewrote the locking using (un)lock_file().

Tomohiro Kusumi (7):
  configure: Drop default CONFIG_LITTLE_ENDIAN for Cygwin
  configure: Use x86 instead of i386 for $cpu for IA32
  Drop conditional declaration of disk_list
  Always set ->real_file_size to -1 when failed to get file size
  Add missing "rand"/"trimwrite" strings to corresponding ddir slots
  Acquire file ->lock while the lock itself is being copied
  Use 0 instead of DDIR_READ to iterate from 0 to DDIR_RWDIR_CNT

 configure        |  3 +--
 diskutil.c       |  2 --
 engines/falloc.c |  2 +-
 eta.c            |  6 +++---
 file.h           |  2 +-
 filesetup.c      | 18 +++++++++++-------
 io_ddir.h        |  4 ++--
 io_u.c           |  6 +++---
 libfio.c         |  5 -----
 stat.c           |  4 ++--
 steadystate.c    |  2 +-
 11 files changed, 25 insertions(+), 29 deletions(-)

-- 
2.9.3

[PATCH v2 1/7] configure: Drop default CONFIG_LITTLE_ENDIAN for Cygwin

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

It's a bit strange that Cygwin is assumed to be le by default when
no other platforms have such assumption even if they only target
a single or certain arch (e.g. non-Android Linux often runs on x86,
DragonFlyBSD only runs on x86, and so on..).

Endianness will be detected by pointer arithmetic anyway, and it's
better to rely on it to handle any sort of newly supported archs
in the future.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 configure | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configure b/configure
index 4f17cf8..cba81af 100755
--- a/configure
+++ b/configure
@@ -306,7 +306,6 @@ CYGWIN*)
       fi
     fi
   fi
-  output_sym "CONFIG_LITTLE_ENDIAN"
   if test ! -z "$build_32bit_win" && test "$build_32bit_win" = "yes"; then
     output_sym "CONFIG_32BIT"
   else
-- 
2.9.3

[PATCH v2 2/7] configure: Use x86 instead of i386 for $cpu for IA32

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

fio internally uses "x86" for any sort of IA32, so use x86 for $cpu
as well which gets printed on ./configure.

This change should have been a part of e12f4ede in 2016.

 # grep i386 . -rI
 ./arch/arch.h:#if defined(__i386__)
 ./configure:elif check_define __i386__ ; then
 ./configure:  cpu="i386"
 ./configure:  i386|i486|i586|i686|i86pc|BePC)
 ./configure:    cpu="i386"
 ./crc/sha1.c:#if defined(__i386__) || defined(__x86_64__)
 ./crc/xxhash.c:#if defined(__ARM_FEATURE_UNALIGNED) || defined(__i386) || defined(_M_IX86) || defined(__x86_64__) || defined(_M_X64)

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index cba81af..3f5386f 100755
--- a/configure
+++ b/configure
@@ -377,7 +377,7 @@ case "$cpu" in
     cpu="$cpu"
   ;;
   i386|i486|i586|i686|i86pc|BePC)
-    cpu="i386"
+    cpu="x86"
   ;;
   x86_64|amd64)
     cpu="x86_64"
-- 
2.9.3

[PATCH v2 3/7] Drop conditional declaration of disk_list

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

disk_list is used unconditionally whether it's Linux or not,
so leave the one in libfio.c, and remove the one in diskutil.c.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 diskutil.c | 2 --
 libfio.c   | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/diskutil.c b/diskutil.c
index c3bcec9..dca3748 100644
--- a/diskutil.c
+++ b/diskutil.c
@@ -18,8 +18,6 @@ static struct disk_util *last_du;
 
 static struct fio_mutex *disk_util_mutex;
 
-FLIST_HEAD(disk_list);
-
 static struct disk_util *__init_per_file_disk_util(struct thread_data *td,
 		int majdev, int mindev, char *path);
 
diff --git a/libfio.c b/libfio.c
index 7e0d32c..4b53c92 100644
--- a/libfio.c
+++ b/libfio.c
@@ -36,12 +36,7 @@
 #include "helper_thread.h"
 #include "filehash.h"
 
-/*
- * Just expose an empty list, if the OS does not support disk util stats
- */
-#ifndef FIO_HAVE_DISK_UTIL
 FLIST_HEAD(disk_list);
-#endif
 
 unsigned long arch_flags = 0;
 
-- 
2.9.3

[PATCH v2 4/7] Always set ->real_file_size to -1 when failed to get file size

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

In some cases it's set to -1, but on others it's not,
while the existing code (e.g. get_fs_free_counts(), setup_files(), etc)
seem to expect ->real_file_size to be -1 in case of any error.

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

diff --git a/filesetup.c b/filesetup.c
index eb28826..3fa8b32 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -375,10 +375,12 @@ static int get_file_size(struct thread_data *td, struct fio_file *f)
 	else if (f->filetype == FIO_TYPE_CHAR)
 		ret = char_size(td, f);
 	else
-		f->real_file_size = -1;
+		f->real_file_size = -1ULL;
 
-	if (ret)
+	if (ret) {
+		f->real_file_size = -1ULL;
 		return ret;
+	}
 
 	if (f->file_offset > f->real_file_size) {
 		log_err("%s: offset extends end (%llu > %llu)\n", td->o.name,
-- 
2.9.3

[PATCH v2 5/7] Add missing "rand"/"trimwrite" strings to corresponding ddir slots

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

though "rand" alone is unused.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 io_ddir.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io_ddir.h b/io_ddir.h
index 2141119..613d5fb 100644
--- a/io_ddir.h
+++ b/io_ddir.h
@@ -61,9 +61,9 @@ static inline int ddir_rw(enum fio_ddir ddir)
 
 static inline const char *ddir_str(enum td_ddir ddir)
 {
-	static const char *__str[] = { NULL, "read", "write", "rw", NULL,
+	static const char *__str[] = { NULL, "read", "write", "rw", "rand",
 				"randread", "randwrite", "randrw",
-				"trim", NULL, NULL, NULL, "randtrim" };
+				"trim", NULL, "trimwrite", NULL, "randtrim" };
 
 	return __str[ddir];
 }
-- 
2.9.3

[PATCH v2 6/7] Acquire file ->lock while the lock itself is being copied

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

to the destination file pointer.
The ones in dup_files() doesn't seem to require locking from
the way it's been called.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 engines/falloc.c |  2 +-
 file.h           |  2 +-
 filesetup.c      | 12 +++++++-----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/engines/falloc.c b/engines/falloc.c
index 2b00d52..9dff9bf 100644
--- a/engines/falloc.c
+++ b/engines/falloc.c
@@ -39,7 +39,7 @@ static int open_file(struct thread_data *td, struct fio_file *f)
 	}
 
 open_again:
-	from_hash = file_lookup_open(f, O_CREAT|O_RDWR);
+	from_hash = file_lookup_open(td, f, O_CREAT|O_RDWR);
 
 	if (f->fd == -1) {
 		char buf[FIO_VERROR_SIZE];
diff --git a/file.h b/file.h
index ac00ff8..b9bf75f 100644
--- a/file.h
+++ b/file.h
@@ -192,7 +192,7 @@ extern int __must_check file_invalidate_cache(struct thread_data *, struct fio_f
 extern int __must_check generic_open_file(struct thread_data *, struct fio_file *);
 extern int __must_check generic_close_file(struct thread_data *, struct fio_file *);
 extern int __must_check generic_get_file_size(struct thread_data *, struct fio_file *);
-extern int __must_check file_lookup_open(struct fio_file *f, int flags);
+extern int __must_check file_lookup_open(struct thread_data *, struct fio_file *, int flags);
 extern int __must_check pre_read_files(struct thread_data *);
 extern unsigned long long get_rand_file_size(struct thread_data *td);
 extern int add_file(struct thread_data *, const char *, int, int);
diff --git a/filesetup.c b/filesetup.c
index 3fa8b32..73fc7e6 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -492,7 +492,7 @@ int generic_close_file(struct thread_data fio_unused *td, struct fio_file *f)
 	return ret;
 }
 
-int file_lookup_open(struct fio_file *f, int flags)
+int file_lookup_open(struct thread_data *td, struct fio_file *f, int flags)
 {
 	struct fio_file *__f;
 	int from_hash;
@@ -501,9 +501,11 @@ int file_lookup_open(struct fio_file *f, int flags)
 	if (__f) {
 		dprint(FD_FILE, "found file in hash %s\n", f->file_name);
 		/*
-		 * racy, need the __f->lock locked
+		 * acquire file lock while it gets copied to f.
 		 */
+		lock_file(td, __f, td_read(td) ? DDIR_READ : DDIR_WRITE);
 		f->lock = __f->lock;
+		unlock_file(td, __f);
 		from_hash = 1;
 	} else {
 		dprint(FD_FILE, "file not found in hash %s\n", f->file_name);
@@ -588,7 +590,7 @@ open_again:
 		if (is_std)
 			f->fd = dup(STDOUT_FILENO);
 		else
-			from_hash = file_lookup_open(f, flags);
+			from_hash = file_lookup_open(td, f, flags);
 	} else if (td_read(td)) {
 		if (f->filetype == FIO_TYPE_CHAR && !read_only)
 			flags |= O_RDWR;
@@ -598,10 +600,10 @@ open_again:
 		if (is_std)
 			f->fd = dup(STDIN_FILENO);
 		else
-			from_hash = file_lookup_open(f, flags);
+			from_hash = file_lookup_open(td, f, flags);
 	} else { //td trim
 		flags |= O_RDWR;
-		from_hash = file_lookup_open(f, flags);
+		from_hash = file_lookup_open(td, f, flags);
 	}
 
 	if (f->fd == -1) {
-- 
2.9.3

[PATCH v2 7/7] Use 0 instead of DDIR_READ to iterate from 0 to DDIR_RWDIR_CNT

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

I think this is better since we want to count up DDIR_RWDIR_CNT,
but not specifically (count - something) even if something is 0,
and that's basically what DDIR_RWDIR_CNT is there for to equal
SYNC=3 after READ/WRITE/TRIM in io_ddir.h.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 eta.c         | 6 +++---
 io_u.c        | 6 +++---
 stat.c        | 4 ++--
 steadystate.c | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/eta.c b/eta.c
index 1d66163..adf7f94 100644
--- a/eta.c
+++ b/eta.c
@@ -440,7 +440,7 @@ bool calc_thread_status(struct jobs_eta *je, int force)
 		if (td->runstate > TD_SETTING_UP) {
 			int ddir;
 
-			for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+			for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 				if (unified_rw_rep) {
 					io_bytes[0] += td->io_bytes[ddir];
 					io_iops[0] += td->io_blocks[ddir];
@@ -574,7 +574,7 @@ void display_thread_status(struct jobs_eta *je)
 			sprintf(perc_str, "%3.1f%%", perc);
 		}
 
-		for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+		for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 			rate_str[ddir] = num2str(je->rate[ddir], 4,
 						1024, je->is_pow2, je->unit_base);
 			iops_str[ddir] = num2str(je->iops[ddir], 4, 1, 0, N2S_NONE);
@@ -601,7 +601,7 @@ void display_thread_status(struct jobs_eta *je)
 			p += sprintf(p, "%*s", linelen_last - l, "");
 		linelen_last = l;
 
-		for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+		for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 			free(rate_str[ddir]);
 			free(iops_str[ddir]);
 		}
diff --git a/io_u.c b/io_u.c
index 1daaf7b..f1a3916 100644
--- a/io_u.c
+++ b/io_u.c
@@ -1902,7 +1902,7 @@ static void init_icd(struct thread_data *td, struct io_completion_data *icd,
 	icd->nr = nr;
 
 	icd->error = 0;
-	for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++)
+	for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++)
 		icd->bytes_done[ddir] = 0;
 }
 
@@ -1941,7 +1941,7 @@ int io_u_sync_complete(struct thread_data *td, struct io_u *io_u)
 		return -1;
 	}
 
-	for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++)
+	for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++)
 		td->bytes_done[ddir] += icd.bytes_done[ddir];
 
 	return 0;
@@ -1980,7 +1980,7 @@ int io_u_queued_complete(struct thread_data *td, int min_evts)
 		return -1;
 	}
 
-	for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++)
+	for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++)
 		td->bytes_done[ddir] += icd.bytes_done[ddir];
 
 	return ret;
diff --git a/stat.c b/stat.c
index f1d468c..0bb21d0 100644
--- a/stat.c
+++ b/stat.c
@@ -2442,7 +2442,7 @@ static int add_bw_samples(struct thread_data *td, struct timeval *t)
 	/*
 	 * Compute both read and write rates for the interval.
 	 */
-	for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+	for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 		uint64_t delta;
 
 		delta = td->this_io_bytes[ddir] - td->stat_io_bytes[ddir];
@@ -2517,7 +2517,7 @@ static int add_iops_samples(struct thread_data *td, struct timeval *t)
 	/*
 	 * Compute both read and write rates for the interval.
 	 */
-	for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+	for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 		uint64_t delta;
 
 		delta = td->this_io_blocks[ddir] - td->stat_io_blocks[ddir];
diff --git a/steadystate.c b/steadystate.c
index 43c715c..98f027c 100644
--- a/steadystate.c
+++ b/steadystate.c
@@ -231,7 +231,7 @@ void steadystate_check(void)
 		}
 
 		td_io_u_lock(td);
-		for (ddir = DDIR_READ; ddir < DDIR_RWDIR_CNT; ddir++) {
+		for (ddir = 0; ddir < DDIR_RWDIR_CNT; ddir++) {
 			td_iops += td->io_blocks[ddir];
 			td_bytes += td->io_bytes[ddir];
 		}
-- 
2.9.3

Re: [PATCH v2 6/7] Acquire file ->lock while the lock itself is being copied

[Jens Axboe]

On Tue, Feb 14 2017, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> to the destination file pointer.
> The ones in dup_files() doesn't seem to require locking from
> the way it's been called.

I've been thinking about this a little bit, since I could not see what
the race was. Do you spot one, or are you just acting on the comment?
I think the original race was that we copied over the lock, lock owner,
batch, etc. Hence the race was that if someone else grabbed the lock
while we were doing that copy, the fields weren't in a consistent state.
But now we're just copying the pointer to the lock, so I don't think
there's a race. It's just a stale comment.

-- 
Jens Axboe

Re: [PATCH v2 0/7]

[Jens Axboe]

On Tue, Feb 14 2017, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> configure: Use x86 instead of i386 for $cpu for IA32
>   Added "This change should have been a part of e12f4ede in 2016.".
> 
> Acquire file ->lock while the lock itself is being copied
>   Rewrote the locking using (un)lock_file().
> 
> Tomohiro Kusumi (7):
>   configure: Drop default CONFIG_LITTLE_ENDIAN for Cygwin
>   configure: Use x86 instead of i386 for $cpu for IA32
>   Drop conditional declaration of disk_list
>   Always set ->real_file_size to -1 when failed to get file size
>   Add missing "rand"/"trimwrite" strings to corresponding ddir slots
>   Acquire file ->lock while the lock itself is being copied
>   Use 0 instead of DDIR_READ to iterate from 0 to DDIR_RWDIR_CNT

Applied 1-5, and 7. I don't think we need 6, but I emailed on that
separately. Thanks!

-- 
Jens Axboe

Re: [PATCH v2 6/7] Acquire file ->lock while the lock itself is being copied

[Tomohiro Kusumi]

I couldn't see a race too, but assumed there is since you (the author) say so.
For the other one in dup_files(), I didn't think there's any race as commented.

But yes, if there's no race, please drop this.

2017-02-14 17:23 GMT+02:00 Jens Axboe <axboe@kernel.dk>:
> On Tue, Feb 14 2017, kusumi.tomohiro@gmail.com wrote:
>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>
>> to the destination file pointer.
>> The ones in dup_files() doesn't seem to require locking from
>> the way it's been called.
>
> I've been thinking about this a little bit, since I could not see what
> the race was. Do you spot one, or are you just acting on the comment?
> I think the original race was that we copied over the lock, lock owner,
> batch, etc. Hence the race was that if someone else grabbed the lock
> while we were doing that copy, the fields weren't in a consistent state.
> But now we're just copying the pointer to the lock, so I don't think
> there's a race. It's just a stale comment.
>
> --
> Jens Axboe
>

Re: [PATCH v2 6/7] Acquire file ->lock while the lock itself is being copied

[Jens Axboe]

On 02/14/2017 08:27 AM, Tomohiro Kusumi wrote:
> I couldn't see a race too, but assumed there is since you (the author) say so.
> For the other one in dup_files(), I didn't think there's any race as commented.
> 
> But yes, if there's no race, please drop this.

OK good, then I'll just drop it. BTW, for reference, this was the
original code:

if (__f) {
        /*
         * racy, need the __f->lock locked
         */
        f->lock = __f->lock;
        f->lock_owner = __f->lock_owner;
        f->lock_batch = __f->lock_batch;
        f->lock_ddir = __f->lock_ddir;
	[...]

where that comment was from.

-- 
Jens Axboe