xfs
[Top] [All Lists]

Re: [LOCKDEP] xfs: possible recursive locking detected

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


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