nfsd: use __fput_sync() to avoid delayed closing of files.

Calling fput() directly or though filp_close() from a kernel thread like
nfsd causes the final __fput() (if necessary) to be called from a
workqueue.  This means that nfsd is not forced to wait for any work to
complete.  If the ->release or ->destroy_inode function is slow for any
reason, this can result in nfsd closing files more quickly than the
workqueue can complete the close and the queue of pending closes can
grow without bounces (30 million has been seen at one customer site,
though this was in part due to a slowness in xfs which has since been
fixed).

nfsd does not need this.  It is quite appropriate and safe for nfsd to
do its own close work.  There is no reason that close should ever wait
for nfsd, so no deadlock can occur.

It should be safe and sensible to change all fput() calls to
__fput_sync().  However in the interests of caution this patch only
changes two - the two that can be most directly affected by client
behaviour and could occur at high frequency.

- the fput() implicitly in flip_close() is changed to __fput_sync()
  by calling get_file() first to ensure filp_close() doesn't do
  the final fput() itself.  If is where files opened for IO are closed.

- the fput() in nfsd_read() is also changed.  This is where directories
  opened for readdir are closed.

This ensure that minimal fput work is queued to the workqueue.

This removes the need for the flush_delayed_fput() call in
nfsd_file_close_inode_sync()

Signed-off-by: NeilBrown <neilb@suse.de>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index f8b100bca6e4dab80e6822f8ae475ce7474e7751..8d9f7b07e35b39bf5e9e392c9d98da10d2aaddb5 100644 (file)
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -280,7 +280,7 @@ nfsd_file_free(struct nfsd_file *nf)
                nfsd_file_mark_put(nf->nf_mark);
        if (nf->nf_file) {
                nfsd_file_check_write_error(nf);
-               filp_close(nf->nf_file, NULL);
+               nfsd_filp_close(nf->nf_file);
        }
 
        /*
@@ -658,7 +658,6 @@ nfsd_file_close_inode_sync(struct inode *inode)
                list_del_init(&nf->nf_lru);
                nfsd_file_free(nf);
        }
-       flush_delayed_fput();
 }
 
 static int

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index b7c7a9273ea01d9d84bcc2ea487bd6d886bc060d..f57749cd6f0b1ac778c7d257984c7ca2f428536b 100644 (file)
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1906,10 +1906,10 @@ out_unlock:
        fh_drop_write(ffhp);
 
        /*
-        * If the target dentry has cached open files, then we need to try to
-        * close them prior to doing the rename. Flushing delayed fput
-        * shouldn't be done with locks held however, so we delay it until this
-        * point and then reattempt the whole shebang.
+        * If the target dentry has cached open files, then we need to
+        * try to close them prior to doing the rename.  Final fput
+        * shouldn't be done with locks held however, so we delay it
+        * until this point and then reattempt the whole shebang.
         */
        if (close_cached) {
                close_cached = false;
@@ -2177,11 +2177,43 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
        if (err == nfserr_eof || err == nfserr_toosmall)
                err = nfs_ok; /* can still be found in ->err */
 out_close:
-       fput(file);
+       nfsd_filp_close(file);
 out:
        return err;
 }
 
+/**
+ * nfsd_filp_close: close a file synchronously
+ * @fp: the file to close
+ *
+ * nfsd_filp_close() is similar in behaviour to filp_close().
+ * The difference is that if this is the final close on the
+ * file, the that finalisation happens immediately, rather then
+ * being handed over to a work_queue, as it the case for
+ * filp_close().
+ * When a user-space process closes a file (even when using
+ * filp_close() the finalisation happens before returning to
+ * userspace, so it is effectively synchronous.  When a kernel thread
+ * uses file_close(), on the other hand, the handling is completely
+ * asynchronous.  This means that any cost imposed by that finalisation
+ * is not imposed on the nfsd thread, and nfsd could potentually
+ * close files more quickly than the work queue finalises the close,
+ * which would lead to unbounded growth in the queue.
+ *
+ * In some contexts is it not safe to synchronously wait for
+ * close finalisation (see comment for __fput_sync()), but nfsd
+ * does not match those contexts.  In partcilarly it does not, at the
+ * time that this function is called, hold and locks and no finalisation
+ * of any file, socket, or device driver would have any cause to wait
+ * for nfsd to make progress.
+ */
+void nfsd_filp_close(struct file *fp)
+{
+       get_file(fp);
+       filp_close(fp, NULL);
+       __fput_sync(fp);
+}
+
 /*
  * Get file system stats
  * N.B. After this call fhp needs an fh_put

diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 702fbc4483bf169098e297cd38c38f9176276c2d..1efa4e8dfb03495482caedb23ae69fedaea67398 100644 (file)
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -148,6 +148,8 @@ __be32              nfsd_statfs(struct svc_rqst *, struct svc_fh *,
 __be32         nfsd_permission(struct svc_rqst *, struct svc_export *,
                                struct dentry *, int);
 
+void           nfsd_filp_close(struct file *fp);
+
 static inline int fh_want_write(struct svc_fh *fh)
 {
        int ret;