3.1.0-rc3 -- INFO: possible circular locking dependency detected

3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Miles Lane]

[ INFO: possible circular locking dependency detected ]
3.1.0-rc3 #2
-------------------------------------------------------
dconf-service/1836 is trying to acquire lock:
 (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<ffffffff8116df1a>]
ext4_evict_inode+0x88/0x32b

 but task is already holding lock:
 (&mm->mmap_sem){++++++}, at: [<ffffffff810d4393>] sys_munmap+0x36/0x5b

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&mm->mmap_sem){++++++}:
       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
       [<ffffffff810cddbd>] might_fault+0x68/0x8b
       [<ffffffff810fcf5e>] filldir+0x6a/0xc2
       [<ffffffff811651a1>] call_filldir+0x91/0xb8
       [<ffffffff811654bf>] ext4_readdir+0x1af/0x510
       [<ffffffff810fd1a4>] vfs_readdir+0x76/0xac
       [<ffffffff810fd2b6>] sys_getdents+0x79/0xc9
       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b

-> #0 (&sb->s_type->i_mutex_key#12){+.+.+.}:
       [<ffffffff81068b10>] __lock_acquire+0xa5e/0xd52
       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
       [<ffffffff8140f1a2>] __mutex_lock_common+0x64/0x413
       [<ffffffff8140f5b0>] mutex_lock_nested+0x16/0x18
       [<ffffffff8116df1a>] ext4_evict_inode+0x88/0x32b
       [<ffffffff81102d8a>] evict+0x94/0x14e
       [<ffffffff81102fd0>] iput+0x18c/0x195
       [<ffffffff810ffdd4>] dentry_kill+0x11e/0x140
       [<ffffffff8110019b>] dput+0xd4/0xe4
       [<ffffffff810efac3>] fput+0x1a5/0x1bd
       [<ffffffff810d3214>] remove_vma+0x37/0x5f
       [<ffffffff810d4239>] do_munmap+0x2ed/0x306
       [<ffffffff810d43a1>] sys_munmap+0x44/0x5b
       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_sem);
                               lock(&sb->s_type->i_mutex_key);
                               lock(&mm->mmap_sem);
  lock(&sb->s_type->i_mutex_key);

 *** DEADLOCK ***

1 lock held by dconf-service/1836:
 #0:  (&mm->mmap_sem){++++++}, at: [<ffffffff810d4393>] sys_munmap+0x36/0x5b

stack backtrace:
Pid: 1836, comm: dconf-service Tainted: G        W   3.1.0-rc3 #2
Call Trace:
 [<ffffffff81407d14>] print_circular_bug+0x1f8/0x209
 [<ffffffff81068b10>] __lock_acquire+0xa5e/0xd52
 [<ffffffff8116df1a>] ? ext4_evict_inode+0x88/0x32b
 [<ffffffff8106933a>] lock_acquire+0x129/0x14e
 [<ffffffff8116df1a>] ? ext4_evict_inode+0x88/0x32b
 [<ffffffff8140f1a2>] __mutex_lock_common+0x64/0x413
 [<ffffffff8116df1a>] ? ext4_evict_inode+0x88/0x32b
 [<ffffffff81102d5a>] ? evict+0x64/0x14e
 [<ffffffff8116df1a>] ? ext4_evict_inode+0x88/0x32b
 [<ffffffff8102fa1c>] ? get_parent_ip+0xe/0x3e
 [<ffffffff8140f5b0>] mutex_lock_nested+0x16/0x18
 [<ffffffff8116df1a>] ext4_evict_inode+0x88/0x32b
 [<ffffffff8116de92>] ? ext4_da_writepages+0x53f/0x53f
 [<ffffffff81102d8a>] evict+0x94/0x14e
 [<ffffffff81102fd0>] iput+0x18c/0x195
 [<ffffffff810ffdd4>] dentry_kill+0x11e/0x140
 [<ffffffff8110019b>] dput+0xd4/0xe4
 [<ffffffff810efac3>] fput+0x1a5/0x1bd
 [<ffffffff810d4393>] ? sys_munmap+0x36/0x5b
 [<ffffffff810d3214>] remove_vma+0x37/0x5f
 [<ffffffff810d4239>] do_munmap+0x2ed/0x306
 [<ffffffff810d43a1>] sys_munmap+0x44/0x5b
 [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Josh Boyer]

