nfs: commit layouts in fdatasync

PATCH In review Christoph Hellwig 2 participants 5 messages Apr 21, 2014 – Jun 2, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Christoph Hellwig

author

2014-04-21 17:29 UTC #0 lore raw

PATCH 1/1 nfs: commit layouts in fdatasync

Quoted 1 line

From fdatasync(2):

"fdatasync() is similar to fsync(), but does not flush modified metadata
unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled."

We absolutely need to commit the layouts to be able to retrieve the data in case either the client, the server or the storage subsystem go down.

Signed-off-by Christoph Hellwig <[email protected]>

fs/nfs/nfs4file.c +1 -2

index 8de3407..464db9d 100644
@@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 			break;
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
-		if (!ret && !datasync)
-			/* application has asked for meta-data sync */
+		if (!ret)
 			ret = pnfs_layoutcommit_inode(inode, true);
 		mutex_unlock(&inode->i_mutex);
 		/*

Christoph Hellwig

author

2014-05-05 06:56 UTC #1 lore raw

ping? This is a fairly serious data integrity issue for pnfs users.

Quoted 26 lines · Christoph Hellwig

Quoted 1 line
From fdatasync(2):
"fdatasync() is similar to fsync(), but does not flush modified metadata
unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled."
We absolutely need to commit the layouts to be able to retrieve the data in case either the client, the server or the storage subsystem go down.
Signed-off-by Christoph Hellwig <[email protected]>
fs/nfs/nfs4file.c +1 -3
index 8de3407..464db9d 100644
@@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 			break;
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
-		if (!ret && !datasync)
-			/* application has asked for meta-data sync */
+		if (!ret)
 			ret = pnfs_layoutcommit_inode(inode, true);
 		mutex_unlock(&inode->i_mutex);
 		/*
--

Quoted 3 lines

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

---end quoted text---

Christoph Hellwig

author

2014-05-27 16:00 UTC #2 lore raw

Quoted 1 line · Christoph Hellwig

ping? This is a fairly serious data integrity issue for pnfs users.

ping^2

without this fdatasync and O_DSYNC writes on pnfs are more or less noops, so at least getting a review would be helpful, nevermind forwarding it to Linus and -stable.

Quoted 24 lines · Christoph Hellwig

Quoted 22 lines
Quoted 1 line
From fdatasync(2):
"fdatasync() is similar to fsync(), but does not flush modified metadata
unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled."
We absolutely need to commit the layouts to be able to retrieve the data in case either the client, the server or the storage subsystem go down.
Signed-off-by Christoph Hellwig <[email protected]>
fs/nfs/nfs4file.c +1 -2
index 8de3407..464db9d 100644
@@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 			break;
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
-		if (!ret && !datasync)
-			/* application has asked for meta-data sync */
+		if (!ret)

Quoted 8 lines

Quoted 7 lines
fs/nfs/nfs4file.c +0 -1
 			ret = pnfs_layoutcommit_inode(inode, true);
 		mutex_unlock(&inode->i_mutex);
 		/*
--
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
---end quoted text---

---end quoted text---

Trond Myklebust

2014-05-28 23:00 UTC #3 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 1 line · Christoph Hellwig
ping? This is a fairly serious data integrity issue for pnfs users.
ping^2
without this fdatasync and O_DSYNC writes on pnfs are more or less noops, so at least getting a review would be helpful, nevermind forwarding it to Linus and -stable.

Applied for 3.16. Not sure about -stable eligibility, since this only affects pnfs blocks for now (files and objects should both be able to recover in case of client failure). Are you seeing this in production environments?

Cheers
Trond

Quoted 32 lines

Quoted 23 lines · Christoph Hellwig
Quoted 22 lines
Quoted 1 line
From fdatasync(2):
"fdatasync() is similar to fsync(), but does not flush modified metadata
unless that metadata is needed in order to allow a subsequent data retrieval to be correctly handled."
We absolutely need to commit the layouts to be able to retrieve the data in case either the client, the server or the storage subsystem go down.
Signed-off-by Christoph Hellwig <[email protected]>
fs/nfs/nfs4file.c +1 -2
index 8de3407..464db9d 100644
@@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
                    break;
            mutex_lock(&inode->i_mutex);
            ret = nfs_file_fsync_commit(file, start, end, datasync);
-           if (!ret && !datasync)
-                   /* application has asked for meta-data sync */
+           if (!ret)
Quoted 8 lines
Quoted 7 lines
fs/nfs/nfs4file.c +0 -1
                    ret = pnfs_layoutcommit_inode(inode, true);
            mutex_unlock(&inode->i_mutex);
            /*
--
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
---end quoted text---
---end quoted text---

Signature · 5 lines

Trond Myklebust

Linux NFS client maintainer, PrimaryData

[email protected]

Christoph Hellwig

author

2014-06-02 08:19 UTC #4 lore raw

Quoted 3 lines · Trond Myklebust

Applied for 3.16. Not sure about -stable eligibility, since this only affects pnfs blocks for now (files and objects should both be able to recover in case of client failure).

At least the linux-nfs.org object server needs the layoutcommit to update i_size on the inode, so changes won't be persistent without the layoutcommit. I expect loosely coupled file servers to work in a similar fashion.

Quoted 2 lines

Are you seeing this in production
environments?

This was found during data integrity testing.