[PATCH] ext4: remove filemap_fdatawrite() when synchronizing a data journaled file

[PATCH] ext4: remove filemap_fdatawrite() when synchronizing a data journaled file

[Daeho Jeong]

When fsync() system call is executed, all data of the file should be written on the storage.
But, in a case of that the file is operated in data journal mode, the data doesn't need to be
flushed out and waited for its I/O completion. Moreover, by invoking filemap_fdatawrite()
function on the data journaled file, ext4_sync_file() suffers from an unnecessary delay
which is caused by flushing out and waiting for the I/O completion of a ton of newly-dirtied
pages which were dirtied by the previous commit.

Signed-off-by: Daeho Jeong <daeho.jeong@samsung.com>
---
 fs/ext4/fsync.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 8850254..e0fabf7 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -112,9 +112,6 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
   goto out;
  }
 
- ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
- if (ret)
-  return ret;
  /*
   * data=writeback,ordered:
   *  The caller's filemap_fdatawrite()/wait will sync the data.
@@ -125,13 +122,15 @@ int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
   *  filemap_fdatawrite won't do anything (the buffers are clean).
   *  ext4_force_commit will write the file data into the journal and
   *  will wait on that.
-  *  filemap_fdatawait() will encounter a ton of newly-dirtied pages
-  *  (they were dirtied by commit).  But that's OK - the blocks are
-  *  safe in-journal, which is all fsync() needs to ensure.
   */
  if (ext4_should_journal_data(inode)) {
   ret = ext4_force_commit(inode->i_sb);
   goto out;
+ } else {
+  ret = filemap_write_and_wait_range(inode->i_mapping,
+         start, end);
+  if (ret)
+   return ret;
  }
 
  commit_tid = datasync ? ei->i_datasync_tid : ei->i_sync_tid;
-- 
1.7.9.5

Re: [PATCH] ext4: remove filemap_fdatawrite() when synchronizing a data journaled file

[Theodore Ts'o]

On Tue, Jul 28, 2015 at 02:17:26AM +0000, Daeho Jeong wrote:
> When fsync() system call is executed, all data of the file should be written on the storage.
> But, in a case of that the file is operated in data journal mode, the data doesn't need to be
> flushed out and waited for its I/O completion. Moreover, by invoking filemap_fdatawrite()
> function on the data journaled file, ext4_sync_file() suffers from an unnecessary delay
> which is caused by flushing out and waiting for the I/O completion of a ton of newly-dirtied
> pages which were dirtied by the previous commit.

No, this patch is incorrect.  Data journalling works by having the
buffers getting dirtied, and that doesn't happen until we call
filemap_fdatawrite_range().  It's true that we don't have to call
filemap_write_and_wait_range(), but simply skipping the writeback
entirely will cause data writes to get lost after an fsync() if we
crash immediately afterwards.

I'm guessing you didn't actually test this patch for correctness?

    	     	 		      	   	 - Ted