BKL removal from find_exported_dentry()

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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

===== fs/exportfs/expfs.c 1.13 vs edited =====

@@ -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;

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.

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

...

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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs