xfs
[Top] [All Lists]

Re: [LOCKDEP] xfs: possible recursive locking detected

To: Nathan Scott <nathans@xxxxxxx>
Subject: Re: [LOCKDEP] xfs: possible recursive locking detected
From: Ingo Molnar <mingo@xxxxxxx>
Date: Tue, 4 Jul 2006 11:57:43 +0200
Cc: Alexey Dobriyan <adobriyan@xxxxxxxxx>, Matthew Wilcox <matthew@xxxxxx>, linux-fsdevel@xxxxxxxxxxxxxxx, xfs@xxxxxxxxxxx, Arjan van de Ven <arjan@xxxxxxxxxxxxx>
In-reply-to: <20060704191100.C1497438@xxxxxxxxxxxxxxxxxxxxxxxx>
References: <20060704004116.GA7612@xxxxxxxxxxxxxxxxxxxxxx> <20060704011858.GG1605@xxxxxxxxxxxxxxxx> <20060704112503.H1495869@xxxxxxxxxxxxxxxxxxxxxxxx> <20060704063225.GA2752@xxxxxxx> <20060704084143.GA12931@xxxxxxx> <20060704191100.C1497438@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
* Nathan Scott <nathans@xxxxxxx> 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


<Prev in Thread] Current Thread [Next in Thread>