On Tue, Aug 23, 2011 at 12:39 AM, Miles Lane <miles.lane@gmail.com> wrote:
> [ INFO: possible circular locking dependency detected ]
> 3.1.0-rc3 #2
> -------------------------------------------------------
> dconf-service/1836 is trying to acquire lock:
>  (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<ffffffff8116df1a>]
> ext4_evict_inode+0x88/0x32b
>
>  but task is already holding lock:
>  (&mm->mmap_sem){++++++}, at: [<ffffffff810d4393>] sys_munmap+0x36/0x5b
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&mm->mmap_sem){++++++}:
>       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
>       [<ffffffff810cddbd>] might_fault+0x68/0x8b
>       [<ffffffff810fcf5e>] filldir+0x6a/0xc2
>       [<ffffffff811651a1>] call_filldir+0x91/0xb8
>       [<ffffffff811654bf>] ext4_readdir+0x1af/0x510
>       [<ffffffff810fd1a4>] vfs_readdir+0x76/0xac
>       [<ffffffff810fd2b6>] sys_getdents+0x79/0xc9
>       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
>
> -> #0 (&sb->s_type->i_mutex_key#12){+.+.+.}:
>       [<ffffffff81068b10>] __lock_acquire+0xa5e/0xd52
>       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
>       [<ffffffff8140f1a2>] __mutex_lock_common+0x64/0x413
>       [<ffffffff8140f5b0>] mutex_lock_nested+0x16/0x18
>       [<ffffffff8116df1a>] ext4_evict_inode+0x88/0x32b
>       [<ffffffff81102d8a>] evict+0x94/0x14e
>       [<ffffffff81102fd0>] iput+0x18c/0x195
>       [<ffffffff810ffdd4>] dentry_kill+0x11e/0x140
>       [<ffffffff8110019b>] dput+0xd4/0xe4
>       [<ffffffff810efac3>] fput+0x1a5/0x1bd
>       [<ffffffff810d3214>] remove_vma+0x37/0x5f
>       [<ffffffff810d4239>] do_munmap+0x2ed/0x306
>       [<ffffffff810d43a1>] sys_munmap+0x44/0x5b
>       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
>
> other info that might help us debug this:
>
>  Possible unsafe locking scenario:
>
>       CPU0                    CPU1
>       ----                    ----
>  lock(&mm->mmap_sem);
>                               lock(&sb->s_type->i_mutex_key);
>                               lock(&mm->mmap_sem);
>  lock(&sb->s_type->i_mutex_key);
>
>  *** DEADLOCK ***

This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
and we're hoping Ted (or someone else from the ext4 camp) can comment
on why ext4_evict_inode is holding i_mutex.

josh

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Dave Chinner]

On Tue, Aug 23, 2011 at 07:35:00AM -0400, Josh Boyer wrote:
> On Tue, Aug 23, 2011 at 12:39 AM, Miles Lane <miles.lane@gmail.com> wrote:
> > [ INFO: possible circular locking dependency detected ]
> > 3.1.0-rc3 #2
> > -------------------------------------------------------
> > dconf-service/1836 is trying to acquire lock:
> >  (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<ffffffff8116df1a>]
> > ext4_evict_inode+0x88/0x32b
> >
> >  but task is already holding lock:
> >  (&mm->mmap_sem){++++++}, at: [<ffffffff810d4393>] sys_munmap+0x36/0x5b
> >
> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (&mm->mmap_sem){++++++}:
> >       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
> >       [<ffffffff810cddbd>] might_fault+0x68/0x8b
> >       [<ffffffff810fcf5e>] filldir+0x6a/0xc2
> >       [<ffffffff811651a1>] call_filldir+0x91/0xb8
> >       [<ffffffff811654bf>] ext4_readdir+0x1af/0x510
> >       [<ffffffff810fd1a4>] vfs_readdir+0x76/0xac
> >       [<ffffffff810fd2b6>] sys_getdents+0x79/0xc9
> >       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
> >
> > -> #0 (&sb->s_type->i_mutex_key#12){+.+.+.}:
> >       [<ffffffff81068b10>] __lock_acquire+0xa5e/0xd52
> >       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
> >       [<ffffffff8140f1a2>] __mutex_lock_common+0x64/0x413
> >       [<ffffffff8140f5b0>] mutex_lock_nested+0x16/0x18
> >       [<ffffffff8116df1a>] ext4_evict_inode+0x88/0x32b
> >       [<ffffffff81102d8a>] evict+0x94/0x14e
> >       [<ffffffff81102fd0>] iput+0x18c/0x195
> >       [<ffffffff810ffdd4>] dentry_kill+0x11e/0x140
> >       [<ffffffff8110019b>] dput+0xd4/0xe4
> >       [<ffffffff810efac3>] fput+0x1a5/0x1bd
> >       [<ffffffff810d3214>] remove_vma+0x37/0x5f
> >       [<ffffffff810d4239>] do_munmap+0x2ed/0x306
> >       [<ffffffff810d43a1>] sys_munmap+0x44/0x5b
> >       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
> >
> > other info that might help us debug this:
> >
> >  Possible unsafe locking scenario:
> >
> >       CPU0                    CPU1
> >       ----                    ----
> >  lock(&mm->mmap_sem);
> >                               lock(&sb->s_type->i_mutex_key);
> >                               lock(&mm->mmap_sem);
> >  lock(&sb->s_type->i_mutex_key);
> >
> >  *** DEADLOCK ***
> 
> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
> and we're hoping Ted (or someone else from the ext4 camp) can comment
> on why ext4_evict_inode is holding i_mutex.

Actually, the problem has nothing to do with ext4. the problem is
that remove_vma() is holding the mmap_sem while calling fput(). The
correct locking order is i_mutex->mmap_sem, as documented in
mm/filemap.c:

 *  ->i_mutex                   (generic_file_buffered_write)
 *    ->mmap_sem	        (fault_in_pages_readable->do_page_fault)


The way remove_vma() calls fput() also triggers lockdep reports in
XFS and it will do so with any filesystem that takes an inode
specific lock in it's evict() processing. IOWs, remove_vma() needs
fixing, not ext4....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Josh Boyer]

On Tue, Aug 23, 2011 at 7:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Aug 23, 2011 at 07:35:00AM -0400, Josh Boyer wrote:
>> On Tue, Aug 23, 2011 at 12:39 AM, Miles Lane <miles.lane@gmail.com> wrote:
>> > [ INFO: possible circular locking dependency detected ]
>> > 3.1.0-rc3 #2
>> > -------------------------------------------------------
>> > dconf-service/1836 is trying to acquire lock:
>> >  (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<ffffffff8116df1a>]
>> > ext4_evict_inode+0x88/0x32b
>> >
>> >  but task is already holding lock:
>> >  (&mm->mmap_sem){++++++}, at: [<ffffffff810d4393>] sys_munmap+0x36/0x5b
>> >
>> > which lock already depends on the new lock.
>> >
>> > the existing dependency chain (in reverse order) is:
>> >
>> > -> #1 (&mm->mmap_sem){++++++}:
>> >       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
>> >       [<ffffffff810cddbd>] might_fault+0x68/0x8b
>> >       [<ffffffff810fcf5e>] filldir+0x6a/0xc2
>> >       [<ffffffff811651a1>] call_filldir+0x91/0xb8
>> >       [<ffffffff811654bf>] ext4_readdir+0x1af/0x510
>> >       [<ffffffff810fd1a4>] vfs_readdir+0x76/0xac
>> >       [<ffffffff810fd2b6>] sys_getdents+0x79/0xc9
>> >       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
>> >
>> > -> #0 (&sb->s_type->i_mutex_key#12){+.+.+.}:
>> >       [<ffffffff81068b10>] __lock_acquire+0xa5e/0xd52
>> >       [<ffffffff8106933a>] lock_acquire+0x129/0x14e
>> >       [<ffffffff8140f1a2>] __mutex_lock_common+0x64/0x413
>> >       [<ffffffff8140f5b0>] mutex_lock_nested+0x16/0x18
>> >       [<ffffffff8116df1a>] ext4_evict_inode+0x88/0x32b
>> >       [<ffffffff81102d8a>] evict+0x94/0x14e
>> >       [<ffffffff81102fd0>] iput+0x18c/0x195
>> >       [<ffffffff810ffdd4>] dentry_kill+0x11e/0x140
>> >       [<ffffffff8110019b>] dput+0xd4/0xe4
>> >       [<ffffffff810efac3>] fput+0x1a5/0x1bd
>> >       [<ffffffff810d3214>] remove_vma+0x37/0x5f
>> >       [<ffffffff810d4239>] do_munmap+0x2ed/0x306
>> >       [<ffffffff810d43a1>] sys_munmap+0x44/0x5b
>> >       [<ffffffff814162fb>] system_call_fastpath+0x16/0x1b
>> >
>> > other info that might help us debug this:
>> >
>> >  Possible unsafe locking scenario:
>> >
>> >       CPU0                    CPU1
>> >       ----                    ----
>> >  lock(&mm->mmap_sem);
>> >                               lock(&sb->s_type->i_mutex_key);
>> >                               lock(&mm->mmap_sem);
>> >  lock(&sb->s_type->i_mutex_key);
>> >
>> >  *** DEADLOCK ***
>>
>> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
>> and we're hoping Ted (or someone else from the ext4 camp) can comment
>> on why ext4_evict_inode is holding i_mutex.
>
> Actually, the problem has nothing to do with ext4. the problem is
> that remove_vma() is holding the mmap_sem while calling fput(). The
> correct locking order is i_mutex->mmap_sem, as documented in
> mm/filemap.c:
>
>  *  ->i_mutex                   (generic_file_buffered_write)
>  *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)
>
>
> The way remove_vma() calls fput() also triggers lockdep reports in
> XFS and it will do so with any filesystem that takes an inode
> specific lock in it's evict() processing. IOWs, remove_vma() needs
> fixing, not ext4....

Er... ok.  So the remove_vma code hasn't changed since 2008.  We're
only seeing this issue now because the debugging code has improved,
or?

At any rate, the proposed solution is to make remove_vma drop mmap_sem
before calling fput, or make it not call fput, or?

josh

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Dave Chinner]

On Tue, Aug 23, 2011 at 07:59:20AM -0400, Josh Boyer wrote:
> On Tue, Aug 23, 2011 at 7:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> >  Possible unsafe locking scenario:
> >> >
> >> >       CPU0                    CPU1
> >> >       ----                    ----
> >> >  lock(&mm->mmap_sem);
> >> >                               lock(&sb->s_type->i_mutex_key);
> >> >                               lock(&mm->mmap_sem);
> >> >  lock(&sb->s_type->i_mutex_key);
> >> >
> >> >  *** DEADLOCK ***
> >>
> >> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
> >> and we're hoping Ted (or someone else from the ext4 camp) can comment
> >> on why ext4_evict_inode is holding i_mutex.
> >
> > Actually, the problem has nothing to do with ext4. the problem is
> > that remove_vma() is holding the mmap_sem while calling fput(). The
> > correct locking order is i_mutex->mmap_sem, as documented in
> > mm/filemap.c:
> >
> >  *  ->i_mutex                   (generic_file_buffered_write)
> >  *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)
> >
> >
> > The way remove_vma() calls fput() also triggers lockdep reports in
> > XFS and it will do so with any filesystem that takes an inode
> > specific lock in it's evict() processing. IOWs, remove_vma() needs
> > fixing, not ext4....
> 
> Er... ok.  So the remove_vma code hasn't changed since 2008.  We're
> only seeing this issue now because the debugging code has improved,
> or?

The problem has been there since at least 2008.  Here's an early
XFS report from 2.6.24:

http://oss.sgi.com/archives/xfs/2008-02/msg00931.html

Here's an XFS report
to match the ext4 one in this thread from 2009:

http://oss.sgi.com/archives/xfs/2009-03/msg00149.html

You won't find reports much older than this - it only started to be
reported when lockdep support in XFS matured and it started to be
widely used....

> At any rate, the proposed solution is to make remove_vma drop mmap_sem
> before calling fput, or make it not call fput, or?

Ask the VM folk - this is the only response I can remember from them
is this:

http://oss.sgi.com/archives/xfs/2009-03/msg00224.html

Maybe now that ext4 is hitting the problem something will be done
about it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Josh Boyer]

On Tue, Aug 23, 2011 at 9:04 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Aug 23, 2011 at 07:59:20AM -0400, Josh Boyer wrote:
>> On Tue, Aug 23, 2011 at 7:49 AM, Dave Chinner <david@fromorbit.com> wrote:
>> >> >  Possible unsafe locking scenario:
>> >> >
>> >> >       CPU0                    CPU1
>> >> >       ----                    ----
>> >> >  lock(&mm->mmap_sem);
>> >> >                               lock(&sb->s_type->i_mutex_key);
>> >> >                               lock(&mm->mmap_sem);
>> >> >  lock(&sb->s_type->i_mutex_key);
>> >> >
>> >> >  *** DEADLOCK ***
>> >>
>> >> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
>> >> and we're hoping Ted (or someone else from the ext4 camp) can comment
>> >> on why ext4_evict_inode is holding i_mutex.
>> >
>> > Actually, the problem has nothing to do with ext4. the problem is
>> > that remove_vma() is holding the mmap_sem while calling fput(). The
>> > correct locking order is i_mutex->mmap_sem, as documented in
>> > mm/filemap.c:
>> >
>> >  *  ->i_mutex                   (generic_file_buffered_write)
>> >  *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)
>> >
>> >
>> > The way remove_vma() calls fput() also triggers lockdep reports in
>> > XFS and it will do so with any filesystem that takes an inode
>> > specific lock in it's evict() processing. IOWs, remove_vma() needs
>> > fixing, not ext4....
>>
>> Er... ok.  So the remove_vma code hasn't changed since 2008.  We're
>> only seeing this issue now because the debugging code has improved,
>> or?
>
> The problem has been there since at least 2008.  Here's an early
> XFS report from 2.6.24:
>
> http://oss.sgi.com/archives/xfs/2008-02/msg00931.html
>
> Here's an XFS report
> to match the ext4 one in this thread from 2009:
>
> http://oss.sgi.com/archives/xfs/2009-03/msg00149.html
>
> You won't find reports much older than this - it only started to be
> reported when lockdep support in XFS matured and it started to be
> widely used....
>
>> At any rate, the proposed solution is to make remove_vma drop mmap_sem
>> before calling fput, or make it not call fput, or?
>
> Ask the VM folk - this is the only response I can remember from them
> is this:
>
> http://oss.sgi.com/archives/xfs/2009-03/msg00224.html
>
> Maybe now that ext4 is hitting the problem something will be done
> about it...

OK.  I've CC'd Andrew and Hugh, so maybe we can get a discussion going.

josh

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3767 bytes --]

On Tue, 23 Aug 2011, Josh Boyer wrote:
> On Tue, Aug 23, 2011 at 9:04 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Tue, Aug 23, 2011 at 07:59:20AM -0400, Josh Boyer wrote:
> >> On Tue, Aug 23, 2011 at 7:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> >> >> >  Possible unsafe locking scenario:
> >> >> >
> >> >> >       CPU0                    CPU1
> >> >> >       ----                    ----
> >> >> >  lock(&mm->mmap_sem);
> >> >> >                               lock(&sb->s_type->i_mutex_key);
> >> >> >                               lock(&mm->mmap_sem);
> >> >> >  lock(&sb->s_type->i_mutex_key);
> >> >> >
> >> >> >  *** DEADLOCK ***
> >> >>
> >> >> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
> >> >> and we're hoping Ted (or someone else from the ext4 camp) can comment
> >> >> on why ext4_evict_inode is holding i_mutex.
> >> >
> >> > Actually, the problem has nothing to do with ext4. the problem is
> >> > that remove_vma() is holding the mmap_sem while calling fput(). The
> >> > correct locking order is i_mutex->mmap_sem, as documented in
> >> > mm/filemap.c:
> >> >
> >> >  *  ->i_mutex                   (generic_file_buffered_write)
> >> >  *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)
> >> >
> >> >
> >> > The way remove_vma() calls fput() also triggers lockdep reports in
> >> > XFS and it will do so with any filesystem that takes an inode
> >> > specific lock in it's evict() processing. IOWs, remove_vma() needs
> >> > fixing, not ext4....
> >>
> >> Er... ok.  So the remove_vma code hasn't changed since 2008.  We're
> >> only seeing this issue now because the debugging code has improved,
> >> or?
> >
> > The problem has been there since at least 2008.  Here's an early
> > XFS report from 2.6.24:
> >
> > http://oss.sgi.com/archives/xfs/2008-02/msg00931.html
> >
> > Here's an XFS report
> > to match the ext4 one in this thread from 2009:
> >
> > http://oss.sgi.com/archives/xfs/2009-03/msg00149.html
> >
> > You won't find reports much older than this - it only started to be
> > reported when lockdep support in XFS matured and it started to be
> > widely used....
> >
> >> At any rate, the proposed solution is to make remove_vma drop mmap_sem
> >> before calling fput, or make it not call fput, or?
> >
> > Ask the VM folk - this is the only response I can remember from them
> > is this:
> >
> > http://oss.sgi.com/archives/xfs/2009-03/msg00224.html
> >
> > Maybe now that ext4 is hitting the problem something will be done
> > about it...
> 
> OK.  I've CC'd Andrew and Hugh, so maybe we can get a discussion going.

My first reaction would be that this is quite simply a filesystem bug.
Apparently a long-standing bug in the XFS case, but one in which ext4
has just now (3.1-rc) joined it.

The mm/fs locking hierarchy has been that way forever: mm does not assume
that the fs will not take any inode-specific lock in its fput(), but yes,
it does expect fput() not to take the i_mutex.

In this new ext4 case, it appears to be just an issue on final eviction
of the inode, which I think makes actual deadlock (when writing to file
needs to fault in a page from mm) impossible - we wouldn't be evicting
it if there were still references.  Just needs some lockdep notation?

Dropping mmap_sem while doing fput() in munmap() doesn't sound appealing
to me: although we have converted a number of paths to drop mmap_sem in
strategic places, getting back to the (possibly changed) vma sequence
afterwards is tiresome (when the munmap covers multiple vmas), and
instinct says that munmap() might be a more difficult case to get
right than most.  Leave the fput()s to a workqueue instead?

Hugh

Re: 3.1.0-rc3 -- INFO: possible circular locking dependency detected

[Dave Chinner]

On Wed, Aug 24, 2011 at 03:31:14PM -0700, Hugh Dickins wrote:
> On Tue, 23 Aug 2011, Josh Boyer wrote:
> > On Tue, Aug 23, 2011 at 9:04 AM, Dave Chinner <david@fromorbit.com> wrote:
> > > On Tue, Aug 23, 2011 at 07:59:20AM -0400, Josh Boyer wrote:
> > >> On Tue, Aug 23, 2011 at 7:49 AM, Dave Chinner <david@fromorbit.com> wrote:
> > >> >> >  Possible unsafe locking scenario:
> > >> >> >
> > >> >> >       CPU0                    CPU1
> > >> >> >       ----                    ----
> > >> >> >  lock(&mm->mmap_sem);
> > >> >> >                               lock(&sb->s_type->i_mutex_key);
> > >> >> >                               lock(&mm->mmap_sem);
> > >> >> >  lock(&sb->s_type->i_mutex_key);
> > >> >> >
> > >> >> >  *** DEADLOCK ***
> > >> >>
> > >> >> This one was reported yesterday: https://lkml.org/lkml/2011/8/21/163
> > >> >> and we're hoping Ted (or someone else from the ext4 camp) can comment
> > >> >> on why ext4_evict_inode is holding i_mutex.
> > >> >
> > >> > Actually, the problem has nothing to do with ext4. the problem is
> > >> > that remove_vma() is holding the mmap_sem while calling fput(). The
> > >> > correct locking order is i_mutex->mmap_sem, as documented in
> > >> > mm/filemap.c:
> > >> >
> > >> >  *  ->i_mutex                   (generic_file_buffered_write)
> > >> >  *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)
> > >> >
> > >> >
> > >> > The way remove_vma() calls fput() also triggers lockdep reports in
> > >> > XFS and it will do so with any filesystem that takes an inode
> > >> > specific lock in it's evict() processing. IOWs, remove_vma() needs
> > >> > fixing, not ext4....
> > >>
> > >> Er... ok.  So the remove_vma code hasn't changed since 2008.  We're
> > >> only seeing this issue now because the debugging code has improved,
> > >> or?
> > >
> > > The problem has been there since at least 2008.  Here's an early
> > > XFS report from 2.6.24:
> > >
> > > http://oss.sgi.com/archives/xfs/2008-02/msg00931.html
> > >
> > > Here's an XFS report
> > > to match the ext4 one in this thread from 2009:
> > >
> > > http://oss.sgi.com/archives/xfs/2009-03/msg00149.html
> > >
> > > You won't find reports much older than this - it only started to be
> > > reported when lockdep support in XFS matured and it started to be
> > > widely used....
> > >
> > >> At any rate, the proposed solution is to make remove_vma drop mmap_sem
> > >> before calling fput, or make it not call fput, or?
> > >
> > > Ask the VM folk - this is the only response I can remember from them
> > > is this:
> > >
> > > http://oss.sgi.com/archives/xfs/2009-03/msg00224.html
> > >
> > > Maybe now that ext4 is hitting the problem something will be done
> > > about it...
> > 
> > OK.  I've CC'd Andrew and Hugh, so maybe we can get a discussion going.
> 
> My first reaction would be that this is quite simply a filesystem bug.
> Apparently a long-standing bug in the XFS case, but one in which ext4
> has just now (3.1-rc) joined it.
> 
> The mm/fs locking hierarchy has been that way forever:

That doesn't make it right - the XFS people have been complaining
about this mmap_sem/fput problem for as long as I can remember.

> mm does not assume
> that the fs will not take any inode-specific lock in its fput(), but yes,
> it does expect fput() not to take the i_mutex.

I think that is a broken assumption. Looking at teh documentation
for the file operations, Documentation/filesystems/vfs.txt has this
comment:

	Again, all methods are called without any locks being held, unless
	otherwise noted.

And from Documentation/filesystems/Locking:

	locking rules:
		All may block except for ->setlease.
		No VFS locks held on entry except for ->setlease.

So given that fput() can call both f_ops->async and f_ops->release,
which according to the above documentation should be called without
any locks held and. That implies that fput() should not be called
with locks held.

There is nothing to say any of the file operations methods cannot
take i_mutex or other inode locks. The implication that they are
called without locks held is clear - it is supposed to be safe for
filesystems to take locks in these file operation methods.

And from mm/filemap.c where lock orders are defined:

 *  ->i_mutex                   (generic_file_buffered_write)
 *    ->mmap_sem                (fault_in_pages_readable->do_page_fault)

It's pretty clear that calling fput() with the mmap_sem held
violates this order....

Similarly, fput() calls dput(), which is how we're ending up in
.evict

> 
> In this new ext4 case, it appears to be just an issue on final eviction
> of the inode, which I think makes actual deadlock (when writing to file
> needs to fault in a page from mm) impossible - we wouldn't be evicting
> it if there were still references.  Just needs some lockdep notation?

For evict, that's necessary for the shrinkers not to throw lockdep
woarnings.

But that's not the only case that the mmap_sem/fput inversion can
trigger: .release is another. We've had to place try lock semantics
in the XFS code to avoid deadlocks there because inodes are not
being reclaimed when .release is called....

> Dropping mmap_sem while doing fput() in munmap() doesn't sound appealing
> to me: although we have converted a number of paths to drop mmap_sem in
> strategic places, getting back to the (possibly changed) vma sequence
> afterwards is tiresome (when the munmap covers multiple vmas), and
> instinct says that munmap() might be a more difficult case to get
> right than most.  Leave the fput()s to a workqueue instead?

Some kind of deferred processing is needed, I guess. I don't know
what is the best way to do it. perhaps an operation that will drop
all but the last filp reference in line, but push the last one to a
workqueue. eg. fput_background()?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com