[PATCH 0/2] kill-the-bkl/reiserfs: rebase against -rc6, fixes

[PATCH 0/2] kill-the-bkl/reiserfs: rebase against -rc6, fixes

[Frederic Weisbecker]

Hi Al, Stephen,

This patchset is subsequent to a rebase of the bkl killing in reiserfs
work rebased against -rc6. It contains only fixes.
The old topic is left unchanged but the new one reiserfs/kill-bkl-rc6 can
be pulled against -rc6 without problem.

Also, Al, I don't know if you would still accept to apply this work in the VFS
tree.
The fact is that it becomes late in the process, we are already in -rc6
and I would like it to be tested at least in linux-next.

So, it's up to you. Whether you prefer to apply it and make it
available to next, or I can ask Stephen to pull directly from my tree.

Thanks,
Frederic.

The following changes since commit b693ba70c541a5a26eb4e8f2db5ade454afe1c74:
  Frederic Weisbecker (1):
        kill-the-bkl/reiserfs: unlock only when needed in search_by_key

are available in the git repository at:

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

Frederic Weisbecker (2):
      kill-the-bkl/reiserfs: acquire the inode mutex safely
      kill-the-bkl/reiserfs: move the concurrent tree accesses checks per superblock

 fs/reiserfs/do_balan.c         |   17 +++++------------
 fs/reiserfs/fix_node.c         |    5 +----
 fs/reiserfs/journal.c          |   34 ----------------------------------
 fs/reiserfs/prints.c           |    4 ----
 fs/reiserfs/stree.c            |    5 +----
 fs/reiserfs/xattr.c            |    4 ++--
 include/linux/reiserfs_fs.h    |   35 +++++++++++++++++++++++++++++++++++
 include/linux/reiserfs_fs_sb.h |   11 +++++++++++
 8 files changed, 55 insertions(+), 60 deletions(-)

[PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Frederic Weisbecker]

While searching a pathname, an inode mutex can be acquired
in do_lookup() which calls reiserfs_lookup() which in turn
acquires the write lock.

On the other side reiserfs_fill_super() can acquire the write_lock
and then call reiserfs_lookup_privroot() which can acquire an
inode mutex (the root of the mount point).

So we theoretically risk an AB - BA lock inversion that could lead
to a deadlock.

As for other lock dependencies found since the bkl to mutex
conversion, the fix is to use reiserfs_mutex_lock_safe() which
drops the lock dependency to the write lock.

[ Impact: fix a possible deadlock with reiserfs ]

Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Alexander Beregalov <a.beregalov@gmail.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 fs/reiserfs/journal.c       |   34 ----------------------------------
 fs/reiserfs/xattr.c         |    4 ++--
 include/linux/reiserfs_fs.h |   35 +++++++++++++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/fs/reiserfs/journal.c b/fs/reiserfs/journal.c
index 3c3e00d..86c1ff4 100644
--- a/fs/reiserfs/journal.c
+++ b/fs/reiserfs/journal.c
@@ -537,40 +537,6 @@ static inline void insert_journal_hash(struct reiserfs_journal_cnode **table,
 	journal_hash(table, cn->sb, cn->blocknr) = cn;
 }
 
