Hi.

Currently fdatasync is identical to fsync in ext3,4.

I think fdatasync should skip journal flush in data=ordered and data=writeback mode

because this syscall is not required to synchronize the metadata.
My patch as below is similar to the approach of GFS2's fsync code(gfs2_fsync).
Thanks.
Signed-off-by :Hisashi Hifumi <hifumi.hisashi@oss.ntt.co.jp>
diff -Nrup linux-2.6.24-rc2.org/fs/ext3/fsync.c linux-2.6.24-rc2/fs/ext3/fsync.c

--- linux-2.6.24-rc2.org/fs/ext3/fsync.c	2007-11-07 06:57:46.000000000 +0900

+++ linux-2.6.24-rc2/fs/ext3/fsync.c	2007-11-15 17:50:24.000000000 +0900

@@ -72,6 +72,9 @@ int ext3_sync_file(struct file * file, s

 		goto out;

 	}
+	if (datasync)

+		goto out;

+

 	/*

 	* The VFS has written the file data.  If the inode is unaltered

 	* then we need not start a commit.

diff -Nrup linux-2.6.24-rc2.org/fs/ext4/fsync.c linux-2.6.24-rc2/fs/ext4/fsync.c

--- linux-2.6.24-rc2.org/fs/ext4/fsync.c	2007-11-07 06:57:46.000000000 +0900

+++ linux-2.6.24-rc2/fs/ext4/fsync.c	2007-11-15 17:50:54.000000000 +0900

@@ -72,6 +72,9 @@ int ext4_sync_file(struct file * file, s

 		goto out;

 	}
+	if (datasync)

+		goto out;

+

 	/*

 	* The VFS has written the file data.  If the inode is unaltered

 	* then we need not start a commit.

This is wrong.  If I_DIRTY_DATASYNC is set, the inode needs to be

written even for datasync.
How about the patch below?
Jörn
--

Audacity augments courage; hesitation, fear.

-- Publilius Syrus
Signed-off-by: Jörn Engel <joern@logfs.org>

---
 fs/ext3/fsync.c |    3 ++-

 fs/ext4/fsync.c |    3 ++-

 2 files changed, 4 insertions(+), 2 deletions(-)
--- git_I_DIRTY/fs/ext3/fsync.c~ext3_datasync	2007-11-15 20:51:54.000000000 +0100

+++ git_I_DIRTY/fs/ext3/fsync.c	2007-11-16 04:42:28.000000000 +0100

@@ -76,7 +76,8 @@ int ext3_sync_file(struct file * file, s

 	 * The VFS has written the file data.  If the inode is unaltered

 	 * then we need not start a commit.

 	 */

