[PATCH 18/27] NFS: Prevent nfs_getattr() hang during heavy write workloads

[PATCH 18/27] NFS: Prevent nfs_getattr() hang during heavy write workloads

[Chuck Lever]

POSIX requires that ctime and mtime, as reported by the stat(2) call,
reflect the activity of the most recent write(2).  To that end, nfs_getattr()
flushes pending dirty writes to a file before doing a GETATTR to allow the
NFS server to set the file's size, ctime, and mtime properly.

However, nfs_getattr() can be starved when a constant stream of application
writes to a file prevents nfs_wb_nocommit() from completing.  This usually
results in hangs of programs doing a stat against an NFS file that is being
written.  "ls -l" is a common victim of this behavior.

To prevent starvation, hold the file's i_mutex in nfs_getattr() to
freeze applications writes temporarily so the client can more quickly obtain
clean values for a file's size, mtime, and ctime.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/inode.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index cd0e57f..cc3a09d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -461,9 +461,18 @@ int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
 	int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
 	int err;
 
-	/* Flush out writes to the server in order to update c/mtime */
-	if (S_ISREG(inode->i_mode))
+	/*
+	 * Flush out writes to the server in order to update c/mtime.
+	 *
+	 * Hold the i_mutex to suspend application writes temporarily;
+	 * this prevents long-running writing applications from blocking
+	 * nfs_wb_nocommit.
+	 */
+	if (S_ISREG(inode->i_mode)) {
+		mutex_lock(&inode->i_mutex);
 		nfs_wb_nocommit(inode);
+		mutex_unlock(&inode->i_mutex);
+	}
 
 	/*
 	 * We may force a getattr if the user cares about atime.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 18/27] NFS: Prevent nfs_getattr() hang during heavy write workloads

[Talpey, Thomas]

Can you expound a little on whether holding a mutex across the
nfs_wb_nocommit() call is safe? In the hard-mount non-interruptible
case, isn't it possible that the writeback will take an unbounded time?

Tom.

At 01:32 PM 10/26/2007, Chuck Lever wrote:
>POSIX requires that ctime and mtime, as reported by the stat(2) call,
>reflect the activity of the most recent write(2).  To that end, nfs_getattr()
>flushes pending dirty writes to a file before doing a GETATTR to allow the
>NFS server to set the file's size, ctime, and mtime properly.
>
>However, nfs_getattr() can be starved when a constant stream of application
>writes to a file prevents nfs_wb_nocommit() from completing.  This usually
>results in hangs of programs doing a stat against an NFS file that is being
>written.  "ls -l" is a common victim of this behavior.
>
>To prevent starvation, hold the file's i_mutex in nfs_getattr() to
>freeze applications writes temporarily so the client can more quickly obtain
>clean values for a file's size, mtime, and ctime.
>
>Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>---
>
> fs/nfs/inode.c |   13 +++++++++++--
> 1 files changed, 11 insertions(+), 2 deletions(-)
>
>diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>index cd0e57f..cc3a09d 100644
>--- a/fs/nfs/inode.c
>+++ b/fs/nfs/inode.c
>@@ -461,9 +461,18 @@ int nfs_getattr(struct vfsmount *mnt, struct 
>dentry *dentry, struct kstat *stat)
> 	int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
> 	int err;
> 
>-	/* Flush out writes to the server in order to update c/mtime */
>-	if (S_ISREG(inode->i_mode))
>+	/*
>+	* Flush out writes to the server in order to update c/mtime.
>+	*
>+	* Hold the i_mutex to suspend application writes temporarily;
>+	* this prevents long-running writing applications from blocking
>+	* nfs_wb_nocommit.
>+	*/
>+	if (S_ISREG(inode->i_mode)) {
>+		mutex_lock(&inode->i_mutex);
> 		nfs_wb_nocommit(inode);
>+		mutex_unlock(&inode->i_mutex);
>+	}
> 
> 	/*
> 	* We may force a getattr if the user cares about atime.
>
>
>-------------------------------------------------------------------------
>This SF.net email is sponsored by: Splunk Inc.
>Still grepping through log files to find problems?  Stop.
>Now Search log events and configuration files using AJAX and a browser.
>Download your FREE copy of Splunk now >> http://get.splunk.com/
>_______________________________________________
>NFS maillist  -  NFS@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/nfs

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH 18/27] NFS: Prevent nfs_getattr() hang during heavy write workloads

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

Talpey, Thomas wrote:
> Can you expound a little on whether holding a mutex across the
> nfs_wb_nocommit() call is safe?   In the hard-mount non-interruptible
> case, isn't it possible that the writeback will take an unbounded time?

Without this change, nfs_wb_nocommit() will wait forever anyway in that 
case.  As applications continue queuing writes to that inode, eventually 
the mount point's bdi congestion logic kicks in and user space is forced 
to wait for that file as well.

With the change, the i_mutex prevents new application writes.  User 
space will stop and wait sooner, but the end result is the same.

> At 01:32 PM 10/26/2007, Chuck Lever wrote:
>> POSIX requires that ctime and mtime, as reported by the stat(2) call,
>> reflect the activity of the most recent write(2).  To that end, nfs_getattr()
>> flushes pending dirty writes to a file before doing a GETATTR to allow the
>> NFS server to set the file's size, ctime, and mtime properly.
>>
>> However, nfs_getattr() can be starved when a constant stream of application
>> writes to a file prevents nfs_wb_nocommit() from completing.  This usually
>> results in hangs of programs doing a stat against an NFS file that is being
>> written.  "ls -l" is a common victim of this behavior.
>>
>> To prevent starvation, hold the file's i_mutex in nfs_getattr() to
>> freeze applications writes temporarily so the client can more quickly obtain
>> clean values for a file's size, mtime, and ctime.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/nfs/inode.c |   13 +++++++++++--
>> 1 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index cd0e57f..cc3a09d 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -461,9 +461,18 @@ int nfs_getattr(struct vfsmount *mnt, struct 
>> dentry *dentry, struct kstat *stat)
>> 	int need_atime = NFS_I(inode)->cache_validity & NFS_INO_INVALID_ATIME;
>> 	int err;
>>
>> -	/* Flush out writes to the server in order to update c/mtime */
>> -	if (S_ISREG(inode->i_mode))
>> +	/*
>> +	* Flush out writes to the server in order to update c/mtime.
>> +	*
>> +	* Hold the i_mutex to suspend application writes temporarily;
>> +	* this prevents long-running writing applications from blocking
>> +	* nfs_wb_nocommit.
>> +	*/
>> +	if (S_ISREG(inode->i_mode)) {
>> +		mutex_lock(&inode->i_mutex);
>> 		nfs_wb_nocommit(inode);
>> +		mutex_unlock(&inode->i_mutex);
>> +	}
>>
>> 	/*
>> 	* We may force a getattr if the user cares about atime.
>>
>>
>> -------------------------------------------------------------------------
>> This SF.net email is sponsored by: Splunk Inc.
>> Still grepping through log files to find problems?  Stop.
>> Now Search log events and configuration files using AJAX and a browser.
>> Download your FREE copy of Splunk now >> http://get.splunk.com/
>> _______________________________________________
>> NFS maillist  -  NFS@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/nfs

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs