nfsd: Replace nfsd_sync() with vfs_fsync() and vfs_fsync_range()

From: Trond Myklebust <[email protected]>

Currently, the nfs server holds the inode->i_mutex across both the filemap_write_and_wait() call and the fsync() call itself. However we know that filemap_write_and_wait() is already safe against livelocks, so we only need to hold the mutex across the fsync() call.

Fix this by reusing vfs_fsync(), which already does the right thing. Also make sure that we use vfs_fsync_range() in the COMMIT operation, to improve the efficiency for clients that do specify a range.

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfsd/vfs.c |   58 +++++++++++++++++++++------------------------------------
 1 files changed, 21 insertions(+), 37 deletions(-)

fs/nfsd/vfs.c +10 -34

index c194793..e4568d6 100644
@@ -769,40 +769,17 @@ nfsd_close(struct file *filp)
 }
 
 /*
- * Sync a file
- * As this calls fsync (not fdatasync) there is no need for a write_inode
- * after it.
+ * Sync a directory
+ * returns 0 if the directory had no fsync method
  */
-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_write_and_wait(inode->i_mapping);
-	if (err == 0 && fop && (fsync = fop->fsync))
-		err = fsync(filp, dp, 0);
-	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;
-}
-
 int
 nfsd_sync_dir(struct dentry *dp)
 {
-	return nfsd_dosync(NULL, dp, dp->d_inode->i_fop);
+	int err;
+
+	dprintk("nfsd: sync directory %s\n", dp->d_name.name);
+	err = vfs_fsync(NULL, dp, 0);
+	return (err != -EINVAL) ? err : 0;
 }
 
 /*

@@ -1008,7 +985,9 @@ static int wait_for_concurrent_writes(struct file *file)
 
 	if (inode->i_state & I_DIRTY) {
 		dprintk("nfsd: write sync %d\n", task_pid_nr(current));
-		err = nfsd_sync(file);
+		err = vfs_fsync(file, file->f_path.dentry, 0);
+		if (err == -EINVAL)
+			err = 0;
 	}
 	last_ino = inode->i_ino;
 	last_dev = inode->i_sb->s_dev;

@@ -1156,8 +1135,6 @@ out:
 #ifdef CONFIG_NFSD_V3
 /*
  * Commit all pending writes to stable storage.
- * Strictly speaking, we could sync just the indicated file region here,
- * but there's currently no way we can ask the VFS to do so.
  *
  * Unfortunately we cannot lock the file to make sure we return full WCC
  * data to the client, as locking happens lower down in the filesystem.

@@ -1176,11 +1153,18 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
 	if (err)
 		return err;
 	if (EX_ISSYNC(fhp->fh_export)) {
-		if (file->f_op && file->f_op->fsync) {

From: Trond Myklebust <[email protected]>

Currently, the nfs server holds the inode->i_mutex across both the filemap_write_and_wait() call and the fsync() call itself. However we know that filemap_write_and_wait() is already safe against livelocks, so we only need to hold the mutex across the fsync() call.

Fix this by reusing vfs_fsync(), which already does the right thing. Also make sure that we use vfs_fsync_range() in the COMMIT operation, to improve the efficiency for clients that do specify a range.

Quoted 7 lines · Trond Myklebust

From: Trond Myklebust <[email protected]>

Currently, the nfs server holds the inode->i_mutex across both the filemap_write_and_wait() call and the fsync() call itself. However we know that filemap_write_and_wait() is already safe against livelocks, so we only need to hold the mutex across the fsync() call.

Quoted 3 lines

Fix this by reusing vfs_fsync(), which already does the right thing. Also make sure that we use vfs_fsync_range() in the COMMIT operation, to improve the efficiency for clients that do specify a range.

I already sent a patch to replace nfsd_sync with it that should be queued up. We can't use vfs_fsync for nfsd_sync_dir as we're already holding i_mutex when calling it. The vfs_fsync_range optimizations seems like something that should be applied ontop, though.

OK. I missed your patch as it flew by on the list, but I assume it is this one:

http://git.linux-nfs.org/?p=bfields/linux.git;a=commitdiff;h=6a68f89ee1f2d177af4a5410fa7a45734c975fd6;hp=de3cab793c6a5c8505d66bee111edcc7098380ba

I'll separate out the vfs_sync_range() changes and cobble up a patch on top of the above changeset...

Cheers
Trond

nfsd: Use vfs_fsync_range() in nfsd_commit

From: Trond Myklebust <[email protected]>

The NFS COMMIT operation allows the client to specify the exact byte range that it wishes to sync to disk in order to optimise server performance.

Signed-off-by Trond Myklebust <[email protected]>

* Commit all pending writes to stable storage.
- * Strictly speaking, we could sync just the indicated file region here,
- * but there's currently no way we can ask the VFS to do so.

Quoted 7 lines · Trond Myklebust

nfsd: Use vfs_fsync_range() in nfsd_commit

From: Trond Myklebust <[email protected]>

The NFS COMMIT operation allows the client to specify the exact byte range that it wishes to sync to disk in order to optimise server performance.

Quoted 1 line

Signed-off-by Trond Myklebust <[email protected]>

Looks good to me,

Reviewed-by Christoph Hellwig <[email protected]>

Quoted 3 lines

* Commit all pending writes to stable storage.
- * Strictly speaking, we could sync just the indicated file region here,
- * but there's currently no way we can ask the VFS to do so.

The commit above could use some addition that the commit only happens for the specified range.

And not actually touched by your patch, but that is the reason to open/close the file if we don't actually do anything with it for an async export?

nfsd: Use vfs_fsync_range() in nfsd_commit

From: Trond Myklebust <[email protected]>

The NFS COMMIT operation allows the client to specify the exact byte range that it wishes to sync to disk in order to optimise server performance.

Signed-off-by Trond Myklebust <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---

 fs/nfsd/vfs.c |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

fs/nfsd/vfs.c +20 -10

index 79d216f..ed024d3 100644
@@ -1141,8 +1141,9 @@ out:
 #ifdef CONFIG_NFSD_V3
 /*
  * Commit all pending writes to stable storage.
- * Strictly speaking, we could sync just the indicated file region here,
- * but there's currently no way we can ask the VFS to do so.
+ *
+ * Note: we only guarantee that data that lies within the range specified
+ * by the 'offset' and 'count' parameters will be synced.
  *
  * Unfortunately we cannot lock the file to make sure we return full WCC
  * data to the client, as locking happens lower down in the filesystem.

@@ -1152,23 +1153,32 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
                loff_t offset, unsigned long count)
 {
 	struct file	*file;
-	__be32		err;
+	loff_t		end = LLONG_MAX;
+	__be32		err = nfserr_inval;
 
-	if ((u64)count > ~(u64)offset)
-		return nfserr_inval;
+	if (offset < 0)
+		goto out;
+	if (count != 0) {
+		end = offset + (loff_t)count - 1;
+		if (end < offset)
+			goto out;
+	}
 
 	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE, &file);
 	if (err)
-		return err;
+		goto out;
 	if (EX_ISSYNC(fhp->fh_export)) {
-		if (file->f_op && file->f_op->fsync) {
-			err = nfserrno(vfs_fsync(file, file->f_path.dentry, 0));
-		} else {
+		int err2 = vfs_fsync_range(file, file->f_path.dentry,
+				offset, end, 0);
+
+		if (err2 != -EINVAL)
+			err = nfserrno(err2);
+		else
 			err = nfserr_notsupp;
-		}
 	}
 
 	nfsd_close(file);
+out:
 	return err;
 }
 #endif /* CONFIG_NFSD_V3 */

