Linux CIFS filesystem development
 help / color / mirror / Atom feed
From: Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
To: linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: [PATCH] Always update the dentry cache with fresh readdir() results
Date: Thu, 05 Jul 2012 18:38:27 +1000	[thread overview]
Message-ID: <1341477507.22307.21.camel@obed> (raw)

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

When we do a readdir() in CIFS, we are potentially efficiently
collecting a great deal of current, catchable stat information.

It is important that we always keep the dentry cache current for two
reasons:
 - the information may have changed (within the actime timeout).
 - if we still have a dentry cache value after that timeout, it is quite
expensive (1xRTT per entry) to find out if it was still correct.

This hits folks who are using CIFS over a WAN very badly.  For example
on an emulated 50ms delay I would have ls --color complete in .1
seconds, and a second run take 4.5 seconds, as each stat() (for the
colouring) would create a trans2 query_path_info query for each file,
right after getting the same information in the trans2 find_first2.

This patch implements the simplest approach, I would welcome a
correction on if there is a better approach than d_drop() and dput().

Tested on 3.4.4-3.cifsrevalidate.fc17.i686 with a 50ms WANem emulated
WAN against Samba 4.0 beta3.

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[-- Attachment #2: 0001-fs-cifs-Always-recreate-the-cifs-dentry-cache-when-w.patch --]
[-- Type: text/x-patch, Size: 2575 bytes --]

>From 4478a902d3606205313eb37a225da22712841bff Mon Sep 17 00:00:00 2001
From: Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Date: Thu, 5 Jul 2012 15:48:08 +1000
Subject: [PATCH] fs/cifs: Always recreate the cifs dentry cache when we have
 fresh information

When we do a readdir() in CIFS, we are potentially efficiently
collecting a great deal of current, cache-able stat information.

It is important that we always keep the dentry cache current for two reasons:
 - the information may have changed (within the actime timeout).
 - if we still have a dentry cache value after that timeout, it is quite
   expensive (1xRTT per entry) to find out if it was still correct.

This hits folks who are using CIFS over a WAN very badly.  For example
on an emulated 50ms delay I would have ls --color complete in .1
seconds, and a second run take 4.5 seconds, as each stat() (for the
colouring) would create a trans2 query_path_info query for each file,
right after getting the same information in the trans2 find_first2.

This patch implements the simplest approach, I would welcome a
correction on if there is a better approach than d_drop() and dput().

Tested on 3.4.4-3.cifsrevalidate.fc17.i686 against Samba 4.0 beta3.

Andrew Bartlett

Signed-off-by: Andrew Bartlett <abartlet-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/readdir.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index 0a8224d..f964b40 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -66,9 +66,13 @@ static inline void dump_cifs_file_struct(struct file *file, char *label)
 #endif /* DEBUG2 */
 
 /*
- * Find the dentry that matches "name". If there isn't one, create one. If it's
- * a negative dentry or the uniqueid changed, then drop it and recreate it.
+ * Find the dentry that matches "name". If there isn't one, create
+ * one.  Otherwise recreate it fresh so we start the timeout for
+ * revalidation again.  Local locks are much faster than the network
+ * ops to revalidate it, particularly as revalidation will be
+ * per-inode, but here we have a whole directory at once!
  */
+
 static struct dentry *
 cifs_readdir_lookup(struct dentry *parent, struct qstr *name,
 		    struct cifs_fattr *fattr)
@@ -86,9 +90,6 @@ cifs_readdir_lookup(struct dentry *parent, struct qstr *name,
 
 	dentry = d_lookup(parent, name);
 	if (dentry) {
-		/* FIXME: check for inode number changes? */
-		if (dentry->d_inode != NULL)
-			return dentry;
 		d_drop(dentry);
 		dput(dentry);
 	}
-- 
1.7.7.6


             reply	other threads:[~2012-07-05  8:38 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-07-05  8:38 Andrew Bartlett [this message]
2012-07-05 10:02 ` [PATCH] Always update the dentry cache with fresh readdir() results Andrew Bartlett
2012-07-05 11:24   ` Jeff Layton
     [not found]     ` <20120705072401.7eb1a7ee-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-07-05 23:31       ` Andrew Bartlett
2012-07-06  1:46         ` Jeff Layton
     [not found]           ` <20120705214608.2a3a681b-4QP7MXygkU+dMjc06nkz3ljfA9RmPOcC@public.gmane.org>
2012-07-06  6:20             ` Andrew Bartlett
2012-07-06 11:03               ` Jeff Layton
2012-07-06  6:30       ` Andrew Bartlett
2012-07-06 11:11         ` Jeff Layton
     [not found]           ` <20120706071123.2563c615-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org>
2012-07-06 22:42             ` Andrew Bartlett

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1341477507.22307.21.camel@obed \
    --to=abartlet-eunubhrolfbytjvyw6ydsg@public.gmane.org \
    --cc=linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox