[PATCH] ext3: enable cache flush in ext3_sync_file

[PATCH] ext3: enable cache flush in ext3_sync_file

[Christoph Hellwig]

We need to flush the write cache unconditionally in ->fsync, otherwise
writes into already allocated blocks can get lost.  Writes into fully
allocated files are very common when using disk images for
virtualization, and without this fix can easily lose data after
an fdatasync, which is the typical implementation for a cache flush on
the virtual drive.


Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: linux-2.6/fs/ext3/fsync.c
===================================================================
--- linux-2.6.orig/fs/ext3/fsync.c
+++ linux-2.6/fs/ext3/fsync.c
@@ -23,6 +23,7 @@
  */
 
 #include <linux/time.h>
+#include <linux/blkdev.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/writeback.h>
@@ -87,5 +88,7 @@ int ext3_sync_file(struct file * file, s
 		ret = sync_inode(inode, &wbc);
 	}
 out:
+	if (test_opt(inode->i_sb, BARRIER))
+		blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
 	return ret;
 }

Re: [PATCH] ext3: enable cache flush in ext3_sync_file

[Eric Sandeen]

Christoph Hellwig wrote:
> We need to flush the write cache unconditionally in ->fsync, otherwise
> writes into already allocated blocks can get lost.  Writes into fully
> allocated files are very common when using disk images for
> virtualization, and without this fix can easily lose data after
> an fdatasync, which is the typical implementation for a cache flush on
> the virtual drive.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Given that I tried to do the same thing 1.5 years ago (though not quite
correctly) ...

Acked-by: Eric Sandeen <sandeen@redhat.com>

> 
> Index: linux-2.6/fs/ext3/fsync.c
> ===================================================================
> --- linux-2.6.orig/fs/ext3/fsync.c
> +++ linux-2.6/fs/ext3/fsync.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include <linux/time.h>
> +#include <linux/blkdev.h>
>  #include <linux/fs.h>
>  #include <linux/sched.h>
>  #include <linux/writeback.h>
> @@ -87,5 +88,7 @@ int ext3_sync_file(struct file * file, s
>  		ret = sync_inode(inode, &wbc);
>  	}
>  out:
> +	if (test_opt(inode->i_sb, BARRIER))
> +		blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
>  	return ret;
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ext3: enable cache flush in ext3_sync_file

[Jan Kara]

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

> Christoph Hellwig wrote:
> > We need to flush the write cache unconditionally in ->fsync, otherwise
> > writes into already allocated blocks can get lost.  Writes into fully
> > allocated files are very common when using disk images for
> > virtualization, and without this fix can easily lose data after
> > an fdatasync, which is the typical implementation for a cache flush on
> > the virtual drive.
> > 
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Given that I tried to do the same thing 1.5 years ago (though not quite
> correctly) ...
> 
> Acked-by: Eric Sandeen <sandeen@redhat.com>
  But would the patch below be better? When we force a transaction
commit we don't have to flush caches again. Or am I missing something?

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

[-- Attachment #2: 0001-ext3-Flush-disk-caches-on-fsync-when-needed.patch --]
[-- Type: text/x-diff, Size: 1628 bytes --]

>From 4412c3d5cc04849d959e9083d3bdf0b4a058e947 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Tue, 8 Sep 2009 14:59:42 +0200
Subject: [PATCH] ext3: Flush disk caches on fsync when needed

In case we fsync() a file and inode is not dirty, we don't force a transaction
to disk and hence don't flush disk caches. Thus file data could be just in disk
caches and not on persistent storage. Fix the problem by flushing disk caches
if we didn't force a transaction commit.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/fsync.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/fs/ext3/fsync.c b/fs/ext3/fsync.c
index d336341..451d166 100644
--- a/fs/ext3/fsync.c
+++ b/fs/ext3/fsync.c
@@ -23,6 +23,7 @@
  */
 
 #include <linux/time.h>
+#include <linux/blkdev.h>
 #include <linux/fs.h>
 #include <linux/sched.h>
 #include <linux/writeback.h>
@@ -73,7 +74,7 @@ int ext3_sync_file(struct file * file, struct dentry *dentry, int datasync)
 	}
 
 	if (datasync && !(inode->i_state & I_DIRTY_DATASYNC))
-		goto out;
+		goto flush;
 
 	/*
 	 * The VFS has written the file data.  If the inode is unaltered
@@ -85,7 +86,16 @@ int ext3_sync_file(struct file * file, struct dentry *dentry, int datasync)
 			.nr_to_write = 0, /* sys_fsync did this */
 		};
 		ret = sync_inode(inode, &wbc);
+		goto out;
 	}
+flush:
+	/*
+	 * In case we didn't commit a transaction, we have to flush
+	 * disk caches manually so that data really is on persistent
+	 * storage
+	 */
+	if (test_opt(inode->i_sb, BARRIER))
+		blkdev_issue_flush(inode->i_sb->s_bdev, NULL);
 out:
 	return ret;
 }
-- 
1.6.0.2

Re: [PATCH] ext3: enable cache flush in ext3_sync_file

[Christoph Hellwig]

On Tue, Sep 08, 2009 at 03:04:38PM +0200, Jan Kara wrote:
> > Acked-by: Eric Sandeen <sandeen@redhat.com>
>   But would the patch below be better? When we force a transaction
> commit we don't have to flush caches again. Or am I missing something?

If you know that you really did issue a barrier for sure you don't have
to flush the cache again.  I don't know extN enough to easily figure out
how, but if this patch ensures that it's good.  Same scheme should also
apply to ext4.