-	if (inode->i_state & (I_DIRTY_SYNC|I_DIRTY_DATASYNC)) {

+	if (((inode->i_state & I_DIRTY_SYNC) && !datasync)

+			|| (inode->i_state & I_DIRTY_DATASYNC)) {

 		struct writeback_control wbc = {

 			.sync_mode = WB_SYNC_ALL,

 			.nr_to_write = 0, /* sys_fsync did this */

--- git_I_DIRTY/fs/ext4/fsync.c~ext3_datasync	2007-11-15 20:51:54.000000000 +0100

+++ git_I_DIRTY/fs/ext4/fsync.c	2007-11-16 04:44:29.000000000 +0100

@@ -76,7 +76,8 @@ int ext4_sync_file(struct file * file, s

 	 * The VFS has written the file data.  If the inode is unaltered

 	 * then we need not start a commit.

 	 */

-	if (inode->i_state & (I_DIRTY_SYNC|I_DIRTY_DATASYNC)) {

+	if (((inode->i_state & I_DIRTY_SYNC) && !datasync)

+			|| (inode->i_state & I_DIRTY_DATASYNC)) {

 		struct writeback_control wbc = {

 			.sync_mode = WB_SYNC_ALL,

 			.nr_to_write = 0, /* sys_fsync did this */

-

I suppose so.  Although one wonders what earthly point there is in syncing

a file's data if we haven't yet written out the metadata which is required

for locating that data.
IOW, fdatasync() is only useful if the application knows that it is overwriting

already-instantiated blocks.
In which case it might as well have used fsync().  For ext2-style filesystems,

anyway.
hm.  It needs some thought.

-

Hi.
Currently fdatasync is identical to fsync in ext3,4.

I think fdatasync should skip journal flush in data=ordered and data=writeback mode

when it overwrites to already-instantiated blocks on HDD.

When I_DIRTY_DATASYNC flag is not set, fdatasync should skip journal writeout

because this indicates only atime or/and mtime updates.  
Following patch is the same approach of ext2's fsync code(ext2_sync_file).
I did a performance test using the sysbench.
#sysbench --num-threads=128 --max-requests=50000 --test=fileio --file-total-size=128G

--file-test-mode=rndwr --file-fsync-mode=fdatasync run
The result was:
	-2.6.24

	Operations performed:  0 Read, 50080 Write, 59600 Other = 109680 Total

	Read 0b  Written 782.5Mb  Total transferred 782.5Mb  (12.116Mb/sec)

	  775.45 Requests/sec executed
	Test execution summary:

	    total time:                          64.5814s

	    total number of events:              50080

	    total time taken by event execution: 3713.9836

	    per-request statistics:

	         min:                            0.0000s

	         avg:                            0.0742s

	         max:                            0.9375s

	         approx.  95 percentile:         0.2901s
	Threads fairness:

	    events (avg/stddev):           391.2500/23.26

	    execution time (avg/stddev):   29.0155/1.99
	-2.6.24-patched

	Operations performed:  0 Read, 50009 Write, 61596 Other = 111605 Total

	Read 0b  Written 781.39Mb  Total transferred 781.39Mb  (16.419Mb/sec)

	 1050.83 Requests/sec executed
	Test execution summary:

	    total time:                          47.5900s

	    total number of events:              50009

	    total time taken by event execution: 2934.5768

	    per-request statistics:

 	         min:                            0.0000s

	         avg:                            0.0587s

 	         max:                            0.8938s

	         approx.  95 percentile:         0.1993s
	Threads fairness:

	    events (avg/stddev):           390.6953/2...
[ message continues ]

Yes, the patch looks fine. You can add

Acked-by: Jan Kara <jack@suse.cz>

  if you wish.

--

Jan Kara <jack@suse.cz>

SuSE CR Labs

-

Thank you for your comment.

Please merge my patch.
Thanks.
[PATCH] ext3,4:fdatasync should skip metadata writeout when overwriting
Currently fdatasync is identical to fsync in ext3,4.

I think fdatasync should skip journal flush in data=ordered and data=writeback mode

when it overwrites to already-instantiated blocks on HDD.

When I_DIRTY_DATASYNC flag is not set, fdatasync should skip journal writeout

because this indicates only atime or/and mtime updates.  
Following patch is the same approach of ext2's fsync code(ext2_sync_file).
I did a performance test using the sysbench.
#sysbench --num-threads=128 --max-requests=50000 --test=fileio --file-total-size=128G

--file-test-mode=rndwr --file-fsync-mode=fdatasync run
The result was:
	-2.6.24

	Operations performed:  0 Read, 50080 Write, 59600 Other = 109680 Total

	Read 0b  Written 782.5Mb  Total transferred 782.5Mb  (12.116Mb/sec)

	  775.45 Requests/sec executed
	Test execution summary:

	    total time:                          64.5814s

	    total number of events:              50080

	    total time taken by event execution: 3713.9836

	    per-request statistics:

	         min:                            0.0000s

	         avg:                            0.0742s

	         max:                            0.9375s

	         approx.  95 percentile:         0.2901s
	Threads fairness:

	    events (avg/stddev):           391.2500/23.26

	    execution time (avg/stddev):   29.0155/1.99
	-2.6.24-patched

	Operations performed:  0 Read, 50009 Write, 61596 Other = 111605 Total

	Read 0b  Written 781.39Mb  Total transferred 781.39Mb  (16.419Mb/sec)

	1050.83 Requests/sec executed
	Test execution summary:

	    total time:                          47.5900s

	    total number of events:              50009

	    total time taken by event execution: 2934.5768

	    per-request statistics:

 	         min:                            0.0000s

	         avg:                            0.0587s

 	         max:                     ...
[ message continues ]

At 11:59 07/11/16, Andrew Morton wrote:

 >

 >I suppose so.  Although one wonders what earthly point there is in syncing

 >a file's data if we haven't yet written out the metadata which is required

 >for locating that data.

 >

 >IOW, fdatasync() is only useful if the application knows that it is overwriting

 >already-instantiated blocks.

 >

 >In which case it might as well have used fsync().  For ext2-style filesystems,

 >anyway.

 >

 >hm.  It needs some thought.
I did a test to measure the file overwriting performance difference between

original fdatasync and one that skips journal flush.

The test program and obtained result is as follows:
Test program source code:
#include <stdio.h>

#include <sys/types.h>

#include <sys/stat.h>

#include <fcntl.h>

#include <unistd.h>

#include <time.h>
#define BUFSIZE 8192

#define LOOP 1024*1024
main(void)

{

	int i;

	int fd;

	char buf[BUFSIZE];

	time_t t1,t2;
	memset(buf,0,BUFSIZE);
	fd = open("testfile", O_CREAT|O_RDWR);

	if (fd < 0)

		perror("cannot open file\n");
	for (i = 0; i < LOOP; i++)

		write(fd,buf,BUFSIZE);

	fsync(fd);
	lseek(fd, 0, SEEK_SET);

	time(&t1);

	for (i = 0; i < LOOP; i++) {

		write(fd,buf,BUFSIZE);

		fdatasync(fd);

	}

	time(&t2);
	printf("%d sec\n",t2-t1);

}
Result:
  2.6.24-rc3:
	264 sec
  2.6.23-rc3-fdatasync-skips-journal-flush-patched
	253 sec
Hardware environment:

  Dell Poweredge 850

  CPU Pentium D  3GHz

  memory 4GB

  HDD Maxtor 6L160M0
I got somewhat better result from the patched ext3 skipping journal flush.
Some DBMS such as PostgreSQL can use fdatasync. So I think skipping

journal flush on overwriting leads to performance improvement for

these application.
I am for the notion that skipping metadata writeout unconditionally is wrong,

and "important metadata" such as i_size, block-bitmap etc should be

synched even if fdatasync is issued , but unimportant meta s...
[ message continues ]

There are non-trivial amount of performance critical programs,

particularly in financial application segment ported from legacy UNIX

platforms, know the difference between fsync() and fdatasync(). Those

can certainly take advantages of this separation. Don't underestimate

the talents of these application programmers.
-- Wendy
-

If they're that good, they'll be using sync_file_range() ;)

-

fsync() will sync an inode even if only i_atime was changed.

fdatasync() would ignore such changes.  I guess atime was the major

reason for creating fdatasync() in the first place.
The patch I sent you just minutes ago sorta documents this.

I_DIRTY_DATASYNC was added with patch-2.4.0-test12 for just this reason.
So basically an application can almost always use fdatasync() instead of

fsync() and rely on the kernel to only cut corners, if doing so will not

endanger the data synced to disk.
Jörn
--

Joern's library part 6:

http://www.gzip.org/zlib/feldspar.html

-

I think it was mtime.  One doesn't normally call any kind of sync when one

is just reading the file.  But keeping an accurate mtime is often not

worth the I/O.
And theoretically, there could be all kinds of "truly meta" metadata that

changes as you write to the file but would probably be considered more

expendable than the file's actual data.
But I think it was always intended that fdatasync() would sync the data in

a meaningful way -- i.e. such that the data can be retrieved after a

system failure; it surely wasn't meant for the user to understand

filesystem internals.  I've heard the term "data-related metadata" to

distinguish such things as allocation maps and pointer blocks from mtime,

permissions, etc.
--

Bryan Henderson                     IBM Almaden Research Center

San Jose CA                         Filesystems
-