[PATCH] ecryptfs: remove unnecessary d_drop calls in ecryptfs_link

[PATCH] ecryptfs: remove unnecessary d_drop calls in ecryptfs_link

[Erez Zadok]

Tyler/Dustin,

We found three unnecessary calls to d_drop inside ecryptfs_link().  They are
called unconditionally whether ->link succeeds or not.  They are unnecessary
because they often remove valid objects from the dcache, which forces these
objects to be re-looked up the next time they're needed.  Presumably the
user process which called link(2) is likely to use those newly created
hardlinks right afterwards.  So removing them from the dcache hurts
performance.

We've tested the following patch in ecryptfs by successfully hard-linking a
few files, looking them up, deleting them, etc.  Nothing too intensive
thou.n However, we don't have those d_drop calls in unionfs, and it's been
working fine for years.  Moreover, we've tested with and without those three
d_drop calls in our wrapfs "pass-through" stackable file system with the
full ltp/racer/etc. suites and all's well.  Therefore, we're reasonably
certain that this patch is good.  Bug discovered by Aseem Rastogi during
code review.

Still, perhaps someone can recall some history and how come these d_drop
calls showed up in ecryptfs_link()?

Cheers,
Erez.

------------------------------------------------------------------------------

ecryptfs: remove unnecessary d_drop calls in wrapfs_link

Unnecessary because it would unhash perfectly valid dentries, causing them
to have to be re-looked up the next time they're needed, which presumably is
right after.

Signed-off-by: Aseem Rastogi <arastogi@cs.sunysb.edu>
Signed-off-by: Shrikar archak <shrikar84@gmail.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 186d744..2dc0bdb 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -463,9 +463,6 @@ out_lock:
 	unlock_dir(lower_dir_dentry);
 	dput(lower_new_dentry);
 	dput(lower_old_dentry);
-	d_drop(lower_old_dentry);
-	d_drop(new_dentry);
-	d_drop(old_dentry);
 	return rc;
 }
 

Re: [PATCH] ecryptfs: remove unnecessary d_drop calls in ecryptfs_link

[Tyler Hicks]

On Sun Dec 06, 2009 at 06:05:30PM -0500, Erez Zadok (ezk@cs.sunysb.edu) was quoted:
> Tyler/Dustin,
> 
> We found three unnecessary calls to d_drop inside ecryptfs_link().  They are
> called unconditionally whether ->link succeeds or not.  They are unnecessary
> because they often remove valid objects from the dcache, which forces these
> objects to be re-looked up the next time they're needed.  Presumably the
> user process which called link(2) is likely to use those newly created
> hardlinks right afterwards.  So removing them from the dcache hurts
> performance.
> 
> We've tested the following patch in ecryptfs by successfully hard-linking a
> few files, looking them up, deleting them, etc.  Nothing too intensive
> thou.n However, we don't have those d_drop calls in unionfs, and it's been
> working fine for years.  Moreover, we've tested with and without those three
> d_drop calls in our wrapfs "pass-through" stackable file system with the
> full ltp/racer/etc. suites and all's well.  Therefore, we're reasonably
> certain that this patch is good.  Bug discovered by Aseem Rastogi during
> code review.
> 
> Still, perhaps someone can recall some history and how come these d_drop
> calls showed up in ecryptfs_link()?

git blame points out commits 45ec4aba and ae56fb16, but you aren't the
first person to point out these seemingly pointless d_drop's.  I think
I'll go ahead and commit this to the next branch and give it plenty of
testing.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next

> 
> Cheers,
> Erez.
> 
> ------------------------------------------------------------------------------
> 
> ecryptfs: remove unnecessary d_drop calls in wrapfs_link
> 
> Unnecessary because it would unhash perfectly valid dentries, causing them
> to have to be re-looked up the next time they're needed, which presumably is
> right after.
> 
> Signed-off-by: Aseem Rastogi <arastogi@cs.sunysb.edu>
> Signed-off-by: Shrikar archak <shrikar84@gmail.com>
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 186d744..2dc0bdb 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -463,9 +463,6 @@ out_lock:
>  	unlock_dir(lower_dir_dentry);
>  	dput(lower_new_dentry);
>  	dput(lower_old_dentry);
> -	d_drop(lower_old_dentry);
> -	d_drop(new_dentry);
> -	d_drop(old_dentry);
>  	return rc;
>  }
>