[PATCH, RFC] add a vfs_fsync helper

[PATCH, RFC] add a vfs_fsync helper

[Christoph Hellwig]

Fsync currently has a fdatawrite/fdatawait pair around the method call,
and a mutex_lock/unlock of the inode mutex.  All callers of fsync have
to duplicate this, but we have a few and most of them don't quite get
it right.  This patch adds a new vfs_fsync that takes care of this.
It's a little more complicated as usual as ->fsync might get a NULL file
pointer and just a dentry from nfsd, but otherwise gets afile and we
want to take the mapping and file operations from it when it is there.

Notes on the fsync callers:

 - ecryptfs wasn't calling filemap_fdatawrite / filemap_fdatawait on the
   	lower file
 - coda wasn't calling filemap_fdatawrite / filemap_fdatawait on the host
	file, and returning 0 when ->fsync was missing
 - shm wasn't calling either filemap_fdatawrite / filemap_fdatawait nor
   taking i_mutex.  Now given that shared memory doesn't have disk
   backing not doing anything in fsync seems fine and I left it out of
   the vfs_fsync conversion for now, but in that case we might just
   not pass it through to the lower file at all but just call the no-op
   simple_sync_file directly.


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

Index: xfs/fs/nfsd/vfs.c
===================================================================
--- xfs.orig/fs/nfsd/vfs.c	2008-12-19 15:02:53.816810089 +0100
+++ xfs/fs/nfsd/vfs.c	2008-12-21 12:22:17.178896598 +0100
@@ -743,45 +743,16 @@
 	fput(filp);
 }
 
-/*
- * Sync a file
- * As this calls fsync (not fdatasync) there is no need for a write_inode
- * after it.
- */
-static inline int nfsd_dosync(struct file *filp, struct dentry *dp,
-			      const struct file_operations *fop)
-{
-	struct inode *inode = dp->d_inode;
-	int (*fsync) (struct file *, struct dentry *, int);
-	int err;
-
-	err = filemap_fdatawrite(inode->i_mapping);
-	if (err == 0 && fop && (fsync = fop->fsync))
-		err = fsync(filp, dp, 0);
-	if (err == 0)
-		err = filemap_fdatawait(inode->i_mapping);
-
-	return err;
-}
-	
-
 static int
 nfsd_sync(struct file *filp)
 {
-        int err;
-	struct inode *inode = filp->f_path.dentry->d_inode;
-	dprintk("nfsd: sync file %s\n", filp->f_path.dentry->d_name.name);
-	mutex_lock(&inode->i_mutex);
-	err=nfsd_dosync(filp, filp->f_path.dentry, filp->f_op);
-	mutex_unlock(&inode->i_mutex);
-
-	return err;
+	return vfs_fsync(filp, filp->f_path.dentry->d_inode, 0);
 }
 
 int
-nfsd_sync_dir(struct dentry *dp)
+nfsd_sync_dir(struct dentry *dentry)
 {
-	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
+	return vfs_fsync(NULL, dentry, 0);
 }
 
 /*
Index: xfs/fs/sync.c
===================================================================
--- xfs.orig/fs/sync.c	2008-12-19 15:02:53.942907780 +0100
+++ xfs/fs/sync.c	2008-12-21 12:22:17.180896085 +0100
@@ -75,14 +75,39 @@
 	return ret;
 }
 
-long do_fsync(struct file *file, int datasync)
+/**
+ * vfs_fsync - perform a fsync or fdatasync on a file
+ * @file:		file to sync
+ * @dentry:		dentry of @file
+ * @data:		only perform a fdatasync operation
+ *
+ * Write back data and metadata for @file to disk.  If @datasync is
+ * set only metadata needed to access modified file data is written.
+ *
+ * In case this function is called from nfsd @file may be %NULL and
+ * only @dentry is set.  This can only happen when the filesystem
+ * implements the export_operations API.
+ */
+int vfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 {
-	int ret;
-	int err;
-	struct address_space *mapping = file->f_mapping;
+	struct file_operations *fop;
+	struct address_space *mapping;
+	int err, ret;
+
+	/*
+	 * Get mapping and operations from the file in case we have
+	 * as file, or get the default values for them in case we
+	 * don't have a struct file available.  Damn nfsd..
+	 */
+	if (file) {
+		mapping = file->f_mapping;
+		fop = file->f_op;
+	} else {
+		mapping = dentry->d_inode->i_mapping;
+		fop = dentry->d_inode->i_fop;
+	}
 
-	if (!file->f_op || !file->f_op->fsync) {
-		/* Why?  We can still call filemap_fdatawrite */
+	if (!fop || !fop->fsync) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -94,7 +119,7 @@
 	 * livelocks in fsync_buffers_list().
 	 */
 	mutex_lock(&mapping->host->i_mutex);
-	err = file->f_op->fsync(file, file->f_path.dentry, datasync);
+	err = fop->fsync(file, dentry, datasync);
 	if (!ret)
 		ret = err;
 	mutex_unlock(&mapping->host->i_mutex);
@@ -105,14 +130,14 @@
 	return ret;
 }
 
-static long __do_fsync(unsigned int fd, int datasync)
+static int do_fsync(unsigned int fd, int datasync)
 {
 	struct file *file;
 	int ret = -EBADF;
 
 	file = fget(fd);
 	if (file) {
-		ret = do_fsync(file, datasync);
+		ret = vfs_fsync(file, file->f_path.dentry, datasync);
 		fput(file);
 	}
 	return ret;
@@ -120,12 +145,12 @@
 
 asmlinkage long sys_fsync(unsigned int fd)
 {
-	return __do_fsync(fd, 0);
+	return do_fsync(fd, 0);
 }
 
 asmlinkage long sys_fdatasync(unsigned int fd)
 {
-	return __do_fsync(fd, 1);
+	return do_fsync(fd, 1);
 }
 
 /*
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2008-12-19 15:02:54.427785087 +0100
+++ xfs/include/linux/fs.h	2008-12-21 12:22:17.187054621 +0100
@@ -1826,7 +1826,7 @@
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
 
-extern long do_fsync(struct file *file, int datasync);
+extern int vfs_fsync(struct file *file, struct dentry *dentry, int datasync);
 extern void sync_supers(void);
 extern void sync_filesystems(int wait);
 extern void __fsync_super(struct super_block *sb);
Index: xfs/mm/msync.c
===================================================================
--- xfs.orig/mm/msync.c	2008-12-19 15:02:56.646810993 +0100
+++ xfs/mm/msync.c	2008-12-21 12:22:17.188896406 +0100
@@ -82,7 +82,7 @@
 				(vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&mm->mmap_sem);
-			error = do_fsync(file, 0);
+			error = vfs_fsync(file, file->f_path.dentry, 0);
 			fput(file);
 			if (error || start >= end)
 				goto out;
Index: xfs/drivers/usb/gadget/file_storage.c
===================================================================
--- xfs.orig/drivers/usb/gadget/file_storage.c	2008-12-21 12:26:53.254894542 +0100
+++ xfs/drivers/usb/gadget/file_storage.c	2008-12-21 12:28:08.914896191 +0100
@@ -1863,26 +1863,10 @@
 static int fsync_sub(struct lun *curlun)
 {
 	struct file	*filp = curlun->filp;
-	struct inode	*inode;
-	int		rc, err;
 
 	if (curlun->ro || !filp)
 		return 0;
-	if (!filp->f_op->fsync)
-		return -EINVAL;
-
-	inode = filp->f_path.dentry->d_inode;
-	mutex_lock(&inode->i_mutex);
-	rc = filemap_fdatawrite(inode->i_mapping);
-	err = filp->f_op->fsync(filp, filp->f_path.dentry, 1);
-	if (!rc)
-		rc = err;
-	err = filemap_fdatawait(inode->i_mapping);
-	if (!rc)
-		rc = err;
-	mutex_unlock(&inode->i_mutex);
-	VLDBG(curlun, "fdatasync -> %d\n", rc);
-	return rc;
+	return vfs_fsync(filp, filp->f_path.dentry->d_inode, 1);
 }
 
 static void fsync_all(struct fsg_dev *fsg)
Index: xfs/fs/coda/file.c
===================================================================
--- xfs.orig/fs/coda/file.c	2008-12-21 12:28:26.575894132 +0100
+++ xfs/fs/coda/file.c	2008-12-21 12:31:32.556928596 +0100
@@ -200,8 +200,7 @@
 int coda_fsync(struct file *coda_file, struct dentry *coda_dentry, int datasync)
 {
 	struct file *host_file;
-	struct dentry *host_dentry;
-	struct inode *host_inode, *coda_inode = coda_dentry->d_inode;
+	struct inode *coda_inode = coda_dentry->d_inode;
 	struct coda_file_info *cfi;
 	int err = 0;
 
@@ -213,14 +212,7 @@
 	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
 	host_file = cfi->cfi_container;
 
-	if (host_file->f_op && host_file->f_op->fsync) {
-		host_dentry = host_file->f_path.dentry;
-		host_inode = host_dentry->d_inode;
-		mutex_lock(&host_inode->i_mutex);
-		err = host_file->f_op->fsync(host_file, host_dentry, datasync);
-		mutex_unlock(&host_inode->i_mutex);
-	}
-
+	err = vfs_fsync(host_file, host_file->f_path.dentry, datasync);
 	if ( !err && !datasync ) {
 		lock_kernel();
 		err = venus_fsync(coda_inode->i_sb, coda_i2f(coda_inode));
Index: xfs/fs/ecryptfs/file.c
===================================================================
--- xfs.orig/fs/ecryptfs/file.c	2008-12-21 12:31:51.618018979 +0100
+++ xfs/fs/ecryptfs/file.c	2008-12-21 12:34:31.203894170 +0100
@@ -275,18 +275,9 @@
 static int
 ecryptfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 {
-	struct file *lower_file = ecryptfs_file_to_lower(file);
-	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
-	struct inode *lower_inode = lower_dentry->d_inode;
-	int rc = -EINVAL;
-
-	if (lower_inode->i_fop->fsync) {
-		mutex_lock(&lower_inode->i_mutex);
-		rc = lower_inode->i_fop->fsync(lower_file, lower_dentry,
-					       datasync);
-		mutex_unlock(&lower_inode->i_mutex);
-	}
-	return rc;
+	return vfs_fsync(ecryptfs_file_to_lower(file),
+			 ecryptfs_dentry_to_lower(dentry),
+			 datasync);
 }
 
 static int ecryptfs_fasync(int fd, struct file *file, int flag)

Re: [PATCH, RFC] add a vfs_fsync helper

[Nick Piggin]

On Mon, Dec 22, 2008 at 10:03:57AM +0100, Christoph Hellwig wrote:
> Fsync currently has a fdatawrite/fdatawait pair around the method call,
> and a mutex_lock/unlock of the inode mutex.  All callers of fsync have
> to duplicate this, but we have a few and most of them don't quite get
> it right.  This patch adds a new vfs_fsync that takes care of this.
> It's a little more complicated as usual as ->fsync might get a NULL file
> pointer and just a dentry from nfsd, but otherwise gets afile and we
> want to take the mapping and file operations from it when it is there.
> 
> Notes on the fsync callers:
> 
>  - ecryptfs wasn't calling filemap_fdatawrite / filemap_fdatawait on the
>    	lower file
>  - coda wasn't calling filemap_fdatawrite / filemap_fdatawait on the host
> 	file, and returning 0 when ->fsync was missing
>  - shm wasn't calling either filemap_fdatawrite / filemap_fdatawait nor
>    taking i_mutex.  Now given that shared memory doesn't have disk
>    backing not doing anything in fsync seems fine and I left it out of
>    the vfs_fsync conversion for now, but in that case we might just
>    not pass it through to the lower file at all but just call the no-op
>    simple_sync_file directly.

Seems like an improvement.. I've wanted to know, though, what do
we need i_mutex for? Is it just convention, or is there some good
reason to have it in generic code?

Re: [PATCH, RFC] add a vfs_fsync helper

[Christoph Hellwig]

On Mon, Dec 22, 2008 at 01:35:34PM +0100, Nick Piggin wrote:
> Seems like an improvement.. I've wanted to know, though, what do
> we need i_mutex for? Is it just convention, or is there some good
> reason to have it in generic code?

At least XFS doesn't need it.  Same for the filemap_fdatawrite /
filemap_fdatawait which at least for filesystems that want to provide
integrity guarantees is in the wrong place.

This patch is a first one out my work to refactor fsync, and I'm trying
to feed it to mainline in pieces.  The next one will be to make sure
nfsd always has a struct file available when calling fsync, but I need
to do some extensive benchmarking.  After that we can change the
fsync prototype to drop the dentry paramters, and move the
filemap_fdatawrite / filemap_fdatawait aswell as the i_mutex locking
into the actual methods.

Re: [PATCH, RFC] add a vfs_fsync helper

[Nick Piggin]

On Mon, Dec 22, 2008 at 01:56:22PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 22, 2008 at 01:35:34PM +0100, Nick Piggin wrote:
> > Seems like an improvement.. I've wanted to know, though, what do
> > we need i_mutex for? Is it just convention, or is there some good
> > reason to have it in generic code?
> 
> At least XFS doesn't need it.  Same for the filemap_fdatawrite /
> filemap_fdatawait which at least for filesystems that want to provide
> integrity guarantees is in the wrong place.
> 
> This patch is a first one out my work to refactor fsync, and I'm trying
> to feed it to mainline in pieces.  The next one will be to make sure

OK, I gues that's fair enough. The generic pagecache layer as you know
shouldn't require i_mutex... so long as you have a plan in mind to
decouple that requirement from generic code I'm happy with it.


> nfsd always has a struct file available when calling fsync, but I need
> to do some extensive benchmarking.  After that we can change the
> fsync prototype to drop the dentry paramters, and move the
> filemap_fdatawrite / filemap_fdatawait aswell as the i_mutex locking
> into the actual methods.

They all sound like good steps I think. Well I have no really helpful
comments for your RFC except that I think it is a good idea. Probably
some filesystem-side people should have a look at it...

Re: [PATCH, RFC] add a vfs_fsync helper

[Al Viro]

On Mon, Dec 22, 2008 at 01:56:22PM +0100, Christoph Hellwig wrote:
> On Mon, Dec 22, 2008 at 01:35:34PM +0100, Nick Piggin wrote:
> > Seems like an improvement.. I've wanted to know, though, what do
> > we need i_mutex for? Is it just convention, or is there some good
> > reason to have it in generic code?
> 
> At least XFS doesn't need it.  Same for the filemap_fdatawrite /
> filemap_fdatawait which at least for filesystems that want to provide
> integrity guarantees is in the wrong place.
> 
> This patch is a first one out my work to refactor fsync, and I'm trying
> to feed it to mainline in pieces.  The next one will be to make sure
> nfsd always has a struct file available when calling fsync, but I need
> to do some extensive benchmarking.  After that we can change the
> fsync prototype to drop the dentry paramters, and move the
> filemap_fdatawrite / filemap_fdatawait aswell as the i_mutex locking
> into the actual methods.

I don't think that anything generic needs it anymore (if it ever did).
That was before my time (2.1.56, so it's probably Bill Hawes) and I
suspect that the reason had been along the lines of "we really don't
want to think about fsync() vs. truncate() races, let's just serialize
it".

[PATCH] add a vfs_fsync helper

[Christoph Hellwig]

Fsync currently has a fdatawrite/fdatawait pair around the method call,
and a mutex_lock/unlock of the inode mutex.  All callers of fsync have
to duplicate this, but we have a few and most of them don't quite get
it right.  This patch adds a new vfs_fsync that takes care of this.
It's a little more complicated as usual as ->fsync might get a NULL file
pointer and just a dentry from nfsd, but otherwise gets afile and we
want to take the mapping and file operations from it when it is there.
 
Notes on the fsync callers:
 
 - ecryptfs wasn't calling filemap_fdatawrite / filemap_fdatawait on the
   	lower file
 - coda wasn't calling filemap_fdatawrite / filemap_fdatawait on the host
	file, and returning 0 when ->fsync was missing
 - shm wasn't calling either filemap_fdatawrite / filemap_fdatawait nor
   taking i_mutex.  Now given that shared memory doesn't have disk
   backing not doing anything in fsync seems fine and I left it out of
   the vfs_fsync conversion for now, but in that case we might just
   not pass it through to the lower file at all but just call the no-op
   simple_sync_file directly.

[and now actually export vfs_fsync]


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

Index: xfs/fs/nfsd/vfs.c
===================================================================
--- xfs.orig/fs/nfsd/vfs.c	2008-12-22 18:01:32.367372702 +0100
+++ xfs/fs/nfsd/vfs.c	2008-12-22 18:13:29.210247796 +0100
@@ -743,45 +743,16 @@ nfsd_close(struct file *filp)
 	fput(filp);
 }
 
-/*
- * Sync a file
- * As this calls fsync (not fdatasync) there is no need for a write_inode
- * after it.
- */
-static inline int nfsd_dosync(struct file *filp, struct dentry *dp,
-			      const struct file_operations *fop)
-{
-	struct inode *inode = dp->d_inode;
-	int (*fsync) (struct file *, struct dentry *, int);
-	int err;
-
-	err = filemap_fdatawrite(inode->i_mapping);
-	if (err == 0 && fop && (fsync = fop->fsync))
-		err = fsync(filp, dp, 0);
-	if (err == 0)
-		err = filemap_fdatawait(inode->i_mapping);
-
-	return err;
-}
-	
-
 static int
 nfsd_sync(struct file *filp)
 {
-        int err;
-	struct inode *inode = filp->f_path.dentry->d_inode;
-	dprintk("nfsd: sync file %s\n", filp->f_path.dentry->d_name.name);
-	mutex_lock(&inode->i_mutex);
-	err=nfsd_dosync(filp, filp->f_path.dentry, filp->f_op);
-	mutex_unlock(&inode->i_mutex);
-
-	return err;
+	return vfs_fsync(filp, filp->f_path.dentry, 0);
 }
 
 int
-nfsd_sync_dir(struct dentry *dp)
+nfsd_sync_dir(struct dentry *dentry)
 {
-	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
+	return vfs_fsync(NULL, dentry, 0);
 }
 
 /*
Index: xfs/fs/sync.c
===================================================================
--- xfs.orig/fs/sync.c	2008-12-22 18:01:32.372372501 +0100
+++ xfs/fs/sync.c	2008-12-22 18:14:30.534247267 +0100
@@ -75,14 +75,39 @@ int file_fsync(struct file *filp, struct
 	return ret;
 }
 
-long do_fsync(struct file *file, int datasync)
+/**
+ * vfs_fsync - perform a fsync or fdatasync on a file
+ * @file:		file to sync
+ * @dentry:		dentry of @file
+ * @data:		only perform a fdatasync operation
+ *
+ * Write back data and metadata for @file to disk.  If @datasync is
+ * set only metadata needed to access modified file data is written.
+ *
+ * In case this function is called from nfsd @file may be %NULL and
+ * only @dentry is set.  This can only happen when the filesystem
+ * implements the export_operations API.
+ */
+int vfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 {
-	int ret;
-	int err;
-	struct address_space *mapping = file->f_mapping;
+	const struct file_operations *fop;
+	struct address_space *mapping;
+	int err, ret;
+
+	/*
+	 * Get mapping and operations from the file in case we have
+	 * as file, or get the default values for them in case we
+	 * don't have a struct file available.  Damn nfsd..
+	 */
+	if (file) {
+		mapping = file->f_mapping;
+		fop = file->f_op;
+	} else {
+		mapping = dentry->d_inode->i_mapping;
+		fop = dentry->d_inode->i_fop;
+	}
 
-	if (!file->f_op || !file->f_op->fsync) {
-		/* Why?  We can still call filemap_fdatawrite */
+	if (!fop || !fop->fsync) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -94,7 +119,7 @@ long do_fsync(struct file *file, int dat
 	 * livelocks in fsync_buffers_list().
 	 */
 	mutex_lock(&mapping->host->i_mutex);
-	err = file->f_op->fsync(file, file->f_path.dentry, datasync);
+	err = fop->fsync(file, dentry, datasync);
 	if (!ret)
 		ret = err;
 	mutex_unlock(&mapping->host->i_mutex);
@@ -104,15 +129,16 @@ long do_fsync(struct file *file, int dat
 out:
 	return ret;
 }
+EXPORT_SYMBOL(vfs_fsync);
 
-static long __do_fsync(unsigned int fd, int datasync)
+static int do_fsync(unsigned int fd, int datasync)
 {
 	struct file *file;
 	int ret = -EBADF;
 
 	file = fget(fd);
 	if (file) {
-		ret = do_fsync(file, datasync);
+		ret = vfs_fsync(file, file->f_path.dentry, datasync);
 		fput(file);
 	}
 	return ret;
@@ -120,12 +146,12 @@ static long __do_fsync(unsigned int fd, 
 
 asmlinkage long sys_fsync(unsigned int fd)
 {
-	return __do_fsync(fd, 0);
+	return do_fsync(fd, 0);
 }
 
 asmlinkage long sys_fdatasync(unsigned int fd)
 {
-	return __do_fsync(fd, 1);
+	return do_fsync(fd, 1);
 }
 
 /*
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2008-12-22 18:01:32.391372407 +0100
+++ xfs/include/linux/fs.h	2008-12-22 18:01:53.623372207 +0100
@@ -1827,7 +1827,7 @@ extern int __filemap_fdatawrite_range(st
 extern int filemap_fdatawrite_range(struct address_space *mapping,
 				loff_t start, loff_t end);
 
-extern long do_fsync(struct file *file, int datasync);
+extern int vfs_fsync(struct file *file, struct dentry *dentry, int datasync);
 extern void sync_supers(void);
 extern void sync_filesystems(int wait);
 extern void __fsync_super(struct super_block *sb);
Index: xfs/mm/msync.c
===================================================================
--- xfs.orig/mm/msync.c	2008-12-22 18:01:32.396372765 +0100
+++ xfs/mm/msync.c	2008-12-22 18:01:53.624372893 +0100
@@ -82,7 +82,7 @@ asmlinkage long sys_msync(unsigned long 
 				(vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&mm->mmap_sem);
-			error = do_fsync(file, 0);
+			error = vfs_fsync(file, file->f_path.dentry, 0);
 			fput(file);
 			if (error || start >= end)
 				goto out;
Index: xfs/drivers/usb/gadget/file_storage.c
===================================================================
--- xfs.orig/drivers/usb/gadget/file_storage.c	2008-12-22 18:01:32.405372585 +0100
+++ xfs/drivers/usb/gadget/file_storage.c	2008-12-22 18:20:54.054277912 +0100
@@ -1863,26 +1863,10 @@ static int do_write(struct fsg_dev *fsg)
 static int fsync_sub(struct lun *curlun)
 {
 	struct file	*filp = curlun->filp;
-	struct inode	*inode;
-	int		rc, err;
 
 	if (curlun->ro || !filp)
 		return 0;
-	if (!filp->f_op->fsync)
-		return -EINVAL;
-
-	inode = filp->f_path.dentry->d_inode;
-	mutex_lock(&inode->i_mutex);
-	rc = filemap_fdatawrite(inode->i_mapping);
-	err = filp->f_op->fsync(filp, filp->f_path.dentry, 1);
-	if (!rc)
-		rc = err;
-	err = filemap_fdatawait(inode->i_mapping);
-	if (!rc)
-		rc = err;
-	mutex_unlock(&inode->i_mutex);
-	VLDBG(curlun, "fdatasync -> %d\n", rc);
-	return rc;
+	return vfs_fsync(filp, filp->f_path.dentry, 1);
 }
 
 static void fsync_all(struct fsg_dev *fsg)
Index: xfs/fs/coda/file.c
===================================================================
--- xfs.orig/fs/coda/file.c	2008-12-22 18:01:32.379372066 +0100
+++ xfs/fs/coda/file.c	2008-12-22 18:01:53.626372520 +0100
@@ -200,8 +200,7 @@ int coda_release(struct inode *coda_inod
 int coda_fsync(struct file *coda_file, struct dentry *coda_dentry, int datasync)
 {
 	struct file *host_file;
-	struct dentry *host_dentry;
-	struct inode *host_inode, *coda_inode = coda_dentry->d_inode;
+	struct inode *coda_inode = coda_dentry->d_inode;
 	struct coda_file_info *cfi;
 	int err = 0;
 
@@ -213,14 +212,7 @@ int coda_fsync(struct file *coda_file, s
 	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
 	host_file = cfi->cfi_container;
 
-	if (host_file->f_op && host_file->f_op->fsync) {
-		host_dentry = host_file->f_path.dentry;
-		host_inode = host_dentry->d_inode;
-		mutex_lock(&host_inode->i_mutex);
-		err = host_file->f_op->fsync(host_file, host_dentry, datasync);
-		mutex_unlock(&host_inode->i_mutex);
-	}
-
+	err = vfs_fsync(host_file, host_file->f_path.dentry, datasync);
 	if ( !err && !datasync ) {
 		lock_kernel();
 		err = venus_fsync(coda_inode->i_sb, coda_i2f(coda_inode));
Index: xfs/fs/ecryptfs/file.c
===================================================================
--- xfs.orig/fs/ecryptfs/file.c	2008-12-22 18:01:32.385372900 +0100
+++ xfs/fs/ecryptfs/file.c	2008-12-22 18:01:53.627372577 +0100
@@ -275,18 +275,9 @@ static int ecryptfs_release(struct inode
 static int
 ecryptfs_fsync(struct file *file, struct dentry *dentry, int datasync)
 {
-	struct file *lower_file = ecryptfs_file_to_lower(file);
-	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
-	struct inode *lower_inode = lower_dentry->d_inode;
-	int rc = -EINVAL;
-
-	if (lower_inode->i_fop->fsync) {
-		mutex_lock(&lower_inode->i_mutex);
-		rc = lower_inode->i_fop->fsync(lower_file, lower_dentry,
-					       datasync);
-		mutex_unlock(&lower_inode->i_mutex);
-	}
-	return rc;
+	return vfs_fsync(ecryptfs_file_to_lower(file),
+			 ecryptfs_dentry_to_lower(dentry),
+			 datasync);
 }
 
 static int ecryptfs_fasync(int fd, struct file *file, int flag)

Re: [PATCH] add a vfs_fsync helper

[David Brownell]

Just cc'ing Alan Stern here, who wrote the usb/gadget code which
this simplifies.


On Monday 22 December 2008, Christoph Hellwig wrote:
> Fsync currently has a fdatawrite/fdatawait pair around the method call,
> and a mutex_lock/unlock of the inode mutex.  All callers of fsync have
> to duplicate this, but we have a few and most of them don't quite get
> it right.  This patch adds a new vfs_fsync that takes care of this.
> It's a little more complicated as usual as ->fsync might get a NULL file
> pointer and just a dentry from nfsd, but otherwise gets afile and we
> want to take the mapping and file operations from it when it is there.
>  
> Notes on the fsync callers:
>  
>  - ecryptfs wasn't calling filemap_fdatawrite / filemap_fdatawait on the
>    	lower file
>  - coda wasn't calling filemap_fdatawrite / filemap_fdatawait on the host
> 	file, and returning 0 when ->fsync was missing
>  - shm wasn't calling either filemap_fdatawrite / filemap_fdatawait nor
>    taking i_mutex.  Now given that shared memory doesn't have disk
>    backing not doing anything in fsync seems fine and I left it out of
>    the vfs_fsync conversion for now, but in that case we might just
>    not pass it through to the lower file at all but just call the no-op
>    simple_sync_file directly.
> 
> [and now actually export vfs_fsync]
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/nfsd/vfs.c
> ===================================================================
> --- xfs.orig/fs/nfsd/vfs.c	2008-12-22 18:01:32.367372702 +0100
> +++ xfs/fs/nfsd/vfs.c	2008-12-22 18:13:29.210247796 +0100
> @@ -743,45 +743,16 @@ nfsd_close(struct file *filp)
>  	fput(filp);
>  }
>  
> -/*
> - * Sync a file
> - * As this calls fsync (not fdatasync) there is no need for a write_inode
> - * after it.
> - */
> -static inline int nfsd_dosync(struct file *filp, struct dentry *dp,
> -			      const struct file_operations *fop)
> -{
> -	struct inode *inode = dp->d_inode;
> -	int (*fsync) (struct file *, struct dentry *, int);
> -	int err;
> -
> -	err = filemap_fdatawrite(inode->i_mapping);
> -	if (err == 0 && fop && (fsync = fop->fsync))
> -		err = fsync(filp, dp, 0);
> -	if (err == 0)
> -		err = filemap_fdatawait(inode->i_mapping);
> -
> -	return err;
> -}
> -	
> -
>  static int
>  nfsd_sync(struct file *filp)
>  {
> -        int err;
> -	struct inode *inode = filp->f_path.dentry->d_inode;
> -	dprintk("nfsd: sync file %s\n", filp->f_path.dentry->d_name.name);
> -	mutex_lock(&inode->i_mutex);
> -	err=nfsd_dosync(filp, filp->f_path.dentry, filp->f_op);
> -	mutex_unlock(&inode->i_mutex);
> -
> -	return err;
> +	return vfs_fsync(filp, filp->f_path.dentry, 0);
>  }
>  
>  int
> -nfsd_sync_dir(struct dentry *dp)
> +nfsd_sync_dir(struct dentry *dentry)
>  {
> -	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
> +	return vfs_fsync(NULL, dentry, 0);
>  }
>  
>  /*
> Index: xfs/fs/sync.c
> ===================================================================
> --- xfs.orig/fs/sync.c	2008-12-22 18:01:32.372372501 +0100
> +++ xfs/fs/sync.c	2008-12-22 18:14:30.534247267 +0100
> @@ -75,14 +75,39 @@ int file_fsync(struct file *filp, struct
>  	return ret;
>  }
>  
> -long do_fsync(struct file *file, int datasync)
> +/**
> + * vfs_fsync - perform a fsync or fdatasync on a file
> + * @file:		file to sync
> + * @dentry:		dentry of @file
> + * @data:		only perform a fdatasync operation
> + *
> + * Write back data and metadata for @file to disk.  If @datasync is
> + * set only metadata needed to access modified file data is written.
> + *
> + * In case this function is called from nfsd @file may be %NULL and
> + * only @dentry is set.  This can only happen when the filesystem
> + * implements the export_operations API.
> + */
> +int vfs_fsync(struct file *file, struct dentry *dentry, int datasync)
>  {
> -	int ret;
> -	int err;
> -	struct address_space *mapping = file->f_mapping;
> +	const struct file_operations *fop;
> +	struct address_space *mapping;
> +	int err, ret;
> +
> +	/*
> +	 * Get mapping and operations from the file in case we have
> +	 * as file, or get the default values for them in case we
> +	 * don't have a struct file available.  Damn nfsd..
> +	 */
> +	if (file) {
> +		mapping = file->f_mapping;
> +		fop = file->f_op;
> +	} else {
> +		mapping = dentry->d_inode->i_mapping;
> +		fop = dentry->d_inode->i_fop;
> +	}
>  
> -	if (!file->f_op || !file->f_op->fsync) {
> -		/* Why?  We can still call filemap_fdatawrite */
> +	if (!fop || !fop->fsync) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -94,7 +119,7 @@ long do_fsync(struct file *file, int dat
>  	 * livelocks in fsync_buffers_list().
>  	 */
>  	mutex_lock(&mapping->host->i_mutex);
> -	err = file->f_op->fsync(file, file->f_path.dentry, datasync);
> +	err = fop->fsync(file, dentry, datasync);
>  	if (!ret)
>  		ret = err;
>  	mutex_unlock(&mapping->host->i_mutex);
> @@ -104,15 +129,16 @@ long do_fsync(struct file *file, int dat
>  out:
>  	return ret;
>  }
> +EXPORT_SYMBOL(vfs_fsync);
>  
> -static long __do_fsync(unsigned int fd, int datasync)
> +static int do_fsync(unsigned int fd, int datasync)
>  {
>  	struct file *file;
>  	int ret = -EBADF;
>  
>  	file = fget(fd);
>  	if (file) {
> -		ret = do_fsync(file, datasync);
> +		ret = vfs_fsync(file, file->f_path.dentry, datasync);
>  		fput(file);
>  	}
>  	return ret;
> @@ -120,12 +146,12 @@ static long __do_fsync(unsigned int fd, 
>  
>  asmlinkage long sys_fsync(unsigned int fd)
>  {
> -	return __do_fsync(fd, 0);
> +	return do_fsync(fd, 0);
>  }
>  
>  asmlinkage long sys_fdatasync(unsigned int fd)
>  {
> -	return __do_fsync(fd, 1);
> +	return do_fsync(fd, 1);
>  }
>  
>  /*
> Index: xfs/include/linux/fs.h
> ===================================================================
> --- xfs.orig/include/linux/fs.h	2008-12-22 18:01:32.391372407 +0100
> +++ xfs/include/linux/fs.h	2008-12-22 18:01:53.623372207 +0100
> @@ -1827,7 +1827,7 @@ extern int __filemap_fdatawrite_range(st
>  extern int filemap_fdatawrite_range(struct address_space *mapping,
>  				loff_t start, loff_t end);
>  
> -extern long do_fsync(struct file *file, int datasync);
> +extern int vfs_fsync(struct file *file, struct dentry *dentry, int datasync);
>  extern void sync_supers(void);
>  extern void sync_filesystems(int wait);
>  extern void __fsync_super(struct super_block *sb);
> Index: xfs/mm/msync.c
> ===================================================================
> --- xfs.orig/mm/msync.c	2008-12-22 18:01:32.396372765 +0100
> +++ xfs/mm/msync.c	2008-12-22 18:01:53.624372893 +0100
> @@ -82,7 +82,7 @@ asmlinkage long sys_msync(unsigned long 
>  				(vma->vm_flags & VM_SHARED)) {
>  			get_file(file);
>  			up_read(&mm->mmap_sem);
> -			error = do_fsync(file, 0);
> +			error = vfs_fsync(file, file->f_path.dentry, 0);
>  			fput(file);
>  			if (error || start >= end)
>  				goto out;
> Index: xfs/drivers/usb/gadget/file_storage.c
> ===================================================================
> --- xfs.orig/drivers/usb/gadget/file_storage.c	2008-12-22 18:01:32.405372585 +0100
> +++ xfs/drivers/usb/gadget/file_storage.c	2008-12-22 18:20:54.054277912 +0100
> @@ -1863,26 +1863,10 @@ static int do_write(struct fsg_dev *fsg)
>  static int fsync_sub(struct lun *curlun)
>  {
>  	struct file	*filp = curlun->filp;
> -	struct inode	*inode;
> -	int		rc, err;
>  
>  	if (curlun->ro || !filp)
>  		return 0;
> -	if (!filp->f_op->fsync)
> -		return -EINVAL;
> -
> -	inode = filp->f_path.dentry->d_inode;
> -	mutex_lock(&inode->i_mutex);
> -	rc = filemap_fdatawrite(inode->i_mapping);
> -	err = filp->f_op->fsync(filp, filp->f_path.dentry, 1);
> -	if (!rc)
> -		rc = err;
> -	err = filemap_fdatawait(inode->i_mapping);
> -	if (!rc)
> -		rc = err;
> -	mutex_unlock(&inode->i_mutex);
> -	VLDBG(curlun, "fdatasync -> %d\n", rc);
> -	return rc;
> +	return vfs_fsync(filp, filp->f_path.dentry, 1);
>  }
>  
>  static void fsync_all(struct fsg_dev *fsg)
> Index: xfs/fs/coda/file.c
> ===================================================================
> --- xfs.orig/fs/coda/file.c	2008-12-22 18:01:32.379372066 +0100
> +++ xfs/fs/coda/file.c	2008-12-22 18:01:53.626372520 +0100
> @@ -200,8 +200,7 @@ int coda_release(struct inode *coda_inod
>  int coda_fsync(struct file *coda_file, struct dentry *coda_dentry, int datasync)
>  {
>  	struct file *host_file;
> -	struct dentry *host_dentry;
> -	struct inode *host_inode, *coda_inode = coda_dentry->d_inode;
> +	struct inode *coda_inode = coda_dentry->d_inode;
>  	struct coda_file_info *cfi;
>  	int err = 0;
>  
> @@ -213,14 +212,7 @@ int coda_fsync(struct file *coda_file, s
>  	BUG_ON(!cfi || cfi->cfi_magic != CODA_MAGIC);
>  	host_file = cfi->cfi_container;
>  
> -	if (host_file->f_op && host_file->f_op->fsync) {
> -		host_dentry = host_file->f_path.dentry;
> -		host_inode = host_dentry->d_inode;
> -		mutex_lock(&host_inode->i_mutex);
> -		err = host_file->f_op->fsync(host_file, host_dentry, datasync);
> -		mutex_unlock(&host_inode->i_mutex);
> -	}
> -
> +	err = vfs_fsync(host_file, host_file->f_path.dentry, datasync);
>  	if ( !err && !datasync ) {
>  		lock_kernel();
>  		err = venus_fsync(coda_inode->i_sb, coda_i2f(coda_inode));
> Index: xfs/fs/ecryptfs/file.c
> ===================================================================
> --- xfs.orig/fs/ecryptfs/file.c	2008-12-22 18:01:32.385372900 +0100
> +++ xfs/fs/ecryptfs/file.c	2008-12-22 18:01:53.627372577 +0100
> @@ -275,18 +275,9 @@ static int ecryptfs_release(struct inode
>  static int
>  ecryptfs_fsync(struct file *file, struct dentry *dentry, int datasync)
>  {
> -	struct file *lower_file = ecryptfs_file_to_lower(file);
> -	struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
> -	struct inode *lower_inode = lower_dentry->d_inode;
> -	int rc = -EINVAL;
> -
> -	if (lower_inode->i_fop->fsync) {
> -		mutex_lock(&lower_inode->i_mutex);
> -		rc = lower_inode->i_fop->fsync(lower_file, lower_dentry,
> -					       datasync);
> -		mutex_unlock(&lower_inode->i_mutex);
> -	}
> -	return rc;
> +	return vfs_fsync(ecryptfs_file_to_lower(file),
> +			 ecryptfs_dentry_to_lower(dentry),
> +			 datasync);
>  }
>  
>  static int ecryptfs_fasync(int fd, struct file *file, int flag)
> 
>