Always update the dentry cache with fresh readdir() results

From 4478a902d3606205313eb37a225da22712841bff Mon Sep 17 00:00:00 2001

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

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

-------------------------[snip]--------------------------

cifs: always update the inode cache with the results from a FIND_*

When we get back a FIND_FIRST/NEXT result, we have some info about the dentry that we use to instantiate a new inode. We were ignoring and discarding that info when we had an existing dentry in the cache.

Fix this by updating the inode in place when we find an existing dentry and the uniqueid is the same.

Cc <[email protected]> # .31.x

Reported-by Andrew Bartlett <[email protected]>

Reported-by Bill Robertson <[email protected]>

Reported-by Dion Edwards <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/cifs/readdir.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

fs/cifs/readdir.c +5 -2

index 0a8224d..a4217f0 100644
@@ -86,9 +86,12 @@ 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)
+		inode = dentry->d_inode;
+		/* update inode in place if i_ino didn't change */
+		if (inode && CIFS_I(inode)->uniqueid == fattr->cf_uniqueid) {
+			cifs_fattr_to_inode(inode, fattr);
 			return dentry;
+		}
 		d_drop(dentry);
 		dput(dentry);
 	}

Quoted 36 lines · Jeff Layton

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Quoted 2 lines

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Is it really safe to update so much without getting a lock over all the updates?

Quoted 37 lines · On Fri, 06 Jul 2012 09:31:07 +1000 Andrew Bartlett…

Quoted 36 lines · Jeff Layton

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Quoted 6 lines

Quoted 2 lines

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Is it really safe to update so much without getting a lock over all the updates?

What's your worry, specifically?

The vfs only requires that you hold the lock over i_size updates. I suppose it's possible that you could have racing updates to an inode, but in practice, the last one will generally "win".

Quoted 47 lines · Jeff Layton

Quoted 37 lines · On Fri, 06 Jul 2012 09:31:07 +1000 Andrew Bartlett…

Quoted 36 lines · Jeff Layton

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Quoted 6 lines

Quoted 2 lines

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Is it really safe to update so much without getting a lock over all the updates?

Quoted 5 lines

What's your worry, specifically?

The vfs only requires that you hold the lock over i_size updates. I suppose it's possible that you could have racing updates to an inode, but in practice, the last one will generally "win".

Writes are a worry, and I'm not sure I like the idea of parallel updates being able to leave it in a undefined state, but I was more worried about a read, that is someone reading just between:

	inode->i_uid = fattr->cf_uid;
	inode->i_gid = fattr->cf_gid;

and

	/* if dynperm is set, don't clobber existing mode */
	if (inode->i_state & I_NEW ||
	    !(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_DYNPERM))
		inode->i_mode = fattr->cf_mode;

Imagine that this file was changing owner, and was setuid. Don't we have a race here were a very lucky caller could get the entry from the dentry cache between the uid change and the permission change (setuid removal)?

The race is very narrow, and most of the worries are much more mundane, but isn't this why you would lock the inode for the whole update?

I'm not fully up on kernel locking rules, which is why I looked to NFS for the example I mentioned.

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Quoted 36 lines · Jeff Layton

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Quoted 2 lines

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Yes, same behaviour as as per my patch. Thanks you very much, it seems we are on our way to solving this!

Quoted 40 lines · On Fri, 06 Jul 2012 16:30:12 +1000 Andrew Bartlett…

Quoted 38 lines · Jeff Layton

Quoted 28 lines · On Thu, 05 Jul 2012 20:02:47 +1000 Andrew Bartlett…

(CCing in the original reporter)

Quoted 24 lines · Andrew Bartlett

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

Nice work tracking that down and coding up the patch. While it's not incorrect to drop the dentry here, we can be a little more efficient here and just update the inode in place if the uniqueid didn't change.

Something like this (untested) patch should do it. Could you test this and let me know if it also helps?

Quoted 3 lines

Yes, same behaviour as as per my patch. Thanks you very much, it seems we are on our way to solving this!

Thanks for testing it. I've gone ahead and sent that off to Steve for inclusion. If there is anything else required wrt serializing the inode attribute updates, then we can deal with that in a separate patch since that's really a separate problem.