Quoted 4 lines · Trond Myklebust

nfsd: Use vfs_fsync_range() in nfsd_commit

From: Trond Myklebust <[email protected]>

Quoted 5 lines

The NFS COMMIT operation allows the client to specify the exact byte range that it wishes to sync to disk in order to optimise server performance.

Signed-off-by Trond Myklebust <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Applied, thanks!

--b.

Quoted 64 lines

---

 fs/nfsd/vfs.c |   30 ++++++++++++++++++++----------
 1 files changed, 20 insertions(+), 10 deletions(-)

fs/nfsd/vfs.c +20 -10

index 79d216f..ed024d3 100644
@@ -1141,8 +1141,9 @@ out:
 #ifdef CONFIG_NFSD_V3
 /*
  * Commit all pending writes to stable storage.
- * Strictly speaking, we could sync just the indicated file region here,
- * but there's currently no way we can ask the VFS to do so.
+ *
+ * Note: we only guarantee that data that lies within the range specified
+ * by the 'offset' and 'count' parameters will be synced.
  *
  * Unfortunately we cannot lock the file to make sure we return full WCC
  * data to the client, as locking happens lower down in the filesystem.

@@ -1152,23 +1153,32 @@ nfsd_commit(struct svc_rqst *rqstp, struct svc_fh *fhp,
                loff_t offset, unsigned long count)
 {
 	struct file	*file;
-	__be32		err;
+	loff_t		end = LLONG_MAX;
+	__be32		err = nfserr_inval;
 
-	if ((u64)count > ~(u64)offset)
-		return nfserr_inval;
+	if (offset < 0)
+		goto out;
+	if (count != 0) {
+		end = offset + (loff_t)count - 1;
+		if (end < offset)
+			goto out;
+	}
 
 	err = nfsd_open(rqstp, fhp, S_IFREG, NFSD_MAY_WRITE, &file);
 	if (err)
-		return err;
+		goto out;
 	if (EX_ISSYNC(fhp->fh_export)) {
-		if (file->f_op && file->f_op->fsync) {
-			err = nfserrno(vfs_fsync(file, file->f_path.dentry, 0));
-		} else {
+		int err2 = vfs_fsync_range(file, file->f_path.dentry,
+				offset, end, 0);
+
+		if (err2 != -EINVAL)
+			err = nfserrno(err2);
+		else
 			err = nfserr_notsupp;
-		}
 	}
 
 	nfsd_close(file);
+out:
 	return err;
 }
 #endif /* CONFIG_NFSD_V3 */

