Re: BKL removal

[Dave Hansen <haveblue@us.ibm.com>]

Matthew Wilcox wrote:
> aw.  you implemented the less useful check of the two you mentioned!
> i think the release-on-sleep property of the BKL is far more harmful
> than the recursive-holds.  release-on-sleep means that you probably
> have a race there you didn't know about (eg the one i found in setfl
> earlier this week), and means you need to do some serious work in order
> to redesign your code to eliminate the race.

Be careful what you ask for.  These release-on-sleep calls happen 
quite often, maybe as often as a few a second and I can't think of any 
elegant ways of limiting the number of unique printk()s in schedule(). 
  It was easy to limit the printing for a single un/lock_kernel() call 
with static variables, but the scheduling ones are a bit more 
complicated.  I can think of some convoluted ways of doing this, but 
no simple ones.  If you apply this patch, be ready for a load of stuff 
in dmesg!

> once you've done that redesign, chances are you'll understand the locking
> in that area well enough to know that there are no recursive holds in your
> code and you'll switch to a spinlock.  this will automagically reduce the
> number of recursive-holds simply because fewer places will take the BKL.

Some of the times that I've either broken someone else's code or 
simply pointed it out to them, they've fixed it.

> as an aside, replacing the BKL by a non-recursive spinlock isn't always a
> great idea:
...<snip> look at the original if you want to see the whole call sequence!
> it doesn't look _too_ hard to persuade ext2_new_block (btw, the problem
> also occurs with ext2_free_blocks) to unlock_super and retry, but that's
> slightly more dangerous work than i want to do right now ... particularly
> since some of these interfaces may change dramatically as a result of
> the aio work.

That's icky.  So, was this potential problem introduced during Al's 
BKL replacement in ext2?  The real problem here is the faulting 
operation while holding a spinlock which requires the spinlock to 
resolve, right?  I think that all you can do is release the lock, try 
to get some memory, and start the process over.  This is the kind of 
thing that I hope my patch will help us find so that we don't all need 
to have a Viroesque understanding of VFS :)

BTW, the Brits don't have any big parties today, do they?

-- 
Dave Hansen
haveblue@us.ibm.com