Re: [LOCKDEP] xfs: possible recursive locking detected

[Ingo Molnar <mingo@elte.hu>]

* 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