[v4,24/28] NFS: Getattr doesn't require data sync semantics

Commit Message

Christoph Hellwig July 18, 2016, 3:48 a.m. UTC

On Wed, Jul 06, 2016 at 06:30:01PM -0400, Trond Myklebust wrote:
> When retrieving stat() information, NFS unfortunately does require us to
> sync writes to disk in order to ensure that mtime and ctime are up to
> date. However we shouldn't have to ensure that those writes are persisted.
> 
> Relaxing that requirement does mean that we may see an mtime/ctime change
> if the server reboots and forces us to replay all writes.
> 
> The exception to this rule are pNFS clients that are required to send
> layoutcommit, however that is dealt with by the call to pnfs_sync_inode()
> in _nfs_revalidate_inode().

This one breaks xfstests generic/207 on block/scsi layout for me.  The
reason for that is that we need a layoutcommit after writing out all
data for the file for the file size to be updated on the server.

Below is my attempt to fix this by re-adding pnfs_sync_inode to
nfs_getattr.  The call in _nfs_revalidate_inode isn't enough as it
doesn't get called in most cases we care about.


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

Comments

Trond Myklebust July 18, 2016, 4:32 a.m. UTC | #1

Hi Christoph,

> On Jul 17, 2016, at 23:48, Christoph Hellwig <[e-mail]> wrote:
> 
> On Wed, Jul 06, 2016 at 06:30:01PM -0400, Trond Myklebust wrote:
>> When retrieving stat() information, NFS unfortunately does require us to
>> sync writes to disk in order to ensure that mtime and ctime are up to
>> date. However we shouldn't have to ensure that those writes are persisted.
>> 
>> Relaxing that requirement does mean that we may see an mtime/ctime change
>> if the server reboots and forces us to replay all writes.
>> 
>> The exception to this rule are pNFS clients that are required to send
>> layoutcommit, however that is dealt with by the call to pnfs_sync_inode()
>> in _nfs_revalidate_inode().
> 
> This one breaks xfstests generic/207 on block/scsi layout for me.  The
> reason for that is that we need a layoutcommit after writing out all
> data for the file for the file size to be updated on the server.
> 
> Below is my attempt to fix this by re-adding pnfs_sync_inode to
> nfs_getattr.  The call in _nfs_revalidate_inode isn't enough as it
> doesn't get called in most cases we care about.
> 

I’m not understanding this argument. Why do we care if the file size is up to date on the server if we’re not sending an actual GETATTR on the wire to retrieve the file size?

Cheers
  Trond

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

Patch

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 22a53ee..8bd04cf 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -660,11 +660,20 @@  int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
 	int err = 0;
 
 	trace_nfs_getattr_enter(inode);
-	/* Flush out writes to the server in order to update c/mtime.  */
+
+	/*
+	 * Flush out writes to the server in order to update c/mtime as well
+	 * as the file size.  In the pNFS case this also requires a
+	 * LAYOUTCOMMIT.
+	 */
 	if (S_ISREG(inode->i_mode)) {
 		err = filemap_write_and_wait(inode->i_mapping);
 		if (err)
 			goto out;
+
+		err = pnfs_sync_inode(inode, true);
+		if (err)
+			goto out;
 	}
 
 	/*