2007-10-26 17:32:45

by Chuck Lever III

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

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 <[email protected]>
---

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2007-10-29 14:58:54

by Talpey, Thomas

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

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 <[email protected]>
>---
>
> 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 - [email protected]
>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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs