Message-ID: <20100102163644.GA5076@nowhere>
Date:	Sat, 2 Jan 2010 17:36:46 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Christian Kujau <lists@...dbynature.de>,
	Alexander Beregalov <a.beregalov@...il.com>,
	Chris Mason <chris.mason@...cle.com>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [GIT PULL] reiserfs fixes

On Sat, Jan 02, 2010 at 02:41:51PM +0100, Andi Kleen wrote:
> Frederic Weisbecker <fweisbec@...il.com> writes:
> >
> > That's why you'll mostly find dependency inversion fixes based on
> > such pattern:
> >
> > reiserfs_write_unlock()
> > mutex_lock(random_lock)
> > reiserfs_write_lock()
> 
> These `workarounds' look rather ugly and are likely much slower
> than the BKL that was there before.


This is ugly, I can't argue against that. But while this is
apparently uglier than the bkl, it is actually better than it.

The bkl does its dirty workarounds internally. We don't see
it as it's done on schedule time and so. So indeed, placing
a simple lock_kernel() in each outer most caller in reiserfs
looks much proper and pretty than all these workarounds that
try to catch up with its locking-based scheme.

Now try to look at the new reiserfs lock not from a visual
point of view but from its impact. The bkl relax the lock, it is
acquired recursively etc... This usually implies a lot of hard
investigation. The new lock has identified all of
these dirty implicit places, knows when it is required to
relax, knows when it is acquired recursively, knows where
are the lock inversions once converted into a normal lock.

All of this hard work of investigation has been done already.
If someone wants to, one day, refine the locking to make
something smarter (which I doubt), starting from the current
codebase is _way_ much easier than starting from the 2.6.32
bkl based scheme. Actually someone who is going to try that
from the bkl point will undoubtly need an intermediate state
like the current one.

The ugliness is here. But finding it more ugly than the bkl
is a _pure_ illusion.

Concerning the slowness. The xattr operation that have been
patched here don't appear to me beeing in a fast path.
Moreover some lookup areas (apart from relax on random lock) have
been relaxed from the reiserfs lock in this new set.


> Perhaps it's better to simply
> go back to the BKL until this can be all fixed properly
> (or a more faithful emulation for the BKL can be devised)?


There are 99% of chances that nobody will ever fix it. Especially
if one needs to start from the bkl base.
Few people are familiar with the reiserfs code. Among these
people, I doubt someone is motivated to do it.

And for those who aren't familiar with it, reiserfs code is
so messy that I doubt many people will try something
for more than few minutes.

This is especially true now that it is an old and legacy
filesystem. Nobody cares anymore.

I only have reiserfs partitions in my laptop and my testbox,
nothing else. And that because I'm now maintaining it de facto.
Otherwise I wouldn't encumber with that and would immediately
set up btrfs everywhere.

Concerning a more faithful emulation of the bkl. That would
require to divide the bkl in several sub-bkl. This is
pointless and even worst than the bkl.

- that would require a notifier in schedule(), one notifier
  per sub-bkl. That's horrible for performances. And for
  the scheduler. I will be the first to NAK.

- that doesn't solve the problem as this sub-bkl won't ever
  be removed. We just isolate a giant lock in reiserfs. That
  doesn't change anything, nor make the things simpler
  especially since we already know that reiserfs use of the bkl
  is not related to other users of bkl.

- the reiserfs lock is per superblock. It scales better.


> >
> > This is not beautiful but at least that's better than the bkl.
> >
> > Oh and I expect other lock inversions will get reported in
> > the future due to rare and then yet untested paths.
> 
> ... and given that was the conversion really a good idea?


Despite the _apparent_ ugliness compared to the bkl, I'm
still sure this was, and is still, a good idea.

The fact we are going to experience other locking inversions
is a necessary pain. It's impossible to bypass these states,
you can't remove that easily such giant lock from a complex
code base.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists