[GIT PULL] reiserfs fixes

[GIT PULL] reiserfs fixes

[Frederic Weisbecker]

Linus,

Please pull the reiserfs/kill-bkl branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	reiserfs/kill-bkl

Thanks,
	Frederic
---

Frederic Weisbecker (1):
      reiserfs: Fix softlockup while waiting on an inode


 fs/reiserfs/inode.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

---
commit 175359f89df39f4faed663c8cfd6ee0222d2fa1e
Author: Frederic Weisbecker <fweisbec@gmail.com>
Date:   Thu Feb 11 13:13:10 2010 +0100

    reiserfs: Fix softlockup while waiting on an inode
    
    When we wait for an inode through reiserfs_iget(), we hold
    the reiserfs lock. And waiting for an inode may imply waiting
    for its writeback. But the inode writeback path may also require
    the reiserfs lock, which leads to a deadlock.
    
    We just need to release the reiserfs lock from reiserfs_iget()
    to fix this.
    
    Reported-by: Alexander Beregalov <a.beregalov@gmail.com>
    Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
    Tested-by: Christian Kujau <lists@nerdbynature.de>
    Cc: Chris Mason <chris.mason@oracle.com>

diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 9087b10..2df0f5c 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -1497,9 +1497,11 @@ struct inode *reiserfs_iget(struct super_block *s, const struct cpu_key *key)
 
 	args.objectid = key->on_disk_key.k_objectid;
 	args.dirid = key->on_disk_key.k_dir_id;
+	reiserfs_write_unlock(s);
 	inode = iget5_locked(s, key->on_disk_key.k_objectid,
 			     reiserfs_find_actor, reiserfs_init_locked_inode,
 			     (void *)(&args));
+	reiserfs_write_lock(s);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 

[GIT PULL] reiserfs fixes

[Frederic Weisbecker]

Linus,

Please pull the reiserfs/kill-bkl branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	reiserfs/kill-bkl

These changes fix a lot of lock inversions, some of them were
triggering soft lockups very easily in xattrs operations.

As the reiserfs lock is a giant lock (in reiserfs scope),
these dependency inversions couldn't get smart fixes without a deep
locking rewrite.

That's why you'll mostly find dependency inversion fixes based on
such pattern:

reiserfs_write_unlock()
mutex_lock(random_lock)
reiserfs_write_lock()

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.

Thanks,
	Frederic
---

Frederic Weisbecker (13):
      reiserfs: Fix possible recursive lock
      reiserfs: Fix reiserfs lock and journal lock inversion dependency
      reiserfs: Fix reiserfs lock <-> inode mutex dependency inversion
      reiserfs: Fix remaining in-reclaim-fs <-> reclaim-fs-on locking inversion
      reiserfs: Fix reiserfs lock <-> i_xattr_sem dependency inversion
      reiserfs: Warn on lock relax if taken recursively
      reiserfs: Fix reiserfs lock <-> i_mutex dependency inversion on xattr
      reiserfs: Relax reiserfs lock while freeing the journal
      reiserfs: Relax lock before open xattr dir in reiserfs_xattr_set_handle()
      reiserfs: Fix unwanted recursive reiserfs lock in reiserfs_unlink()
      reiserfs: Fix journal mutex <-> inode mutex lock inversion
      reiserfs: Safely acquire i_mutex from reiserfs_for_each_xattr
      reiserfs: Safely acquire i_mutex from xattr_rmdir


 fs/reiserfs/bitmap.c        |    3 +++
 fs/reiserfs/inode.c         |    5 +++--
 fs/reiserfs/journal.c       |   18 ++++++++++++++----
 fs/reiserfs/lock.c          |    9 +++++++++
 fs/reiserfs/namei.c         |    7 ++++---
 fs/reiserfs/xattr.c         |   26 ++++++++++++++++++++------
 include/linux/reiserfs_fs.h |   26 ++++++++++++++++++++++++++
 7 files changed, 79 insertions(+), 15 deletions(-)

Re: [GIT PULL] reiserfs fixes

[Andi Kleen]

Frederic Weisbecker <fweisbec@gmail.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. 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)?

>
> 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?

-Andi


-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [GIT PULL] reiserfs fixes

[Frederic Weisbecker]

On Sat, Jan 02, 2010 at 02:41:51PM +0100, Andi Kleen wrote:
> Frederic Weisbecker <fweisbec@gmail.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.

Re: [GIT PULL] reiserfs fixes

[Linus Torvalds]

On Sat, 2 Jan 2010, Frederic Weisbecker wrote:
> 
> These changes fix a lot of lock inversions, some of them were
> triggering soft lockups very easily in xattrs operations.

Should 2.6.32-stable merge this branch too?

		Linus

Re: [GIT PULL] reiserfs fixes

[Linus Torvalds]

On Sat, 2 Jan 2010, Linus Torvalds wrote:
> 
> Should 2.6.32-stable merge this branch too?

Never mind, none of the reiserfs bkl-removal went into 2.6.32. We had some 
other BKL work that got in there, but all the reiserfs stuff is 
post-2.6.32 only, I guess.

		Linus

Re: [GIT PULL] reiserfs fixes

[Frederic Weisbecker]

On Sat, Jan 02, 2010 at 11:21:51AM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 2 Jan 2010, Linus Torvalds wrote:
> > 
> > Should 2.6.32-stable merge this branch too?
> 
> Never mind, none of the reiserfs bkl-removal went into 2.6.32. We had some 
> other BKL work that got in there, but all the reiserfs stuff is 
> post-2.6.32 only, I guess.
> 
> 		Linus


Exactly.

Re: [GIT PULL] reiserfs fixes

[Frederic Weisbecker]

On Sat, Jan 02, 2010 at 11:19:20AM -0800, Linus Torvalds wrote:
> 
> 
> On Sat, 2 Jan 2010, Frederic Weisbecker wrote:
> > 
> > These changes fix a lot of lock inversions, some of them were
> > triggering soft lockups very easily in xattrs operations.
> 
> Should 2.6.32-stable merge this branch too?
> 
> 		Linus



No this only relies on the bkl removal patches merged in this cycle.

Thanks.