-/*
- * Several mutexes depend on the write lock.
- * However sometimes we want to relax the write lock while we hold
- * these mutexes, according to the release/reacquire on schedule()
- * properties of the Bkl that were used.
- * Reiserfs performances and locking were based on this scheme.
- * Now that the write lock is a mutex and not the bkl anymore, doing so
- * may result in a deadlock:
- *
- * A acquire write_lock
- * A acquire j_commit_mutex
- * A release write_lock and wait for something
- * B acquire write_lock
- * B can't acquire j_commit_mutex and sleep
- * A can't acquire write lock anymore
- * deadlock
- *
- * What we do here is avoiding such deadlock by playing the same game
- * than the Bkl: if we can't acquire a mutex that depends on the write lock,
- * we release the write lock, wait a bit and then retry.
- *
- * The mutexes concerned by this hack are:
- * - The commit mutex of a journal list
- * - The flush mutex
- * - The journal lock
- */
-static inline void reiserfs_mutex_lock_safe(struct mutex *m,
-			       struct super_block *s)
-{
-	reiserfs_write_unlock(s);
-	mutex_lock(m);
-	reiserfs_write_lock(s);
-}
-
 /* lock the current transaction */
 static inline void lock_journal(struct super_block *sb)
 {
diff --git a/fs/reiserfs/xattr.c b/fs/reiserfs/xattr.c
index 2237e10..b4f74e5 100644
--- a/fs/reiserfs/xattr.c
+++ b/fs/reiserfs/xattr.c
@@ -969,7 +969,7 @@ int reiserfs_lookup_privroot(struct super_block *s)
 	int err = 0;
 
 	/* If we don't have the privroot located yet - go find it */
-	mutex_lock(&s->s_root->d_inode->i_mutex);
+	reiserfs_mutex_lock_safe(&s->s_root->d_inode->i_mutex, s);
 	dentry = lookup_one_len(PRIVROOT_NAME, s->s_root,
 				strlen(PRIVROOT_NAME));
 	if (!IS_ERR(dentry)) {
@@ -1005,7 +1005,7 @@ int reiserfs_xattr_init(struct super_block *s, int mount_flags)
 
 	if (privroot->d_inode) {
 		s->s_xattr = reiserfs_xattr_handlers;
-		mutex_lock(&privroot->d_inode->i_mutex);
+		reiserfs_mutex_lock_safe(&privroot->d_inode->i_mutex, s);
 		if (!REISERFS_SB(s)->xattr_root) {
 			struct dentry *dentry;
 			dentry = lookup_one_len(XAROOT_NAME, privroot,
diff --git a/include/linux/reiserfs_fs.h b/include/linux/reiserfs_fs.h
index 39bd4ea..7665f41 100644
--- a/include/linux/reiserfs_fs.h
+++ b/include/linux/reiserfs_fs.h
@@ -63,6 +63,41 @@ int reiserfs_write_lock_once(struct super_block *s);
 void reiserfs_write_unlock_once(struct super_block *s, int lock_depth);
 
 /*
+ * Several mutexes depend on the write lock.
+ * However sometimes we want to relax the write lock while we hold
+ * these mutexes, according to the release/reacquire on schedule()
+ * properties of the Bkl that were used.
+ * Reiserfs performances and locking were based on this scheme.
+ * Now that the write lock is a mutex and not the bkl anymore, doing so
+ * may result in a deadlock:
+ *
+ * A acquire write_lock
+ * A acquire j_commit_mutex
+ * A release write_lock and wait for something
+ * B acquire write_lock
+ * B can't acquire j_commit_mutex and sleep
+ * A can't acquire write lock anymore
+ * deadlock
+ *
+ * What we do here is avoiding such deadlock by playing the same game
+ * than the Bkl: if we can't acquire a mutex that depends on the write lock,
+ * we release the write lock, wait a bit and then retry.
+ *
+ * The mutexes concerned by this hack are:
+ * - The commit mutex of a journal list
+ * - The flush mutex
+ * - The journal lock
+ * - The inode mutex
+ */
+static inline void reiserfs_mutex_lock_safe(struct mutex *m,
+			       struct super_block *s)
+{
+	reiserfs_write_unlock(s);
+	mutex_lock(m);
+	reiserfs_write_lock(s);
+}
+
+/*
  * When we schedule, we usually want to also release the write lock,
  * according to the previous bkl based locking scheme of reiserfs.
  */
-- 
1.6.2.3

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
<fweisbec@gmail.com> wrote:
> While searching a pathname, an inode mutex can be acquired
> in do_lookup() which calls reiserfs_lookup() which in turn
> acquires the write lock.
>
> On the other side reiserfs_fill_super() can acquire the write_lock
> and then call reiserfs_lookup_privroot() which can acquire an
> inode mutex (the root of the mount point).
>
> So we theoretically risk an AB - BA lock inversion that could lead
> to a deadlock.
>
> As for other lock dependencies found since the bkl to mutex
> conversion, the fix is to use reiserfs_mutex_lock_safe() which
> drops the lock dependency to the write lock.
>

I'm curious, did this get applied, and is it related to the following?
 I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
today.  I haven't seen them today.  But then again, I only seen this
happen one time.

May 27 01:56:12 tdamac INFO: task pdflush:15370 blocked for more than
120 seconds.
May 27 01:56:12 tdamac "echo 0 >
/proc/sys/kernel/hung_task_timeout_secs" disables this message.
May 27 01:56:12 tdamac pdflush       D ffff8800518a0000     0 15370      2
May 27 01:56:12 tdamac ffff880025023b50 0000000000000046
0000000025023a90 000000000000d7a0
May 27 01:56:12 tdamac 0000000000004000 0000000000011440
000000000000ca78 ffff880045e71568
May 27 01:56:12 tdamac ffff880045e7156c ffff8800518a0000
ffff880067f54230 ffff8800518a0380
May 27 01:56:12 tdamac Call Trace:
May 27 01:56:12 tdamac [<ffffffff80687d1b>] ? __mutex_lock_slowpath+0xe2/0x124
May 27 01:56:12 tdamac [<ffffffff80687d13>] __mutex_lock_slowpath+0xda/0x124
May 27 01:56:12 tdamac [<ffffffff8068809e>] mutex_lock+0x1e/0x36
May 27 01:56:12 tdamac [<ffffffff803087ae>] flush_commit_list+0x150/0x689
May 27 01:56:12 tdamac [<ffffffff8022f8e5>] ? __wake_up+0x43/0x50
May 27 01:56:12 tdamac [<ffffffff8030ad8a>] do_journal_end+0xb4a/0xd6c
May 27 01:56:12 tdamac [<ffffffff8023053d>] ? dequeue_entity+0x1b/0x1df
May 27 01:56:12 tdamac [<ffffffff8030b020>] journal_end_sync+0x74/0x7d
May 27 01:56:12 tdamac [<ffffffff802fd2fd>] reiserfs_sync_fs+0x41/0x67
May 27 01:56:12 tdamac [<ffffffff80688091>] ? mutex_lock+0x11/0x36
May 27 01:56:12 tdamac [<ffffffff802fd331>] reiserfs_write_super+0xe/0x10
May 27 01:56:12 tdamac [<ffffffff802a532a>] sync_supers+0x61/0xa6
May 27 01:56:12 tdamac [<ffffffff8027e140>] wb_kupdate+0x32/0x128
May 27 01:56:12 tdamac [<ffffffff8027ee7c>] pdflush+0x140/0x21f
May 27 01:56:12 tdamac [<ffffffff8027e10e>] ? wb_kupdate+0x0/0x128
May 27 01:56:12 tdamac [<ffffffff8027ed3c>] ? pdflush+0x0/0x21f
May 27 01:56:12 tdamac [<ffffffff8024fb26>] kthread+0x56/0x83
May 27 01:56:12 tdamac [<ffffffff8020beba>] child_rip+0xa/0x20
May 27 01:56:12 tdamac [<ffffffff8024fad0>] ? kthread+0x0/0x83
May 27 01:56:12 tdamac [<ffffffff8020beb0>] ? child_rip+0x0/0x20

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Frederic Weisbecker]

On Fri, May 29, 2009 at 09:05:31PM -0600, Trenton D. Adams wrote:
> On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
> <fweisbec@gmail.com> wrote:
> > While searching a pathname, an inode mutex can be acquired
> > in do_lookup() which calls reiserfs_lookup() which in turn
> > acquires the write lock.
> >
> > On the other side reiserfs_fill_super() can acquire the write_lock
> > and then call reiserfs_lookup_privroot() which can acquire an
> > inode mutex (the root of the mount point).
> >
> > So we theoretically risk an AB - BA lock inversion that could lead
> > to a deadlock.
> >
> > As for other lock dependencies found since the bkl to mutex
> > conversion, the fix is to use reiserfs_mutex_lock_safe() which
> > drops the lock dependency to the write lock.
> >
> 
> I'm curious, did this get applied, and is it related to the following?
>  I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
> today.  I haven't seen them today.  But then again, I only seen this
> happen one time.


Hi,

No, may be it will come for 2.6.31 but for now it is not merged so
it's not related.

If you see such warning anymore, don't hesitate to tell about
it!

Thanks!


 
> May 27 01:56:12 tdamac INFO: task pdflush:15370 blocked for more than
> 120 seconds.
> May 27 01:56:12 tdamac "echo 0 >
> /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> May 27 01:56:12 tdamac pdflush       D ffff8800518a0000     0 15370      2
> May 27 01:56:12 tdamac ffff880025023b50 0000000000000046
> 0000000025023a90 000000000000d7a0
> May 27 01:56:12 tdamac 0000000000004000 0000000000011440
> 000000000000ca78 ffff880045e71568
> May 27 01:56:12 tdamac ffff880045e7156c ffff8800518a0000
> ffff880067f54230 ffff8800518a0380
> May 27 01:56:12 tdamac Call Trace:
> May 27 01:56:12 tdamac [<ffffffff80687d1b>] ? __mutex_lock_slowpath+0xe2/0x124
> May 27 01:56:12 tdamac [<ffffffff80687d13>] __mutex_lock_slowpath+0xda/0x124
> May 27 01:56:12 tdamac [<ffffffff8068809e>] mutex_lock+0x1e/0x36
> May 27 01:56:12 tdamac [<ffffffff803087ae>] flush_commit_list+0x150/0x689
> May 27 01:56:12 tdamac [<ffffffff8022f8e5>] ? __wake_up+0x43/0x50
> May 27 01:56:12 tdamac [<ffffffff8030ad8a>] do_journal_end+0xb4a/0xd6c
> May 27 01:56:12 tdamac [<ffffffff8023053d>] ? dequeue_entity+0x1b/0x1df
> May 27 01:56:12 tdamac [<ffffffff8030b020>] journal_end_sync+0x74/0x7d
> May 27 01:56:12 tdamac [<ffffffff802fd2fd>] reiserfs_sync_fs+0x41/0x67
> May 27 01:56:12 tdamac [<ffffffff80688091>] ? mutex_lock+0x11/0x36
> May 27 01:56:12 tdamac [<ffffffff802fd331>] reiserfs_write_super+0xe/0x10
> May 27 01:56:12 tdamac [<ffffffff802a532a>] sync_supers+0x61/0xa6
> May 27 01:56:12 tdamac [<ffffffff8027e140>] wb_kupdate+0x32/0x128
> May 27 01:56:12 tdamac [<ffffffff8027ee7c>] pdflush+0x140/0x21f
> May 27 01:56:12 tdamac [<ffffffff8027e10e>] ? wb_kupdate+0x0/0x128
> May 27 01:56:12 tdamac [<ffffffff8027ed3c>] ? pdflush+0x0/0x21f
> May 27 01:56:12 tdamac [<ffffffff8024fb26>] kthread+0x56/0x83
> May 27 01:56:12 tdamac [<ffffffff8020beba>] child_rip+0xa/0x20
> May 27 01:56:12 tdamac [<ffffffff8024fad0>] ? kthread+0x0/0x83
> May 27 01:56:12 tdamac [<ffffffff8020beb0>] ? child_rip+0x0/0x20

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

On Fri, May 29, 2009 at 9:22 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Fri, May 29, 2009 at 09:05:31PM -0600, Trenton D. Adams wrote:
>> On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
>> <fweisbec@gmail.com> wrote:
>> > While searching a pathname, an inode mutex can be acquired
>> > in do_lookup() which calls reiserfs_lookup() which in turn
>> > acquires the write lock.
>> >
>> > On the other side reiserfs_fill_super() can acquire the write_lock
>> > and then call reiserfs_lookup_privroot() which can acquire an
>> > inode mutex (the root of the mount point).
>> >
>> > So we theoretically risk an AB - BA lock inversion that could lead
>> > to a deadlock.
>> >
>> > As for other lock dependencies found since the bkl to mutex
>> > conversion, the fix is to use reiserfs_mutex_lock_safe() which
>> > drops the lock dependency to the write lock.
>> >
>>
>> I'm curious, did this get applied, and is it related to the following?
>>  I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
>> today.  I haven't seen them today.  But then again, I only seen this
>> happen one time.
>
>
> Hi,
>
> No, may be it will come for 2.6.31 but for now it is not merged so
> it's not related.
>
> If you see such warning anymore, don't hesitate to tell about
> it!
>
> Thanks!
>

I was trying to imply that the patch might fix the problem I saw, not
that it was the cause.  I only though that because it mentioned a
potential deadlock, and it seems like that is what the problem I saw
was.

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Frederic Weisbecker]

On Fri, May 29, 2009 at 10:23:44PM -0600, Trenton D. Adams wrote:
> On Fri, May 29, 2009 at 9:22 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Fri, May 29, 2009 at 09:05:31PM -0600, Trenton D. Adams wrote:
> >> On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
> >> <fweisbec@gmail.com> wrote:
> >> > While searching a pathname, an inode mutex can be acquired
> >> > in do_lookup() which calls reiserfs_lookup() which in turn
> >> > acquires the write lock.
> >> >
> >> > On the other side reiserfs_fill_super() can acquire the write_lock
> >> > and then call reiserfs_lookup_privroot() which can acquire an
> >> > inode mutex (the root of the mount point).
> >> >
> >> > So we theoretically risk an AB - BA lock inversion that could lead
> >> > to a deadlock.
> >> >
> >> > As for other lock dependencies found since the bkl to mutex
> >> > conversion, the fix is to use reiserfs_mutex_lock_safe() which
> >> > drops the lock dependency to the write lock.
> >> >
> >>
> >> I'm curious, did this get applied, and is it related to the following?
> >>  I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
> >> today.  I haven't seen them today.  But then again, I only seen this
> >> happen one time.
> >
> >
> > Hi,
> >
> > No, may be it will come for 2.6.31 but for now it is not merged so
> > it's not related.
> >
> > If you see such warning anymore, don't hesitate to tell about
> > it!
> >
> > Thanks!
> >
> 
> I was trying to imply that the patch might fix the problem I saw, not
> that it was the cause.  I only though that because it mentioned a
> potential deadlock, and it seems like that is what the problem I saw
> was.


Ah ok. No it's part of a tree which reworks the reiserfs locking scheme
by removing the old one based on the legacy and obsolete bkl (big kernel
lock). In this tree I had to fix several deadlocks or at least unsafe
lock states because the bkl is converted into a mutex and some new lock
dependencies were borned after that. But these issues had nothing
to deal with upstream problems.

BTW, would you be interested in giving a try with this reiserfs bkl
removal tree? I really lack testing and feedbacks from users.

Thanks!

Frederic.

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

On Sat, May 30, 2009 at 7:41 AM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Fri, May 29, 2009 at 10:23:44PM -0600, Trenton D. Adams wrote:
>>
>> I was trying to imply that the patch might fix the problem I saw, not
>> that it was the cause.  I only though that because it mentioned a
>> potential deadlock, and it seems like that is what the problem I saw
>> was.
>
>
> Ah ok. No it's part of a tree which reworks the reiserfs locking scheme
> by removing the old one based on the legacy and obsolete bkl (big kernel
> lock). In this tree I had to fix several deadlocks or at least unsafe
> lock states because the bkl is converted into a mutex and some new lock
> dependencies were borned after that. But these issues had nothing
> to deal with upstream problems.
>
> BTW, would you be interested in giving a try with this reiserfs bkl
> removal tree? I really lack testing and feedbacks from users.

Well, the problem is that I only use reiserfs on my usb stick at the
moment.  So, the amount of testing I could do would be minimal at the
moment.  However, if I change my local filesystems to reiserfs, I will
remember to check back here for your patch.

Thanks.

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Trenton D. Adams wrote:
> On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
> <fweisbec@gmail.com> wrote:
>> While searching a pathname, an inode mutex can be acquired
>> in do_lookup() which calls reiserfs_lookup() which in turn
>> acquires the write lock.
>>
>> On the other side reiserfs_fill_super() can acquire the write_lock
>> and then call reiserfs_lookup_privroot() which can acquire an
>> inode mutex (the root of the mount point).
>>
>> So we theoretically risk an AB - BA lock inversion that could lead
>> to a deadlock.
>>
>> As for other lock dependencies found since the bkl to mutex
>> conversion, the fix is to use reiserfs_mutex_lock_safe() which
>> drops the lock dependency to the write lock.
>>
> 
> I'm curious, did this get applied, and is it related to the following?
>  I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
> today.  I haven't seen them today.  But then again, I only seen this
> happen one time.
> 
> May 27 01:56:12 tdamac INFO: task pdflush:15370 blocked for more than
> 120 seconds.
> May 27 01:56:12 tdamac "echo 0 >
> /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> May 27 01:56:12 tdamac pdflush       D ffff8800518a0000     0 15370      2
> May 27 01:56:12 tdamac ffff880025023b50 0000000000000046
> 0000000025023a90 000000000000d7a0
> May 27 01:56:12 tdamac 0000000000004000 0000000000011440
> 000000000000ca78 ffff880045e71568
> May 27 01:56:12 tdamac ffff880045e7156c ffff8800518a0000
> ffff880067f54230 ffff8800518a0380
> May 27 01:56:12 tdamac Call Trace:
> May 27 01:56:12 tdamac [<ffffffff80687d1b>] ? __mutex_lock_slowpath+0xe2/0x124
> May 27 01:56:12 tdamac [<ffffffff80687d13>] __mutex_lock_slowpath+0xda/0x124
> May 27 01:56:12 tdamac [<ffffffff8068809e>] mutex_lock+0x1e/0x36
> May 27 01:56:12 tdamac [<ffffffff803087ae>] flush_commit_list+0x150/0x689
> May 27 01:56:12 tdamac [<ffffffff8022f8e5>] ? __wake_up+0x43/0x50
> May 27 01:56:12 tdamac [<ffffffff8030ad8a>] do_journal_end+0xb4a/0xd6c
> May 27 01:56:12 tdamac [<ffffffff8023053d>] ? dequeue_entity+0x1b/0x1df
> May 27 01:56:12 tdamac [<ffffffff8030b020>] journal_end_sync+0x74/0x7d
> May 27 01:56:12 tdamac [<ffffffff802fd2fd>] reiserfs_sync_fs+0x41/0x67
> May 27 01:56:12 tdamac [<ffffffff80688091>] ? mutex_lock+0x11/0x36
> May 27 01:56:12 tdamac [<ffffffff802fd331>] reiserfs_write_super+0xe/0x10
> May 27 01:56:12 tdamac [<ffffffff802a532a>] sync_supers+0x61/0xa6
> May 27 01:56:12 tdamac [<ffffffff8027e140>] wb_kupdate+0x32/0x128
> May 27 01:56:12 tdamac [<ffffffff8027ee7c>] pdflush+0x140/0x21f
> May 27 01:56:12 tdamac [<ffffffff8027e10e>] ? wb_kupdate+0x0/0x128
> May 27 01:56:12 tdamac [<ffffffff8027ed3c>] ? pdflush+0x0/0x21f
> May 27 01:56:12 tdamac [<ffffffff8024fb26>] kthread+0x56/0x83
> May 27 01:56:12 tdamac [<ffffffff8020beba>] child_rip+0xa/0x20
> May 27 01:56:12 tdamac [<ffffffff8024fad0>] ? kthread+0x0/0x83
> May 27 01:56:12 tdamac [<ffffffff8020beb0>] ? child_rip+0x0/0x20

Can you capture a sysrq+t when this happens? The lock is properly
released, but I have a hunch that another thread is doing ordered
writeback that's taking a while. That happens under the j_commit_mutex.

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkopY0MACgkQLPWxlyuTD7IM0gCdGepeXFcB68gcCaXCb3Z/KTg9
F5MAn3rOomgVzmXfI4DKtIHqKxwLNDj0
=qzqo
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

On Fri, Jun 5, 2009 at 12:26 PM, Jeff Mahoney<jeffm@suse.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Trenton D. Adams wrote:
>> On Sat, May 16, 2009 at 12:02 PM, Frederic Weisbecker
>> <fweisbec@gmail.com> wrote:
>>> While searching a pathname, an inode mutex can be acquired
>>> in do_lookup() which calls reiserfs_lookup() which in turn
>>> acquires the write lock.
>>>
>>> On the other side reiserfs_fill_super() can acquire the write_lock
>>> and then call reiserfs_lookup_privroot() which can acquire an
>>> inode mutex (the root of the mount point).
>>>
>>> So we theoretically risk an AB - BA lock inversion that could lead
>>> to a deadlock.
>>>
>>> As for other lock dependencies found since the bkl to mutex
>>> conversion, the fix is to use reiserfs_mutex_lock_safe() which
>>> drops the lock dependency to the write lock.
>>>
>>
>> I'm curious, did this get applied, and is it related to the following?
>>  I was having these in 2.6.30-rc3.  I am now on 2.6.30-rc7 as of
>> today.  I haven't seen them today.  But then again, I only seen this
>> happen one time.
>>
>> May 27 01:56:12 tdamac INFO: task pdflush:15370 blocked for more than
>> 120 seconds.
>> May 27 01:56:12 tdamac "echo 0 >
>> /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>> May 27 01:56:12 tdamac pdflush       D ffff8800518a0000     0 15370      2
>> May 27 01:56:12 tdamac ffff880025023b50 0000000000000046
>> 0000000025023a90 000000000000d7a0
>> May 27 01:56:12 tdamac 0000000000004000 0000000000011440
>> 000000000000ca78 ffff880045e71568
>> May 27 01:56:12 tdamac ffff880045e7156c ffff8800518a0000
>> ffff880067f54230 ffff8800518a0380
>> May 27 01:56:12 tdamac Call Trace:
>> May 27 01:56:12 tdamac [<ffffffff80687d1b>] ? __mutex_lock_slowpath+0xe2/0x124
>> May 27 01:56:12 tdamac [<ffffffff80687d13>] __mutex_lock_slowpath+0xda/0x124
>> May 27 01:56:12 tdamac [<ffffffff8068809e>] mutex_lock+0x1e/0x36
>> May 27 01:56:12 tdamac [<ffffffff803087ae>] flush_commit_list+0x150/0x689
>> May 27 01:56:12 tdamac [<ffffffff8022f8e5>] ? __wake_up+0x43/0x50
>> May 27 01:56:12 tdamac [<ffffffff8030ad8a>] do_journal_end+0xb4a/0xd6c
>> May 27 01:56:12 tdamac [<ffffffff8023053d>] ? dequeue_entity+0x1b/0x1df
>> May 27 01:56:12 tdamac [<ffffffff8030b020>] journal_end_sync+0x74/0x7d
>> May 27 01:56:12 tdamac [<ffffffff802fd2fd>] reiserfs_sync_fs+0x41/0x67
>> May 27 01:56:12 tdamac [<ffffffff80688091>] ? mutex_lock+0x11/0x36
>> May 27 01:56:12 tdamac [<ffffffff802fd331>] reiserfs_write_super+0xe/0x10
>> May 27 01:56:12 tdamac [<ffffffff802a532a>] sync_supers+0x61/0xa6
>> May 27 01:56:12 tdamac [<ffffffff8027e140>] wb_kupdate+0x32/0x128
>> May 27 01:56:12 tdamac [<ffffffff8027ee7c>] pdflush+0x140/0x21f
>> May 27 01:56:12 tdamac [<ffffffff8027e10e>] ? wb_kupdate+0x0/0x128
>> May 27 01:56:12 tdamac [<ffffffff8027ed3c>] ? pdflush+0x0/0x21f
>> May 27 01:56:12 tdamac [<ffffffff8024fb26>] kthread+0x56/0x83
>> May 27 01:56:12 tdamac [<ffffffff8020beba>] child_rip+0xa/0x20
>> May 27 01:56:12 tdamac [<ffffffff8024fad0>] ? kthread+0x0/0x83
>> May 27 01:56:12 tdamac [<ffffffff8020beb0>] ? child_rip+0x0/0x20
>
> Can you capture a sysrq+t when this happens? The lock is properly
> released, but I have a hunch that another thread is doing ordered
> writeback that's taking a while. That happens under the j_commit_mutex.

FYI: I never did anything specific that I knew of, so I didn't
actually notice a delay.  I was rsyncing to a USB key at the time.
And seeing it took over an hour, I walked away, so I wouldn't have
noticed it.  But, I could fiddle around a little to see if I could get
some sort of delay going on.  Any ideas on what I should try?  Then I
can do the sysreq+t for you if I can reproduce.

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Trenton D. Adams wrote:
> FYI: I never did anything specific that I knew of, so I didn't
> actually notice a delay.  I was rsyncing to a USB key at the time.
> And seeing it took over an hour, I walked away, so I wouldn't have
> noticed it.  But, I could fiddle around a little to see if I could get
> some sort of delay going on.  Any ideas on what I should try?  Then I
> can do the sysreq+t for you if I can reproduce.

Well if the rsync triggered it, that's a good start. 

Try applying the following patch as well. It will cause the hung task detector
to do a sysrq+t automatically so it's not as much of a guessing game. You'll need to boot with 
hung_task_show_state=1.

- -Jeff

- ---
 kernel/hung_task.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

- --- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -56,6 +56,14 @@ static int __init hung_task_panic_setup(
 }
 __setup("hung_task_panic=", hung_task_panic_setup);
 
+unsigned int __read_mostly sysctl_hung_task_show_state;
+static int __init hung_task_show_state_setup(char *str)
+{
+	sysctl_hung_task_show_state = simple_strtoul(str, NULL, 0);
+	return 1;
+}
+__setup("hung_task_show_state=", hung_task_show_state_setup);
+
 static int
 hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr)
 {
@@ -102,6 +110,9 @@ static void check_hung_task(struct task_
 
 	touch_nmi_watchdog();
 
+	if (sysctl_hung_task_show_state)
+		show_state();
+
 	if (sysctl_hung_task_panic)
 		panic("hung_task: blocked tasks");
 }

- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkopcjwACgkQLPWxlyuTD7IEdgCfVVzIL/DA0stfnYEW6aixFwxM
qIEAnjJjyn6HQAbVIicRYzvNcGvPwbiq
=z8Pn
-----END PGP SIGNATURE-----

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

I'll see if I can try that this weekend.

Thanks.

On Fri, Jun 5, 2009 at 1:30 PM, Jeff Mahoney<jeffm@suse.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Trenton D. Adams wrote:
>> FYI: I never did anything specific that I knew of, so I didn't
>> actually notice a delay.  I was rsyncing to a USB key at the time.
>> And seeing it took over an hour, I walked away, so I wouldn't have
>> noticed it.  But, I could fiddle around a little to see if I could get
>> some sort of delay going on.  Any ideas on what I should try?  Then I
>> can do the sysreq+t for you if I can reproduce.
>
> Well if the rsync triggered it, that's a good start.
>
> Try applying the following patch as well. It will cause the hung task detector
> to do a sysrq+t automatically so it's not as much of a guessing game. You'll need to boot with
> hung_task_show_state=1.
>
> - -Jeff
>
> - ---
>  kernel/hung_task.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> - --- a/kernel/hung_task.c
> +++ b/kernel/hung_task.c
> @@ -56,6 +56,14 @@ static int __init hung_task_panic_setup(
>  }
>  __setup("hung_task_panic=", hung_task_panic_setup);
>
> +unsigned int __read_mostly sysctl_hung_task_show_state;
> +static int __init hung_task_show_state_setup(char *str)
> +{
> +       sysctl_hung_task_show_state = simple_strtoul(str, NULL, 0);
> +       return 1;
> +}
> +__setup("hung_task_show_state=", hung_task_show_state_setup);
> +
>  static int
>  hung_task_panic(struct notifier_block *this, unsigned long event, void *ptr)
>  {
> @@ -102,6 +110,9 @@ static void check_hung_task(struct task_
>
>        touch_nmi_watchdog();
>
> +       if (sysctl_hung_task_show_state)
> +               show_state();
> +
>        if (sysctl_hung_task_panic)
>                panic("hung_task: blocked tasks");
>  }
>
> - --
> Jeff Mahoney
> SUSE Labs
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.9 (GNU/Linux)
> Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkopcjwACgkQLPWxlyuTD7IEdgCfVVzIL/DA0stfnYEW6aixFwxM
> qIEAnjJjyn6HQAbVIicRYzvNcGvPwbiq
> =z8Pn
> -----END PGP SIGNATURE-----
>

Re: [PATCH 1/2] kill-the-bkl/reiserfs: acquire the inode mutex safely

[Trenton D. Adams]

I have not been seeing this with 2.6.30-rc7.  Perhaps it was resolved
in that version.  I'll let you know if it happens again.

[PATCH 2/2] kill-the-bkl/reiserfs: move the concurrent tree accesses checks per superblock

[Frederic Weisbecker]

When do_balance() balances the tree, a trick is performed to
provide the ability for other tree writers/readers to check whether
do_balance() is executing concurrently (requires CONFIG_REISERFS_CHECK).

This is done to protect concurrent accesses to the tree. The trick
is the following:

When do_balance is called, a unique global variable called cur_tb
takes a pointer to the current tree to be rebalanced.
Once do_balance finishes its work, cur_tb takes the NULL value.

Then, concurrent tree readers/writers just have to check the value
of cur_tb to ensure do_balance isn't executing concurrently.
If it is, then it proves that schedule() occured on do_balance(),
which then relaxed the bkl that protected the tree.

Now that the bkl has be turned into a mutex, this check is still
fine even though do_balance() becomes preemptible: the write lock
will not be automatically released on schedule(), so the tree is
still protected.

But this is only fine if we have a single reiserfs mountpoint.
Indeed, because the bkl is a global lock, it didn't allowed
concurrent executions between a tree reader/writer in a mount point
and a do_balance() on another tree from another mountpoint.

So assuming all these readers/writers weren't supposed to be
reentrant, the current check now sometimes detect false positives with
the current per-superblock mutex which allows this reentrancy.

This patch keeps the concurrent tree accesses check but moves it
per superblock, so that only trees from a same mount point are
checked to be not accessed concurrently.

[ Impact: fix spurious panic while running several reiserfs mount-points ]

Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Chris Mason <chris.mason@oracle.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Alexander Beregalov <a.beregalov@gmail.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 fs/reiserfs/do_balan.c         |   17 +++++------------
 fs/reiserfs/fix_node.c         |    5 +----
 fs/reiserfs/prints.c           |    4 ----
 fs/reiserfs/stree.c            |    5 +----
 include/linux/reiserfs_fs_sb.h |   11 +++++++++++
 5 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/fs/reiserfs/do_balan.c b/fs/reiserfs/do_balan.c
index 4beb964..2787349 100644
--- a/fs/reiserfs/do_balan.c
+++ b/fs/reiserfs/do_balan.c
@@ -21,14 +21,6 @@
 #include <linux/buffer_head.h>
 #include <linux/kernel.h>
 
-#ifdef CONFIG_REISERFS_CHECK
-
-struct tree_balance *cur_tb = NULL;	/* detects whether more than one
-					   copy of tb exists as a means
-					   of checking whether schedule
-					   is interrupting do_balance */
-#endif
-
 static inline void buffer_info_init_left(struct tree_balance *tb,
                                          struct buffer_info *bi)
 {
@@ -1841,11 +1833,12 @@ static int check_before_balancing(struct tree_balance *tb)
 {
 	int retval = 0;
 
-	if (cur_tb) {
+	if (REISERFS_SB(tb->tb_sb)->cur_tb) {
 		reiserfs_panic(tb->tb_sb, "vs-12335", "suspect that schedule "
 			       "occurred based on cur_tb not being null at "
 			       "this point in code. do_balance cannot properly "
-			       "handle schedule occurring while it runs.");
+			       "handle concurrent tree accesses on a same "
+			       "mount point.");
 	}
 
 	/* double check that buffers that we will modify are unlocked. (fix_nodes should already have
@@ -1987,7 +1980,7 @@ static inline void do_balance_starts(struct tree_balance *tb)
 	     "check");*/
 	RFALSE(check_before_balancing(tb), "PAP-12340: locked buffers in TB");
 #ifdef CONFIG_REISERFS_CHECK
-	cur_tb = tb;
+	REISERFS_SB(tb->tb_sb)->cur_tb = tb;
 #endif
 }
 
@@ -1997,7 +1990,7 @@ static inline void do_balance_completed(struct tree_balance *tb)
 #ifdef CONFIG_REISERFS_CHECK
 	check_leaf_level(tb);
 	check_internal_levels(tb);
-	cur_tb = NULL;
+	REISERFS_SB(tb->tb_sb)->cur_tb = NULL;
 #endif
 
 	/* reiserfs_free_block is no longer schedule safe.  So, we need to
diff --git a/fs/reiserfs/fix_node.c b/fs/reiserfs/fix_node.c
index 3a685e3..d2f3133 100644
--- a/fs/reiserfs/fix_node.c
+++ b/fs/reiserfs/fix_node.c
@@ -563,9 +563,6 @@ static int get_num_ver(int mode, struct tree_balance *tb, int h,
 	return needed_nodes;
 }
 
-#ifdef CONFIG_REISERFS_CHECK
-extern struct tree_balance *cur_tb;
-#endif
 
 /* Set parameters for balancing.
  * Performs write of results of analysis of balancing into structure tb,
@@ -2368,7 +2365,7 @@ int fix_nodes(int op_mode, struct tree_balance *tb,
 			return REPEAT_SEARCH;
 	}
 #ifdef CONFIG_REISERFS_CHECK
-	if (cur_tb) {
+	if (REISERFS_SB(tb->tb_sb)->cur_tb) {
 		print_cur_tb("fix_nodes");
 		reiserfs_panic(tb->tb_sb, "PAP-8305",
 			       "there is pending do_balance");
diff --git a/fs/reiserfs/prints.c b/fs/reiserfs/prints.c
index 536eaca..adbc6f5 100644
--- a/fs/reiserfs/prints.c
+++ b/fs/reiserfs/prints.c
@@ -349,10 +349,6 @@ void reiserfs_debug(struct super_block *s, int level, const char *fmt, ...)
 
    .  */
 
-#ifdef CONFIG_REISERFS_CHECK
-extern struct tree_balance *cur_tb;
-#endif
-
 void __reiserfs_panic(struct super_block *sb, const char *id,
 		      const char *function, const char *fmt, ...)
 {
diff --git a/fs/reiserfs/stree.c b/fs/reiserfs/stree.c
index 6b025a4..5fa7118 100644
--- a/fs/reiserfs/stree.c
+++ b/fs/reiserfs/stree.c
@@ -222,9 +222,6 @@ static inline int bin_search(const void *key,	/* Key to search for. */
 	return ITEM_NOT_FOUND;
 }
 
-#ifdef CONFIG_REISERFS_CHECK
-extern struct tree_balance *cur_tb;
-#endif
 
 /* Minimal possible key. It is never in the tree. */
 const struct reiserfs_key MIN_KEY = { 0, 0, {{0, 0},} };
@@ -711,7 +708,7 @@ int search_by_key(struct super_block *sb, const struct cpu_key *key,	/* Key to s
 		       !key_in_buffer(search_path, key, sb),
 		       "PAP-5130: key is not in the buffer");
 #ifdef CONFIG_REISERFS_CHECK
-		if (cur_tb) {
+		if (REISERFS_SB(sb)->cur_tb) {
 			print_cur_tb("5140");
 			reiserfs_panic(sb, "PAP-5140",
 				       "schedule occurred in do_balance!");
diff --git a/include/linux/reiserfs_fs_sb.h b/include/linux/reiserfs_fs_sb.h
index 8a1d409..2f8bb04 100644
--- a/include/linux/reiserfs_fs_sb.h
+++ b/include/linux/reiserfs_fs_sb.h
@@ -417,6 +417,17 @@ struct reiserfs_sb_info {
 	char *s_qf_names[MAXQUOTAS];
 	int s_jquota_fmt;
 #endif
+#ifdef CONFIG_REISERFS_CHECK
+
+	struct tree_balance *cur_tb;	/*
+					 * Detects whether more than one
+					 * copy of tb exists per superblock
+					 * as a means of checking whether
+					 * do_balance is executing concurrently
+					 * against another tree reader/writer
+					 * on a same mount point.
+					 */
+#endif
 };
 
 /* Definitions of reiserfs on-disk properties: */
-- 
1.6.2.3