fix nfsd stable write implementation

PATCH In review J. Bruce Fields 3 participants 7 messages Oct 26, 2012 – Nov 8, 2012 2 messages not in the archive

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2012-10-26 21:06 UTC #0 lore raw

cover fix nfsd stable write implementation

From: "J. Bruce Fields" <[email protected]>

Peter pointed out to me that the nfs server is implementing stable writes by setting the O_SYNC flag. I can't see why we couldn't write and then sync instead, but I don't know this stuff as well as I should; does the following look reasonable to people?

--b.

J. Bruce Fields (2):
nfsd: assume writeable exportabled filesystems have f_sync
nfsd: use vfs_fsync_range(), not O_SYNC, for stable writes

 fs/nfsd/vfs.c |   26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1351285617-20450-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

J. Bruce Fields

author

2012-10-26 21:06 UTC #2 lore raw

PATCH 1/2 nfsd: assume writeable exportabled filesystems have f_sync

From: "J. Bruce Fields" <[email protected]>

I don't really see how you could claim to support nfsd and not support fsync somehow.

And in practice a quick look through the exportable filesystems suggests the only ones without an ->fsync are read-only (efs, isofs, squashfs) or in-memory (shmem).

Also, performing a write and then returning an error if the sync fails (as we would do here in the wgather case) seems unhelpful to clients.

Also remove an incorrect comment.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   13 -------------
 1 file changed, 13 deletions(-)

fs/nfsd/vfs.c +0 ~~-13~~

index c120b48..ed3eb59 100644
@@ -1020,21 +1020,8 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 	inode = dentry->d_inode;
 	exp   = fhp->fh_export;
 
-	/*
-	 * Request sync writes if
-	 *  -	the sync export option has been set, or
-	 *  -	the client requested O_SYNC behavior (NFSv3 feature).
-	 *  -   The file system doesn't support fsync().
-	 * When NFSv2 gathered writes have been configured for this volume,
-	 * flushing the data to disk is handled separately below.
-	 */
 	use_wgather = (rqstp->rq_vers == 2) && EX_WGATHER(exp);
 
-	if (!file->f_op->fsync) {/* COMMIT3 cannot work */
-	       stable = 2;
-	       *stablep = 2; /* FILE_SYNC */
-	}
-
 	if (!EX_ISSYNC(exp))
 		stable = 0;
 	if (stable && !use_wgather) {

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

author

2012-10-26 21:06 UTC #3 lore raw

PATCH 2/2 nfsd: use vfs_fsync_range(), not O_SYNC, for stable writes

In reply to a message not in the archive

From: "J. Bruce Fields" <[email protected]>

NFSv4 shares the same struct file across multiple writes. (And we'd like NFSv2 and NFSv3 to do that as well some day.)

So setting O_SYNC on the struct file as a way to request a synchronous write doesn't work.

Instead, do a vfs_fsync_range() in that case.

Reported-by Peter Staubach <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/vfs.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

fs/nfsd/vfs.c +6 -7

index ed3eb59..b584205 100644
@@ -1024,11 +1024,6 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 
 	if (!EX_ISSYNC(exp))
 		stable = 0;
-	if (stable && !use_wgather) {
-		spin_lock(&file->f_lock);
-		file->f_flags |= O_SYNC;
-		spin_unlock(&file->f_lock);
-	}
 
 	/* Write the data. */
 	oldfs = get_fs(); set_fs(KERNEL_DS);

@@ -1044,8 +1039,12 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct file *file,
 	if (inode->i_mode & (S_ISUID | S_ISGID))
 		kill_suid(dentry);
 
-	if (stable && use_wgather)
-		host_err = wait_for_concurrent_writes(file);
+	if (stable) {
+		if (use_wgather)
+			host_err = wait_for_concurrent_writes(file);
+		else
+			host_err = vfs_fsync_range(file, offset, offset+*cnt, 0);
+	}
 
 out_nfserr:
 	dprintk("nfsd: write complete host_err=%d\n", host_err);

Signature · 6 lines

1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

NeilBrown

2012-10-29 23:28 UTC #4 lore raw

In reply to a message not in the archive

Quoted 6 lines · On Fri, 26 Oct 2012 17:06:55 -0400 "J. Bruce Fields" <[email protected]> wrote:

From: "J. Bruce Fields" <[email protected]>
Peter pointed out to me that the nfs server is implementing stable writes by setting the O_SYNC flag. I can't see why we couldn't write and then sync instead, but I don't know this stuff as well as I should; does the following look reasonable to people?

Bruce changed the code to implement stable writes by calling vfs_fsync_range(). I can't see why we couldn't use O_SYNC instead.

It seems like you are making a change just for the sake of making a change. Is there some reason that you think a separate 'sync' is more efficient than using O_SYNC ?

As a general principle, I think it is best to give the file system as much information as possible to that it can make whatever optimisation decisions that it wants to.

Setting O_SYNC gives the filesystem more information than not, because it allows it to change the behaviour of the 'write' request... though I don't know if any filesystem actually uses the information.

Why the change?

NeilBrown

Quoted 10 lines

--b.
J. Bruce Fields (2):
nfsd: assume writeable exportabled filesystems have f_sync
nfsd: use vfs_fsync_range(), not O_SYNC, for stable writes
 fs/nfsd/vfs.c |   26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

J. Bruce Fields

2012-10-30 14:07 UTC #5 lore raw

Quoted 10 lines · NeilBrown

Quoted 6 lines · On Fri, 26 Oct 2012 17:06:55 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
Peter pointed out to me that the nfs server is implementing stable writes by setting the O_SYNC flag. I can't see why we couldn't write and then sync instead, but I don't know this stuff as well as I should; does the following look reasonable to people?

Quoted 6 lines

Bruce changed the code to implement stable writes by calling vfs_fsync_range(). I can't see why we couldn't use O_SYNC instead.
It seems like you are making a change just for the sake of making a change. Is there some reason that you think a separate 'sync' is more efficient than using O_SYNC ?

Oh, sorry, see the changelog on the second patch: the problem is that the struct file can be shared across multiple writes in the NFSv4 case, so a single stable write could make all subsequent writes synchronous.

(And some day people would like filehandle caching for v2/v3, in which case we'll run into the same problem.)

Quoted 7 lines

As a general principle, I think it is best to give the file system as much information as possible to that it can make whatever optimisation decisions that it wants to.
Setting O_SYNC gives the filesystem more information than not, because it allows it to change the behaviour of the 'write' request... though I don't know if any filesystem actually uses the information.

I'm not sure how to figure out if that's a real problem or not.

--b.

A message that is not in the archive (sent off-list or not yet received): [email protected]

NeilBrown

2012-10-30 20:30 UTC #7 lore raw

Quoted 18 lines · On Tue, 30 Oct 2012 10:07:25 -0400 "J. Bruce Fields" <[email protected]>…

Quoted 10 lines · NeilBrown
Quoted 6 lines · On Fri, 26 Oct 2012 17:06:55 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
Peter pointed out to me that the nfs server is implementing stable writes by setting the O_SYNC flag. I can't see why we couldn't write and then sync instead, but I don't know this stuff as well as I should; does the following look reasonable to people?
Quoted 6 lines
Bruce changed the code to implement stable writes by calling vfs_fsync_range(). I can't see why we couldn't use O_SYNC instead.
It seems like you are making a change just for the sake of making a change. Is there some reason that you think a separate 'sync' is more efficient than using O_SYNC ?

Quoted 6 lines

Oh, sorry, see the changelog on the second patch: the problem is that the struct file can be shared across multiple writes in the NFSv4 case, so a single stable write could make all subsequent writes synchronous.
(And some day people would like filehandle caching for v2/v3, in which case we'll run into the same problem.)

Ahh, I missed that.
Makes sense then, thanks.

Quoted 10 lines

Quoted 8 lines
As a general principle, I think it is best to give the file system as much information as possible to that it can make whatever optimisation decisions that it wants to.
Setting O_SYNC gives the filesystem more information than not, because it allows it to change the behaviour of the 'write' request... though I don't know if any filesystem actually uses the information.
I'm not sure how to figure out if that's a real problem or not.

Search all filesystems for special handling of O_SYNC or O_DSYNC?

I strongly suspect that it is not a real problem.

Thanks,
NeilBrown

J. Bruce Fields

2012-11-08 00:21 UTC #8 lore raw

Quoted 40 lines · NeilBrown

Quoted 18 lines · On Tue, 30 Oct 2012 10:07:25 -0400 "J. Bruce Fields" <[email protected]>…
Quoted 10 lines · NeilBrown
Quoted 6 lines · On Fri, 26 Oct 2012 17:06:55 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
Peter pointed out to me that the nfs server is implementing stable writes by setting the O_SYNC flag. I can't see why we couldn't write and then sync instead, but I don't know this stuff as well as I should; does the following look reasonable to people?
Quoted 6 lines
Bruce changed the code to implement stable writes by calling vfs_fsync_range(). I can't see why we couldn't use O_SYNC instead.
It seems like you are making a change just for the sake of making a change. Is there some reason that you think a separate 'sync' is more efficient than using O_SYNC ?
Quoted 6 lines
Oh, sorry, see the changelog on the second patch: the problem is that the struct file can be shared across multiple writes in the NFSv4 case, so a single stable write could make all subsequent writes synchronous.
(And some day people would like filehandle caching for v2/v3, in which case we'll run into the same problem.)
Ahh, I missed that.
Makes sense then, thanks.
Quoted 9 lines
Quoted 8 lines
As a general principle, I think it is best to give the file system as much information as possible to that it can make whatever optimisation decisions that it wants to.
Setting O_SYNC gives the filesystem more information than not, because it allows it to change the behaviour of the 'write' request... though I don't know if any filesystem actually uses the information.

Quoted 5 lines

Quoted 1 line
I'm not sure how to figure out if that's a real problem or not.
Search all filesystems for special handling of O_SYNC or O_DSYNC?
I strongly suspect that it is not a real problem.

OK, committing for 3.8.

--b.