BKL removal from find_exported_dentry()

BKL removal from find_exported_dentry()

[Jose R. Santos]

Hi all

I was running SpecSFS on a 8-way machine when I encounter a situation were 
find_exported_dentry() was showing up to 75% in my profiles.  After some 
digging, the problem was trigger when the system was under memory pressure 
which cause a dramatic shrinkage in dcache entries.  This shrinkage cause the 
code path thats held under the BKL to be hit under a few filesystems (XFS, JFS 
and EXT3).

At first glance, the BKL seem over kill since I don't see it protecting 
anything that not already protected by other locks inside the code path.  I 
went ahead and removed the kernel lock from find_exported_dentry() and after 
several hours of testing different loads and different filesystems I have yet 
to see any kernel badness or
NFS call failures.

Is there anything in particular the BKL is trying to protect here or is it 
safe to remove?

-JRS


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: BKL removal from find_exported_dentry()

[Jose R. Santos]

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

On 03/09/04 07:54:51, Jose R. Santos wrote:
> Is there anything in particular the BKL is trying to protect here or is it 
> safe to remove?

Should I push this through Andrew Morton or should this go through you guys 
first?  Is there a particular scenario were this would be unsafe?

Looking at the code it seems there is a possibility of racing if directories
get rename or remove, but BKL doesn't seem to protect against these races, it
may slow down thing a bit to avoid them though.

I'm attaching a patch that removes BKL from find_exported_dentry().  I see a 
huge improvement on SpecSFS benchmark and haven't seen any failure up to date.

Comments?

Thanks

-JRS

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: exportfs_bkl_removal.patch --]
[-- Type: text/x-patch; charset=unknown-8bit, Size: 846 bytes --]

===== fs/exportfs/expfs.c 1.13 vs edited =====
--- 1.13/fs/exportfs/expfs.c	Sun Apr 20 01:22:23 2003
+++ edited/fs/exportfs/expfs.c	Thu Mar 11 13:59:35 2004
@@ -135,7 +135,6 @@
 	 * the noprogress counter.  If we go through the loop 10 times (2 is
 	 * probably enough) without getting anywhere, we just give up
 	 */
-	lock_kernel();
 	noprogress= 0;
 	while (target_dir->d_flags & DCACHE_DISCONNECTED && noprogress++ < 10) {
 		struct dentry *pd = target_dir;
@@ -232,7 +231,6 @@
 		/* something went wrong - oh-well */
 		if (!err)
 			err = -ESTALE;
-		unlock_kernel();
 		goto err_target;
 	}
 	/* if we weren't after a directory, have one more step to go */
@@ -254,7 +252,6 @@
 		}
 	}
 	dput(target_dir);
-	unlock_kernel();
 	/* now result is properly connected, it is our best bet */
 	if (acceptable(context, result))
 		return result;

Re: BKL removal from find_exported_dentry()

[Jose R. Santos]

On 03/11/04 15:10:20, Jose R. Santos wrote:
> Looking at the code it seems there is a possibility of racing if directories
> get rename or remove, but BKL doesn't seem to protect against these races, it
> may slow down thing a bit to avoid them though.

After looking at this even further, I'm starting to get the impression that the
BKL is there in order to protect against filesystems that behave badly.  I guess
this is the price for code portability across many filesystems.

Could a better choice to fix this issue be to implementing the get_dentry() for 
each filesystem and making sure that it almost always returns a connected dentry?
Filesystems that don't implement it would just use the generic op. 

I also have concerns about seeing this on busy SMP machines with relative small 
amounts of memory and several NFS exports with very deep directories.

> I'm attaching a patch that removes BKL from find_exported_dentry().  I see a 
> huge improvement on SpecSFS benchmark and haven't seen any failure up to date.

While reduces the time spent on find_exported_dentry() from 75% to less than 1%,
it seems that this may not be a very popular approach.  Guess I should have done 
my homework about the BKL removal subject before posting. :)

I am really interested in knowing if the BKL is there to protect against bad 
filesystem or if its there for other reasons.

Thanks

-JRS




-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: BKL removal from find_exported_dentry()

[Neil Brown]

On Tuesday March 16, jrsantos@austin.ibm.com wrote:
> After looking at this even further, I'm starting to get the impression that the
> BKL is there in order to protect against filesystems that behave badly.  I guess
> this is the price for code portability across many filesystems.
...
> 
> > I'm attaching a patch that removes BKL from find_exported_dentry().  I see a 
> > huge improvement on SpecSFS benchmark and haven't seen any failure up to date.
> 
> While reduces the time spent on find_exported_dentry() from 75% to less than 1%,
> it seems that this may not be a very popular approach.  Guess I should have done 
> my homework about the BKL removal subject before posting. :)
> 
> I am really interested in knowing if the BKL is there to protect against bad 
> filesystem or if its there for other reasons.

It is a long time now since I wrote that code.
I went through several iterations, and at some stages the BKL was
definitely needed.
My vague memory is that towards the end, the BKL was needed to protect
against d_move, and particularly to protect accessed to d_parent.
However even when this code was submitted to linus, dparent_lock was
in place for that, and now it has been replaced by  ->d_lock.
So that is certainly no longer a justification for BKL.

I've stared at the code for a while and cannot see any justification
for leaving the BKL there.

I don't think the idea of protecting badly behaved filesystems is
valid.  All the interfaces that we use into filesystems should
definitely be SMP-safe.

So: yes, send the patch to Andrew, tell him that I approve, but maybe
suggest it stay out of main-line for a little while just-in-case.

Thanks,
NeilBrown


-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs