[PATCH V5 1/2] FS: Add generic data flush to fsync

[PATCH V5 1/2] FS: Add generic data flush to fsync

[Fabian Frederick]

This patch issues a flush in generic_file_fsync.
(Modern filesystems already do it)

Behaviour can be reversed using /sys/devices/.../cache_type
or by calling __generic_file_fsync

Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
v5: patch2/2 ext4 patch fix (Thanks to Ming Lei)
V4: update description
V3: __generic_file_fsync = no flush
V2: No additional flag
V1: First version with MS_BARRIER flag

 fs/libfs.c         | 36 +++++++++++++++++++++++++++++++++---
 include/linux/fs.h |  1 +
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index a184424..4877906 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -3,6 +3,7 @@
  *	Library for filesystems writers.
  */
 
+#include <linux/blkdev.h>
 #include <linux/export.h>
 #include <linux/pagemap.h>
 #include <linux/slab.h>
@@ -923,16 +924,19 @@ struct dentry *generic_fh_to_parent(struct super_block *sb, struct fid *fid,
 EXPORT_SYMBOL_GPL(generic_fh_to_parent);
 
 /**
- * generic_file_fsync - generic fsync implementation for simple filesystems
+ * __generic_file_fsync - generic fsync implementation for simple filesystems
+ *
  * @file:	file to synchronize
+ * @start:	start offset in bytes
+ * @end:	end offset in bytes (inclusive)
  * @datasync:	only synchronize essential metadata if true
  *
  * This is a generic implementation of the fsync method for simple
  * filesystems which track all non-inode metadata in the buffers list
  * hanging off the address_space structure.
  */
-int generic_file_fsync(struct file *file, loff_t start, loff_t end,
-		       int datasync)
+int __generic_file_fsync(struct file *file, loff_t start, loff_t end,
+				 int datasync)
 {
 	struct inode *inode = file->f_mapping->host;
 	int err;
@@ -952,10 +956,36 @@ int generic_file_fsync(struct file *file, loff_t start, loff_t end,
 	err = sync_inode_metadata(inode, 1);
 	if (ret == 0)
 		ret = err;
+
 out:
 	mutex_unlock(&inode->i_mutex);
 	return ret;
 }
+EXPORT_SYMBOL(__generic_file_fsync);
+
+/**
+ * generic_file_fsync - generic fsync implementation for simple filesystems
+ *			with flush
+ * @file:	file to synchronize
+ * @start:	start offset in bytes
+ * @end:	end offset in bytes (inclusive)
+ * @datasync:	only synchronize essential metadata if true
+ *
+ */
+
+int generic_file_fsync(struct file *file, loff_t start, loff_t end,
+		       int datasync)
+{
+	struct inode *inode = file->f_mapping->host;
+	int err;
+
+	err = __generic_file_fsync(file, start, end, datasync);
+	if (err)
+		return err;
+
+	return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
+}
 EXPORT_SYMBOL(generic_file_fsync);
 
 /**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8780312..c3f46e4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2590,6 +2590,7 @@ extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
 extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
 		const void __user *from, size_t count);
 
+extern int __generic_file_fsync(struct file *, loff_t, loff_t, int);
 extern int generic_file_fsync(struct file *, loff_t, loff_t, int);
 
 extern int generic_check_addressable(unsigned, u64);
-- 
1.8.4.5

Re: [PATCH V5 1/2] FS: Add generic data flush to fsync

[Andrew Morton]

On Mon, 12 May 2014 07:20:27 +0200 Fabian Frederick <fabf@skynet.be> wrote:

> This patch issues a flush in generic_file_fsync.
> (Modern filesystems already do it)
> 
> Behaviour can be reversed using /sys/devices/.../cache_type
> or by calling __generic_file_fsync

Well OK, but why?  What effect does the patch have?  Does it make the
kernel better and if so, how?

>
> +/**
> + * generic_file_fsync - generic fsync implementation for simple filesystems
> + *			with flush
> + * @file:	file to synchronize
> + * @start:	start offset in bytes
> + * @end:	end offset in bytes (inclusive)
> + * @datasync:	only synchronize essential metadata if true
> + *
> + */
> +
> +int generic_file_fsync(struct file *file, loff_t start, loff_t end,
> +		       int datasync)
> +{
> +	struct inode *inode = file->f_mapping->host;
> +	int err;
> +
> +	err = __generic_file_fsync(file, start, end, datasync);
> +	if (err)
> +		return err;
> +
> +	return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
> +
> +}

Documentation/SubmitChecklist, section 2b ;)

Re: [PATCH V5 1/2] FS: Add generic data flush to fsync

[Debabrata Banerjee]

On Mon, May 12, 2014 at 5:50 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 12 May 2014 07:20:27 +0200 Fabian Frederick <fabf@skynet.be> wrote:
>
>> This patch issues a flush in generic_file_fsync.
>> (Modern filesystems already do it)
>>
>> Behaviour can be reversed using /sys/devices/.../cache_type
>> or by calling __generic_file_fsync
>
> Well OK, but why?  What effect does the patch have?  Does it make the
> kernel better and if so, how?

Doing flush of the device cache is the right to do. Without this, any
fsync or sync call makes absolutely no guarantees that your data is on
non-volatile storage, and both calls or commands may as well be a NOP,
especially given the sizes of write-back caches on modern drives.

This actually fixes a long standing problem with ext4, that
sync()/fsync() actually don't function if you don't have journal
enabled. However a patch to ext4 may be considered such that nobarrier
mount flag is honored without a journal via calling
__generic_file_fsync instead.

i.e.

diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index a8bc47f..779018a 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -107,7 +107,11 @@ int ext4_sync_file(struct file *file, loff_t
start, loff_t end, int datasync)
        }

        if (!journal) {
-               ret = generic_file_fsync(file, start, end, datasync);
+               if (test_opt(inode->i_sb, BARRIER))
+                       ret = generic_file_fsync(file, start, end, datasync);
+               else
+                       ret = __generic_file_fsync(file, start, end, datasync);
+
                if (!ret && !hlist_empty(&inode->i_dentry))
                        ret = ext4_sync_parent(inode);
                goto out;
diff --git a/Documentation/filesystems/ext4.txt
b/Documentation/filesystems/ext4.txt
index 919a329..3781568 100644
--- a/Documentation/filesystems/ext4.txt
+++ b/Documentation/filesystems/ext4.txt
@@ -182,8 +182,8 @@ commit=nrsec        (*)     Ext4 can be told to
sync all its data and metadata
                        Setting it to very large values will improve
                        performance.

-barrier=<0|1(*)>       This enables/disables the use of write barriers in
-barrier(*)             the jbd code.  barrier=0 disables, barrier=1 enables.
+barrier=<0|1(*)>       This enables/disables the use of write barriers.
+barrier(*)             barrier=0 disables, barrier=1 enables.
 nobarrier              This also requires an IO stack which can support
                        barriers, and if jbd gets an error on a barrier
                        write, it will disable again with a warning.

Re: [PATCH V5 1/2] FS: Add generic data flush to fsync

[Fabian Frederick]

On Mon, 12 May 2014 14:50:27 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 12 May 2014 07:20:27 +0200 Fabian Frederick <fabf@skynet.be> wrote:
> 
> > This patch issues a flush in generic_file_fsync.
> > (Modern filesystems already do it)
> > 
> > Behaviour can be reversed using /sys/devices/.../cache_type
> > or by calling __generic_file_fsync
> 
> Well OK, but why?  What effect does the patch have?  Does it make the
> kernel better and if so, how?
> 
> >
> > +/**
> > + * generic_file_fsync - generic fsync implementation for simple filesystems
> > + *			with flush
> > + * @file:	file to synchronize
> > + * @start:	start offset in bytes
> > + * @end:	end offset in bytes (inclusive)
> > + * @datasync:	only synchronize essential metadata if true
> > + *
> > + */
> > +
> > +int generic_file_fsync(struct file *file, loff_t start, loff_t end,
> > +		       int datasync)
> > +{
> > +	struct inode *inode = file->f_mapping->host;
> > +	int err;
> > +
> > +	err = __generic_file_fsync(file, start, end, datasync);
> > +	if (err)
> > +		return err;
> > +
> > +	return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
> > +
> > +}
> 
> Documentation/SubmitChecklist, section 2b ;)

Hi Andrew,

   As Jan suggested, I can resubmit patches with #ifdef CONFIG_BLOCK around
blkdev_issue_flush and further description. Do you see something else ?

Thanks,
Fabian