[LOCKDEP] xfs: possible recursive locking detected

[LOCKDEP] xfs: possible recursive locking detected

[Alexey Dobriyan]

2.6.17-912b2539e1e062cec73e2e61448e507f7719bd08

While trying to remove 2 small files, 2 empty dirs and 1 empty dir on
xfs partition

=============================================
[ INFO: possible recursive locking detected ]
---------------------------------------------
rm/7596 is trying to acquire lock:
 (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e

but task is already holding lock:
 (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e

other info that might help us debug this:
3 locks held by rm/7596:
 #0:  (&inode->i_mutex/1){--..}, at: [<c0155ff9>] do_rmdir+0x6c/0xc3
 #1:  (&inode->i_mutex){--..}, at: [<c02b21a0>] mutex_lock+0x8/0xa
 #2:  (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e

stack backtrace:
 [<c0102b4c>] show_trace+0xd/0xf
 [<c0102c1e>] dump_stack+0x17/0x19
 [<c012558c>] print_deadlock_bug+0x94/0x9e
 [<c01255d4>] check_deadlock+0x3e/0x52
 [<c0126e34>] __lock_acquire+0x747/0x7ea
 [<c012746f>] lock_acquire+0x5e/0x7e
 [<c01241b8>] down_write+0x19/0x33
 [<c01d3d0d>] xfs_ilock+0x4d/0x6e
 [<c01eefae>] xfs_lock_dir_and_entry+0x8e/0xc8
 [<c01efeb5>] xfs_rmdir+0x1ce/0x3c9
 [<c01f8a45>] xfs_vn_rmdir+0x1c/0x44
 [<c0155f57>] vfs_rmdir+0x5c/0x92
 [<c0156019>] do_rmdir+0x8c/0xc3
 [<c0156060>] sys_rmdir+0x10/0x12
 [<c0102677>] syscall_call+0x7/0xb

Re: [LOCKDEP] xfs: possible recursive locking detected

[Matthew Wilcox]

On Tue, Jul 04, 2006 at 04:41:16AM +0400, Alexey Dobriyan wrote:
> 2.6.17-912b2539e1e062cec73e2e61448e507f7719bd08
> 
> While trying to remove 2 small files, 2 empty dirs and 1 empty dir on
> xfs partition

Probably spurious.  xfs_ilock can be called on both the parent and child,
which wouldn't be a deadlock.

> =============================================
> [ INFO: possible recursive locking detected ]
> ---------------------------------------------
> rm/7596 is trying to acquire lock:
>  (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e
> 
> but task is already holding lock:
>  (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e
> 
> other info that might help us debug this:
> 3 locks held by rm/7596:
>  #0:  (&inode->i_mutex/1){--..}, at: [<c0155ff9>] do_rmdir+0x6c/0xc3
>  #1:  (&inode->i_mutex){--..}, at: [<c02b21a0>] mutex_lock+0x8/0xa
>  #2:  (&(&ip->i_lock)->mr_lock){----}, at: [<c01d3d0d>] xfs_ilock+0x4d/0x6e
> 
> stack backtrace:
>  [<c0102b4c>] show_trace+0xd/0xf
>  [<c0102c1e>] dump_stack+0x17/0x19
>  [<c012558c>] print_deadlock_bug+0x94/0x9e
>  [<c01255d4>] check_deadlock+0x3e/0x52
>  [<c0126e34>] __lock_acquire+0x747/0x7ea
>  [<c012746f>] lock_acquire+0x5e/0x7e
>  [<c01241b8>] down_write+0x19/0x33
>  [<c01d3d0d>] xfs_ilock+0x4d/0x6e
>  [<c01eefae>] xfs_lock_dir_and_entry+0x8e/0xc8
>  [<c01efeb5>] xfs_rmdir+0x1ce/0x3c9
>  [<c01f8a45>] xfs_vn_rmdir+0x1c/0x44
>  [<c0155f57>] vfs_rmdir+0x5c/0x92
>  [<c0156019>] do_rmdir+0x8c/0xc3
>  [<c0156060>] sys_rmdir+0x10/0x12
>  [<c0102677>] syscall_call+0x7/0xb
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Mon, Jul 03, 2006 at 07:18:58PM -0600, Matthew Wilcox wrote:
> On Tue, Jul 04, 2006 at 04:41:16AM +0400, Alexey Dobriyan wrote:
> > 2.6.17-912b2539e1e062cec73e2e61448e507f7719bd08
> > 
> > While trying to remove 2 small files, 2 empty dirs and 1 empty dir on
> > xfs partition
> 
> Probably spurious.  xfs_ilock can be called on both the parent and child,
> which wouldn't be a deadlock.

Hmm... they'd be different inodes though, so different lock addresses
in memory - is lockdep taking that into account?  Would we need to go
annotate xfs_ilock somehow to give better hints to the lockdep code?

thanks.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

* Nathan Scott <nathans@sgi.com> wrote:

> > > While trying to remove 2 small files, 2 empty dirs and 1 empty dir 
> > > on xfs partition
> > 
> > Probably spurious.  xfs_ilock can be called on both the parent and 
> > child, which wouldn't be a deadlock.
> 
> Hmm... they'd be different inodes though, so different lock addresses 
> in memory - is lockdep taking that into account?  Would we need to go 
> annotate xfs_ilock somehow to give better hints to the lockdep code?

correct, lockdep has to be taught about relations between locks within 
the same lock-class. (it detects relations between different 
lock-classes automatically) It's usually a straightforward process.

In this particular case we probably need to do something similar to the 
VFS's 'enum inode_i_mutex_lock_class' subclass categorizations: we need 
xfs_ilock_nested(.., subclass), where in xfs_lock_dir_and_entry() we'd 
pass in ILOCK_PARENT. [normal locking calls have a default subclass ID 
of 0]

I suspect simply creating an XFS filesystem and doing a couple of VFS 
ops on it should trigger these locking patterns?

	Ingo

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Tue, Jul 04, 2006 at 08:32:26AM +0200, Ingo Molnar wrote:
> 
> * Nathan Scott <nathans@sgi.com> wrote:
> 
> > > > While trying to remove 2 small files, 2 empty dirs and 1 empty dir 
> > > > on xfs partition
> > > 
> > > Probably spurious.  xfs_ilock can be called on both the parent and 
> > > child, which wouldn't be a deadlock.
> > 
> > Hmm... they'd be different inodes though, so different lock addresses 
> > in memory - is lockdep taking that into account?  Would we need to go 
> > annotate xfs_ilock somehow to give better hints to the lockdep code?
> 
> correct, lockdep has to be taught about relations between locks within 
> the same lock-class. (it detects relations between different 
> lock-classes automatically) It's usually a straightforward process.
> 
> In this particular case we probably need to do something similar to the 
> VFS's 'enum inode_i_mutex_lock_class' subclass categorizations: we need 
> xfs_ilock_nested(.., subclass), where in xfs_lock_dir_and_entry() we'd 
> pass in ILOCK_PARENT. [normal locking calls have a default subclass ID 
> of 0]
> 
> I suspect simply creating an XFS filesystem and doing a couple of VFS 
> ops on it should trigger these locking patterns?

Yep, looks like its really easy to trigger - I pulled Linus' tree,
enabled everything I could see that looked lockdep related and I
immediately saw warnings during bootup... that was with an XFS root,
should be able to hit it pretty quickly with any simple filesystem
interaction though (root or not).

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > > > While trying to remove 2 small files, 2 empty dirs and 1 empty dir 
> > > > on xfs partition
> > > 
> > > Probably spurious.  xfs_ilock can be called on both the parent and 
> > > child, which wouldn't be a deadlock.
> > 
> > Hmm... they'd be different inodes though, so different lock addresses 
> > in memory - is lockdep taking that into account?  Would we need to go 
> > annotate xfs_ilock somehow to give better hints to the lockdep code?
> 
> correct, lockdep has to be taught about relations between locks within 
> the same lock-class. (it detects relations between different 
> lock-classes automatically) It's usually a straightforward process.
> 
> In this particular case we probably need to do something similar to 
> the VFS's 'enum inode_i_mutex_lock_class' subclass categorizations: we 
> need xfs_ilock_nested(.., subclass), where in xfs_lock_dir_and_entry() 
> we'd pass in ILOCK_PARENT. [normal locking calls have a default 
> subclass ID of 0]

the patch below should solve the case mentioned above, but i'm sure 
there will be other cases as well.

XFS locking seems quite complex all around. All this dynamic 
flag-passing into an opaque function (such as xfs_ilock), just to have 
them untangled in xfs_ilock():

        if (lock_flags & XFS_IOLOCK_EXCL) {
                mrupdate(&ip->i_iolock);
        } else if (lock_flags & XFS_IOLOCK_SHARED) {
                mraccess(&ip->i_iolock);
        }
        if (lock_flags & XFS_ILOCK_EXCL) {
                mrupdate(&ip->i_lock);
        } else if (lock_flags & XFS_ILOCK_SHARED) {
                mraccess(&ip->i_lock);
        }

is pretty inefficient too - there are 85 calls to xfs_ilock(), and 74 of 
them have static flags. 

Also, mrunlock() does:

 static inline void mrunlock(mrlock_t *mrp)
 {
         if (mrp->mr_writer) {
                 mrp->mr_writer = 0;
                 up_write(&mrp->mr_lock);
         } else {
                 up_read(&mrp->mr_lock);
         }
 }

while xfs_iunlock() knows whether the release is for read or for write! 
So ->mr_writer seems like a totally unnecessary layer. In fact basically 
all codepaths should be well aware of whether they locked the inode for 
read or for write.

i'd really suggest to clean this up and to convert:

	xfs_ilock(ip, XFS_ILOCK_EXCL);

to:

	down_write(&ip->i_lock);

and:

	xfs_iunlock(ip, XFS_ILOCK_EXCL);

to:

	up_write(&ip->i_lock);

and eliminate all those layers of indirection.

	Ingo

---
 fs/xfs/linux-2.6/mrlock.h |   33 +++++++++++++++++++++++++++++++++
 fs/xfs/xfs_iget.c         |   12 ++++++++----
 fs/xfs/xfs_inode.h        |    3 ++-
 fs/xfs/xfs_vnodeops.c     |    8 ++++----
 4 files changed, 47 insertions(+), 9 deletions(-)

Index: linux/fs/xfs/linux-2.6/mrlock.h
===================================================================
--- linux.orig/fs/xfs/linux-2.6/mrlock.h
+++ linux/fs/xfs/linux-2.6/mrlock.h
@@ -27,12 +27,33 @@ typedef struct {
 	int			mr_writer;
 } mrlock_t;
 
+/*
+ * ilock nesting subclasses for the lock validator:
+ *
+ * 0: the object of the current VFS operation
+ * 1: parent
+ * 2: child/target
+ * 3: quota file
+ *
+ * The locking order between these classes is
+ * parent -> child -> normal -> quota
+ */
+enum xfs_ilock_class
+{
+	ILOCK_NORMAL,
+	ILOCK_PARENT,
+	ILOCK_CHILD,
+	ILOCK_QUOTA
+};
+
 #define mrinit(mrp, name)	\
 	do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
 #define mrlock_init(mrp, t,n,s)	mrinit(mrp, n)
 #define mrfree(mrp)		do { } while (0)
 #define mraccess(mrp)		mraccessf(mrp, 0)
+#define mraccess_nested(mrp, s)	mraccessf_nested(mrp, 0, s)
 #define mrupdate(mrp)		mrupdatef(mrp, 0)
+#define mrupdate_nested(mrp, s)	mrupdatef_nested(mrp, 0, s)
 
 static inline void mraccessf(mrlock_t *mrp, int flags)
 {
@@ -45,6 +66,18 @@ static inline void mrupdatef(mrlock_t *m
 	mrp->mr_writer = 1;
 }
 
+static inline void mraccessf_nested(mrlock_t *mrp, int flags, int subclass)
+{
+	down_read_nested(&mrp->mr_lock, subclass);
+}
+
+static inline void mrupdatef_nested(mrlock_t *mrp, int flags, int subclass)
+{
+	down_write_nested(&mrp->mr_lock, subclass);
+	mrp->mr_writer = 1;
+}
+
+
 static inline int mrtryaccess(mrlock_t *mrp)
 {
 	return down_read_trylock(&mrp->mr_lock);
Index: linux/fs/xfs/xfs_iget.c
===================================================================
--- linux.orig/fs/xfs/xfs_iget.c
+++ linux/fs/xfs/xfs_iget.c
@@ -859,10 +859,14 @@ xfs_ilock(xfs_inode_t	*ip,
 	} else if (lock_flags & XFS_IOLOCK_SHARED) {
 		mraccess(&ip->i_iolock);
 	}
-	if (lock_flags & XFS_ILOCK_EXCL) {
-		mrupdate(&ip->i_lock);
-	} else if (lock_flags & XFS_ILOCK_SHARED) {
-		mraccess(&ip->i_lock);
+	if (lock_flags & XFS_ILOCK_EXCL_PARENT)
+		mrupdate_nested(&ip->i_lock, ILOCK_PARENT);
+	else {
+		if (lock_flags & XFS_ILOCK_EXCL) {
+			mrupdate(&ip->i_lock);
+		} else if (lock_flags & XFS_ILOCK_SHARED) {
+			mraccess(&ip->i_lock);
+		}
 	}
 	xfs_ilock_trace(ip, 1, lock_flags, (inst_t *)__return_address);
 }
Index: linux/fs/xfs/xfs_inode.h
===================================================================
--- linux.orig/fs/xfs/xfs_inode.h
+++ linux/fs/xfs/xfs_inode.h
@@ -352,11 +352,12 @@ typedef struct xfs_inode {
 #define XFS_SIZE_TOKEN_WR       (XFS_SIZE_TOKEN_RD | XFS_WILLLEND)
 #define XFS_EXTSIZE_WR		(XFS_EXTSIZE_RD | XFS_WILLLEND)
 /*	XFS_SIZE_TOKEN_WANT	0x200 */
+#define	XFS_ILOCK_EXCL_PARENT	0x400
 
 #define XFS_LOCK_MASK	\
 	(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED | XFS_ILOCK_EXCL | \
 	 XFS_ILOCK_SHARED | XFS_EXTENT_TOKEN_RD | XFS_SIZE_TOKEN_RD | \
-	 XFS_WILLLEND)
+	 XFS_WILLLEND | XFS_ILOCK_EXCL_PARENT)
 
 /*
  * Flags for xfs_iflush()
Index: linux/fs/xfs/xfs_vnodeops.c
===================================================================
--- linux.orig/fs/xfs/xfs_vnodeops.c
+++ linux/fs/xfs/xfs_vnodeops.c
@@ -1942,7 +1942,7 @@ xfs_create(
 		goto error_return;
 	}
 
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+	xfs_ilock(dp, XFS_ILOCK_EXCL_PARENT);
 
 	XFS_BMAP_INIT(&free_list, &first_block);
 
@@ -2137,7 +2137,7 @@ xfs_lock_dir_and_entry(
 	attempts = 0;
 
 again:
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+	xfs_ilock(dp, XFS_ILOCK_EXCL_PARENT);
 
 	e_inum = ip->i_ino;
 
@@ -2836,7 +2836,7 @@ xfs_mkdir(
 		goto error_return;
 	}
 
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+	xfs_ilock(dp, XFS_ILOCK_EXCL_PARENT);
 
 	/*
 	 * Check for directory link count overflow.
@@ -3385,7 +3385,7 @@ xfs_symlink(
 		goto error_return;
 	}
 
-	xfs_ilock(dp, XFS_ILOCK_EXCL);
+	xfs_ilock(dp, XFS_ILOCK_EXCL_PARENT);
 
 	/*
 	 * Check whether the directory allows new symlinks or not.

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Tue, Jul 04, 2006 at 10:41:43AM +0200, Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> the patch below should solve the case mentioned above, but i'm sure 
> there will be other cases as well.

Thanks Ingo!  I'll run with this for awhile and see if I can find
any other issues.

> XFS locking seems quite complex all around. All this dynamic 

Mhmm.

> flag-passing into an opaque function (such as xfs_ilock), just to have 
> them untangled in xfs_ilock():
> 
>         if (lock_flags & XFS_IOLOCK_EXCL) {
>                 mrupdate(&ip->i_iolock);
>         } else if (lock_flags & XFS_IOLOCK_SHARED) {
>                 mraccess(&ip->i_iolock);
>         }
>         if (lock_flags & XFS_ILOCK_EXCL) {
>                 mrupdate(&ip->i_lock);
>         } else if (lock_flags & XFS_ILOCK_SHARED) {
>                 mraccess(&ip->i_lock);
>         }
> 
> is pretty inefficient too - there are 85 calls to xfs_ilock(), and 74 of 
> them have static flags. 

Right... but that leaves plenty that don't, and they're not simple
to change.  There are generic routines that need to be called from
different contexts with different locking requirements (xfs_iget).

> while xfs_iunlock() knows whether the release is for read or for write! 
> So ->mr_writer seems like a totally unnecessary layer. In fact basically 

Hmm, I did look into removing that sometime back, but didn't get
there for some reason that escapes me now... (I have a vague memory
of the use of downgrade_write messing me up there).  I'll take
another look when I get some time, it would be beneficial to remove
that if we possibly can.

> i'd really suggest to clean this up and to convert:
> 	xfs_ilock(ip, XFS_ILOCK_EXCL);
> to:
> 	down_write(&ip->i_lock);
> and:
> 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> to:
> 	up_write(&ip->i_lock);
> and eliminate all those layers of indirection.

That would be good, but it doesn't work for all situations
unfortunately, and it would loose that debug-kernel sanity
checking that we have in there which validates ilock/iolock
ordering rules.

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

* Nathan Scott <nathans@sgi.com> wrote:

> > i'd really suggest to clean this up and to convert:
> > 	xfs_ilock(ip, XFS_ILOCK_EXCL);
> > to:
> > 	down_write(&ip->i_lock);
> > and:
> > 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> > to:
> > 	up_write(&ip->i_lock);
> > and eliminate all those layers of indirection.
> 
> That would be good, but it doesn't work for all situations 
> unfortunately, and it would loose that debug-kernel sanity checking 
> that we have in there which validates ilock/iolock ordering rules.

do you have anything in there that spinlock/mutex debugging or lockdep 
does not catch? If yes then i'll add it to the generic lock debugging 
code.

	Ingo

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Tue, Jul 04, 2006 at 11:12:47AM +0200, Ingo Molnar wrote:
> * Nathan Scott <nathans@sgi.com> wrote:
> > That would be good, but it doesn't work for all situations 
> > unfortunately, and it would loose that debug-kernel sanity checking 
> > that we have in there which validates ilock/iolock ordering rules.
> 
> do you have anything in there that spinlock/mutex debugging or lockdep 
> does not catch? If yes then i'll add it to the generic lock debugging 
> code.

The thing we're catching automatically there is potential ordering
violations on the XFS inode iolock vs ilock.  I don't know if the
other methods can help us there too or not.

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Arjan van de Ven]

On Wed, 2006-07-05 at 13:23 +1000, Nathan Scott wrote:
> On Tue, Jul 04, 2006 at 11:12:47AM +0200, Ingo Molnar wrote:
> > * Nathan Scott <nathans@sgi.com> wrote:
> > > That would be good, but it doesn't work for all situations 
> > > unfortunately, and it would loose that debug-kernel sanity checking 
> > > that we have in there which validates ilock/iolock ordering rules.
> > 
> > do you have anything in there that spinlock/mutex debugging or lockdep 
> > does not catch? If yes then i'll add it to the generic lock debugging 
> > code.
> 
> The thing we're catching automatically there is potential ordering
> violations on the XFS inode iolock vs ilock.  

lockdep will catch those just fine.

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

* Nathan Scott <nathans@sgi.com> wrote:

> > flag-passing into an opaque function (such as xfs_ilock), just to have 
> > them untangled in xfs_ilock():
> > 
> >         if (lock_flags & XFS_IOLOCK_EXCL) {
> >                 mrupdate(&ip->i_iolock);
> >         } else if (lock_flags & XFS_IOLOCK_SHARED) {
> >                 mraccess(&ip->i_iolock);
> >         }
> >         if (lock_flags & XFS_ILOCK_EXCL) {
> >                 mrupdate(&ip->i_lock);
> >         } else if (lock_flags & XFS_ILOCK_SHARED) {
> >                 mraccess(&ip->i_lock);
> >         }
> > 
> > is pretty inefficient too - there are 85 calls to xfs_ilock(), and 
> > 74 of them have static flags.
> 
> Right... but that leaves plenty that don't, and they're not simple to 
> change.  There are generic routines that need to be called from 
> different contexts with different locking requirements (xfs_iget).

the main variation in xfs_iget() is whether we lock the inode 
read-write, read-only or not at all, correct? (XFS_ILOCK_EXCL, 
XFS_ILOCK_SHARED and 0)

That could be cleaned up the following way:

- rename the current xfs_iget() to __xfs_iget() and remove ilock locking 
  from it.

- add 3 new APIs: xfs_iget_read(), xfs_iget_write() and 
  xfs_iget_nolock():

   - xfs_iget_read() just calls __xfs_iget() and does a down_read() if 
     the inode was looked up successfully.

   - xfs_iget_write() does the same but with down_write()

   - xfs_iget_nolock() is just an alias to __xfs_iget().

 - update all 13 uses of xfs_iget() to one of the 3 API variants

 - [ there might be other details i missed, but this seems to be the 
     rough list of things to do. ]

NOTE: since the majority (9 out of 13) of xfs_iget() uses are for the 
'no lock' variant, this construction of functions, besides making the 
code more readable, _further_ reduces overhead, because there is no 
ilock-flags checking overhead in __xfs_iget() anymore.

	Ingo

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

another thing: i have added real 'lock allocation debugging' 
(CONFIG_DEBUG_LOCK_ALLOC) to the kernel, which covers spinlocks, 
rwlocks, mutexes and rw-semaphores. It does the following:

         This feature will check whether any held lock (spinlock, rwlock,
         mutex or rwsem) is incorrectly freed by the kernel, via any of the
         memory-freeing routines (kfree(), kmem_cache_free(), free_pages(),
         vfree(), etc.), whether a live lock is incorrectly reinitialized via
         spin_lock_init()/mutex_init()/etc., or whether there is any lock
         held during task exit.

so i suspect:

 fs/xfs/xfs_mount.h:#define   AIL_LOCK_DESTROY(x)     spinlock_destroy(x)
 fs/xfs/linux-2.6/spin.h:#define      spinlock_destroy(lock)

needs to change and we need to implement spinlock_destroy(), a'ka 
mutex_destroy()? [which i added recently too]

	Ingo

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Tue, Jul 04, 2006 at 03:03:38PM +0200, Ingo Molnar wrote:
> so i suspect:
> 
>  fs/xfs/xfs_mount.h:#define   AIL_LOCK_DESTROY(x)     spinlock_destroy(x)
>  fs/xfs/linux-2.6/spin.h:#define      spinlock_destroy(lock)
> 
> needs to change and we need to implement spinlock_destroy(), a'ka 
> mutex_destroy()? [which i added recently too]

Hmm, don't think so - only if you needed to change all other spinlock
uses in the kernel to have a teardown too?  Can't see that in current
git trees, anyway, so I expect that to be OK as is.

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar]

* Nathan Scott <nathans@sgi.com> wrote:

> >  fs/xfs/xfs_mount.h:#define   AIL_LOCK_DESTROY(x)     spinlock_destroy(x)
> >  fs/xfs/linux-2.6/spin.h:#define      spinlock_destroy(lock)
> > 
> > needs to change and we need to implement spinlock_destroy(), a'ka 
> > mutex_destroy()? [which i added recently too]
> 
> Hmm, don't think so - only if you needed to change all other spinlock 
> uses in the kernel to have a teardown too?  Can't see that in current 
> git trees, anyway, so I expect that to be OK as is.

i should have formulated this as a question: should i implement 
spin_lock_destroy()? A few months ago i implemented mutex_destroy() for 
XFS's use, and now we could do it for spinlocks too.

Right now the only upstream requirement wrt. spinlock disposal is that 
it should not be in locked state when it's destroyed. (PREEMPT_RT in the 
-rt tree needed that semantic detail too and there were a handful of 
places in the kernel that freed held locks - we fixed those up in the
past year or so.)

spin_lock_destroy() would work like mutex_destroy(): the magic number in 
the lock is overwritten and hence no further locking API will allow the 
use of that lock from that point on. (up until the lock is reinitialized 
via spin_lock_init())

	Ingo

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Wed, Jul 05, 2006 at 08:46:51AM +0200, Ingo Molnar wrote:
> 
> i should have formulated this as a question: should i implement 
> spin_lock_destroy()? A few months ago i implemented mutex_destroy() for 
> XFS's use, and now we could do it for spinlocks too.
> ...
> spin_lock_destroy() would work like mutex_destroy(): the magic number in 
> the lock is overwritten and hence no further locking API will allow the 
> use of that lock from that point on. (up until the lock is reinitialized 
> via spin_lock_init())

Oh, right, I see - yes, I think that could be generally useful
and we'd get some value out of that in XFS for sure.  Thanks.

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Nathan Scott]

On Tue, Jul 04, 2006 at 11:57:43AM +0200, Ingo Molnar wrote:
> > Right... but that leaves plenty that don't, and they're not simple to 
> > change.  There are generic routines that need to be called from 
> > different contexts with different locking requirements (xfs_iget).
> 
> the main variation in xfs_iget() is whether we lock the inode 
> read-write, read-only or not at all, correct? (XFS_ILOCK_EXCL, 
> XFS_ILOCK_SHARED and 0)
> 
> That could be cleaned up the following way:

*nod*.  One difficulty is that xfs_iget_core would also need this
treatment (the lock_mode parameter is passed down there), and we
may end up be with quite a few functions and/or duplicated code.
But maybe that can be avoided by arranging that code differently.

> NOTE: since the majority (9 out of 13) of xfs_iget() uses are for the 
> 'no lock' variant, this construction of functions, besides making the 
> code more readable, _further_ reduces overhead, because there is no 
> ilock-flags checking overhead in __xfs_iget() anymore.

Indeed; its fairly minimal overhead though really, the readability
angle appeals to me more.  Its just a fair bit of churn for not a
very tangible gain, so I'm balking at it atm.  Russell is looking
at reworking xfs_iget for other reasons, so maybe he can stew on
all of this and clean it up in the context of his other changes in
there.

Thanks Ingo.

cheers.

-- 
Nathan

Re: [LOCKDEP] xfs: possible recursive locking detected

[Andi Kleen]

Nathan Scott <nathans@sgi.com> writes:
> 
> That would be good, but it doesn't work for all situations
> unfortunately, and it would loose that debug-kernel sanity
> checking that we have in there which validates ilock/iolock
> ordering rules.

Isn't that obsolete now with lockdep?

-Andi