Quoted 3 lines · Christoph Hellwig

And not actually touched by your patch, but that is the reason to open/close the file if we don't actually do anything with it for an async export?

I must admit that I was wondering about that too. I'm assuming that the reason is to provide consistent behaviour w.r.t. access checks and possibly also to ensure that NFSv4 delegations are revoked. Perhaps Bruce could comment?

Quoted 9 lines · Trond Myklebust

Quoted 3 lines · Christoph Hellwig

And not actually touched by your patch, but that is the reason to open/close the file if we don't actually do anything with it for an async export?

I must admit that I was wondering about that too. I'm assuming that the reason is to provide consistent behaviour w.r.t. access checks and possibly also to ensure that NFSv4 delegations are revoked. Perhaps Bruce could comment?

Do delegations need to be revoked on commit? (And do we care about access checks?)

We could do both without the need for an actual open, but I don't know that it matters much either way.

Is there any reason that we need to revoke the delegation on a commit?

The only real effect is that writes would be flushed to stable storage. The VM on the server could just decide the flush the data to stable storage whenever it feels like it without revoking the lease. I don't see why we'd need to revoke the lease just because a client asked for the flush.

Bruce has already chimed in on it, but a deadlock of sorts has popped up relating to this:

    https://bugzilla.redhat.com/show_bug.cgi?id=551028#c10

...and it seems like not recalling the delegation on a COMMIT might help resolve it.