lockdep wierdness...

lockdep wierdness...

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

I'm seeing lockdep warning about a potential lock inversion between
&mm->mmap_sem and &inode->i_mutex in NFS (see attachment).

Unfortunately the basis for the warning appears to be the behaviour in
ext3(???). AFAICS there is no way for NFS to share an inode->i_mutex
with ext3. What to do?

Trond

[-- Attachment #2: Type: text/plain, Size: 2363 bytes --]

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.23-rc7-g8809e921 #1
-------------------------------------------------------
beagle-build-in/24375 is trying to acquire lock:
 (&mm->mmap_sem){----}, at: [<c05a2887>] do_page_fault+0x17d/0x591

but task is already holding lock:
 (&inode->i_mutex){--..}, at: [<c059f9e3>] mutex_lock+0x1c/0x1f

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&inode->i_mutex){--..}:
       [<c043d4da>] __lock_acquire+0x9f3/0xba6
       [<c043da62>] lock_acquire+0x5f/0x78
       [<c059f832>] __mutex_lock_slowpath+0xe5/0x27a
       [<c059f9e3>] mutex_lock+0x1c/0x1f
       [<f8c92495>] nfs_revalidate_mapping+0x64/0x9c [nfs]
       [<f8c8ff2a>] nfs_file_mmap+0x46/0x75 [nfs]
       [<c046097c>] mmap_region+0x1ea/0x3b8
       [<c0460e9b>] do_mmap_pgoff+0x27b/0x2da
       [<c0407d77>] sys_mmap2+0x9b/0xb5
       [<c040405e>] sysenter_past_esp+0x5f/0x99
       [<ffffffff>] 0xffffffff

-> #0 (&mm->mmap_sem){----}:
       [<c043d3c6>] __lock_acquire+0x8df/0xba6
       [<c043da62>] lock_acquire+0x5f/0x78
       [<c04360db>] down_read+0x3a/0x4c
       [<c05a2887>] do_page_fault+0x17d/0x591
       [<c05a1382>] error_code+0x72/0x78
       [<f88acaac>] call_filldir+0xac/0xc3 [ext3]
       [<f88acdb2>] ext3_readdir+0x217/0x5e5 [ext3]
       [<c04798a1>] vfs_readdir+0x67/0x93
       [<c0479af6>] sys_getdents+0x5f/0x9d
       [<c040405e>] sysenter_past_esp+0x5f/0x99
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

1 lock held by beagle-build-in/24375:
 #0:  (&inode->i_mutex){--..}, at: [<c059f9e3>] mutex_lock+0x1c/0x1f

stack backtrace:
 [<c04050ee>] show_trace_log_lvl+0x1a/0x2f
 [<c0405b58>] show_trace+0x12/0x14
 [<c0405b70>] dump_stack+0x16/0x18
 [<c043bc05>] print_circular_bug_tail+0x5f/0x68
 [<c043d3c6>] __lock_acquire+0x8df/0xba6
 [<c043da62>] lock_acquire+0x5f/0x78
 [<c04360db>] down_read+0x3a/0x4c
 [<c05a2887>] do_page_fault+0x17d/0x591
 [<c05a1382>] error_code+0x72/0x78
 [<f88acaac>] call_filldir+0xac/0xc3 [ext3]
 [<f88acdb2>] ext3_readdir+0x217/0x5e5 [ext3]
 [<c04798a1>] vfs_readdir+0x67/0x93
 [<c0479af6>] sys_getdents+0x5f/0x9d
 [<c040405e>] sysenter_past_esp+0x5f/0x99
 =======================

Re: lockdep wierdness...

[Steven Rostedt]

On Mon, Sep 24, 2007 at 06:07:38PM -0400, Trond Myklebust wrote:
> I'm seeing lockdep warning about a potential lock inversion between
> &mm->mmap_sem and &inode->i_mutex in NFS (see attachment).
> 
> Unfortunately the basis for the warning appears to be the behaviour in
> ext3(???). AFAICS there is no way for NFS to share an inode->i_mutex
> with ext3. What to do?

Actually this can probably happen just on NFS alone.

> 
> Trond

> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.23-rc7-g8809e921 #1
> -------------------------------------------------------
> beagle-build-in/24375 is trying to acquire lock:
>  (&mm->mmap_sem){----}, at: [<c05a2887>] do_page_fault+0x17d/0x591
> 
> but task is already holding lock:
>  (&inode->i_mutex){--..}, at: [<c059f9e3>] mutex_lock+0x1c/0x1f
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #1 (&inode->i_mutex){--..}:
>        [<c043d4da>] __lock_acquire+0x9f3/0xba6
>        [<c043da62>] lock_acquire+0x5f/0x78
>        [<c059f832>] __mutex_lock_slowpath+0xe5/0x27a
>        [<c059f9e3>] mutex_lock+0x1c/0x1f
>        [<f8c92495>] nfs_revalidate_mapping+0x64/0x9c [nfs]
>        [<f8c8ff2a>] nfs_file_mmap+0x46/0x75 [nfs]
>        [<c046097c>] mmap_region+0x1ea/0x3b8
>        [<c0460e9b>] do_mmap_pgoff+0x27b/0x2da
>        [<c0407d77>] sys_mmap2+0x9b/0xb5
>        [<c040405e>] sysenter_past_esp+0x5f/0x99
>        [<ffffffff>] 0xffffffff
> 
> -> #0 (&mm->mmap_sem){----}:
>        [<c043d3c6>] __lock_acquire+0x8df/0xba6
>        [<c043da62>] lock_acquire+0x5f/0x78
>        [<c04360db>] down_read+0x3a/0x4c
>        [<c05a2887>] do_page_fault+0x17d/0x591
>        [<c05a1382>] error_code+0x72/0x78
>        [<f88acaac>] call_filldir+0xac/0xc3 [ext3]
>        [<f88acdb2>] ext3_readdir+0x217/0x5e5 [ext3]
>        [<c04798a1>] vfs_readdir+0x67/0x93
>        [<c0479af6>] sys_getdents+0x5f/0x9d
>        [<c040405e>] sysenter_past_esp+0x5f/0x99
>        [<ffffffff>] 0xffffffff

The circular lock seems to be this:

#1:

  sys_mmap2:              down_write(&mm->mmap_sem);
  nfs_revalidate_mapping: mutex_lock(&inode->i_mutex);


#0:

  vfs_readdir:     mutex_lock(&inode->i_mutex);
   - during the readdir (filldir64), we take a user fault (missing page?)
    and call do_page_fault -
  do_page_fault:   down_read(&mm->mmap_sem);


So it does indeed look like a circular locking. Now the question is, "is
this a bug?".  Looking like the inode of #1 must be a file or something
else that you can mmap and the inode of #0 seems it must be a directory.
I would say "no".

Now if you can readdir on a file or mmap a directory, then this could be
an issue.

Otherwise, I'd love to see someone teach lockdep about this issue! ;-)

-- Steve

Re: lockdep wierdness...

[Peter Zijlstra]

On Mon, 2007-09-24 at 22:13 -0400, Steven Rostedt wrote:
> On Mon, Sep 24, 2007 at 06:07:38PM -0400, Trond Myklebust wrote:
> > I'm seeing lockdep warning about a potential lock inversion between
> > &mm->mmap_sem and &inode->i_mutex in NFS (see attachment).
> > 
> > Unfortunately the basis for the warning appears to be the behaviour in
> > ext3(???). AFAICS there is no way for NFS to share an inode->i_mutex
> > with ext3. What to do?
> 
> Actually this can probably happen just on NFS alone.
> 
> > 
> > Trond
> 
> > =======================================================
> > [ INFO: possible circular locking dependency detected ]
> > 2.6.23-rc7-g8809e921 #1
> > -------------------------------------------------------
> > beagle-build-in/24375 is trying to acquire lock:
> >  (&mm->mmap_sem){----}, at: [<c05a2887>] do_page_fault+0x17d/0x591
> > 
> > but task is already holding lock:
> >  (&inode->i_mutex){--..}, at: [<c059f9e3>] mutex_lock+0x1c/0x1f
> > 
> > which lock already depends on the new lock.
> > 
> > 
> > the existing dependency chain (in reverse order) is:
> > 
> > -> #1 (&inode->i_mutex){--..}:
> >        [<c043d4da>] __lock_acquire+0x9f3/0xba6
> >        [<c043da62>] lock_acquire+0x5f/0x78
> >        [<c059f832>] __mutex_lock_slowpath+0xe5/0x27a
> >        [<c059f9e3>] mutex_lock+0x1c/0x1f
> >        [<f8c92495>] nfs_revalidate_mapping+0x64/0x9c [nfs]
> >        [<f8c8ff2a>] nfs_file_mmap+0x46/0x75 [nfs]
> >        [<c046097c>] mmap_region+0x1ea/0x3b8
> >        [<c0460e9b>] do_mmap_pgoff+0x27b/0x2da
> >        [<c0407d77>] sys_mmap2+0x9b/0xb5
> >        [<c040405e>] sysenter_past_esp+0x5f/0x99
> >        [<ffffffff>] 0xffffffff
> > 
> > -> #0 (&mm->mmap_sem){----}:
> >        [<c043d3c6>] __lock_acquire+0x8df/0xba6
> >        [<c043da62>] lock_acquire+0x5f/0x78
> >        [<c04360db>] down_read+0x3a/0x4c
> >        [<c05a2887>] do_page_fault+0x17d/0x591
> >        [<c05a1382>] error_code+0x72/0x78
> >        [<f88acaac>] call_filldir+0xac/0xc3 [ext3]
> >        [<f88acdb2>] ext3_readdir+0x217/0x5e5 [ext3]
> >        [<c04798a1>] vfs_readdir+0x67/0x93
> >        [<c0479af6>] sys_getdents+0x5f/0x9d
> >        [<c040405e>] sysenter_past_esp+0x5f/0x99
> >        [<ffffffff>] 0xffffffff
> 
> The circular lock seems to be this:
> 
> #1:
> 
>   sys_mmap2:              down_write(&mm->mmap_sem);
>   nfs_revalidate_mapping: mutex_lock(&inode->i_mutex);
> 
> 
> #0:
> 
>   vfs_readdir:     mutex_lock(&inode->i_mutex);
>    - during the readdir (filldir64), we take a user fault (missing page?)
>     and call do_page_fault -
>   do_page_fault:   down_read(&mm->mmap_sem);
> 
> 
> So it does indeed look like a circular locking. Now the question is, "is
> this a bug?".  Looking like the inode of #1 must be a file or something
> else that you can mmap and the inode of #0 seems it must be a directory.
> I would say "no".
> 
> Now if you can readdir on a file or mmap a directory, then this could be
> an issue.
> 
> Otherwise, I'd love to see someone teach lockdep about this issue! ;-)

Christoph,

does Steve's story make sense? 

If so, do we know at alloc_inode() time what type of inode we're
requesting; file or dir. Again, if so, the lockdep annotation should be
trivial in the light of the recently merged patch:

 + lockdep-give-each-filesystem-its-own-inode-lock-class.patch

All that would need to be done is add an extra lock_class_key to
file_system_type for i_mutex_dir_key, and extend alloc_inode to say
something like:

  if (dir)
    lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_dir_key);
  else
    lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);

Re: lockdep wierdness...

[Christoph Hellwig]

On Thu, Sep 27, 2007 at 03:51:07PM +0200, Peter Zijlstra wrote:
> Christoph,
> 
> does Steve's story make sense? 

Yes.

> All that would need to be done is add an extra lock_class_key to
> file_system_type for i_mutex_dir_key, and extend alloc_inode to say
> something like:
> 
>   if (dir)
>     lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_dir_key);
>   else
>     lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);

Unfortunately we don't know what type of inode we have when calling
alloc_inode.  We only know it after reading in the inode from disk,
aka in unlock_new_inode.  Then again there is no reason to use
i_mutex before unlock_new_inode returns, so maybe we could defer
initializing it until unlock_new_inode.  I'm pretty sure we'll have
to fix a few filesystems that take i_mutex before that despite not
needing it, e.g. through i_size_write, though.

Re: lockdep wierdness...

[Peter Zijlstra]

On Thu, 2007-09-27 at 15:00 +0100, Christoph Hellwig wrote:
> On Thu, Sep 27, 2007 at 03:51:07PM +0200, Peter Zijlstra wrote:
> > Christoph,
> > 
> > does Steve's story make sense? 
> 
> Yes.
> 
> > All that would need to be done is add an extra lock_class_key to
> > file_system_type for i_mutex_dir_key, and extend alloc_inode to say
> > something like:
> > 
> >   if (dir)
> >     lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_dir_key);
> >   else
> >     lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key);
> 
> Unfortunately we don't know what type of inode we have when calling
> alloc_inode.  We only know it after reading in the inode from disk,
> aka in unlock_new_inode.  Then again there is no reason to use
> i_mutex before unlock_new_inode returns, so maybe we could defer
> initializing it until unlock_new_inode.  I'm pretty sure we'll have
> to fix a few filesystems that take i_mutex before that despite not
> needing it, e.g. through i_size_write, though.

How about this:

---
Make a distinction between file and dir usage of i_mutex.

The inode should be complete and unused at unlock_new_inode(), re-init
i_mutex depending on its type.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 fs/inode.c         |   12 ++++++++++++
 include/linux/fs.h |    1 +
 2 files changed, 13 insertions(+)

Index: linux-2.6/fs/inode.c
===================================================================
--- linux-2.6.orig/fs/inode.c
+++ linux-2.6/fs/inode.c
@@ -576,6 +576,18 @@ EXPORT_SYMBOL(new_inode);
 
 void unlock_new_inode(struct inode *inode)
 {
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct file_system_type *type = inode->i_sb->s_type;
+	/*
+	 * ensure nobody is actually holding i_mutex
+	 */
+	mutex_destroy(&inode->i_mutex);
+	mutex_init(&inode->i_mutex);
+	if (inode->i_mode & S_IFDIR)
+		lockdep_set_class(&inode->i_mutex, &type->i_mutex_dir_key);
+	else
+		lockdep_set_class(&inode->i_mutex, &type->i_mutex_key);
+#endif
 	/*
 	 * This is special!  We do not need the spinlock
 	 * when clearing I_LOCK, because we're guaranteed
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h
+++ linux-2.6/include/linux/fs.h
@@ -1426,6 +1426,7 @@ struct file_system_type {
 
 	struct lock_class_key i_lock_key;
 	struct lock_class_key i_mutex_key;
+	struct lock_class_key i_mutex_dir_key;
 	struct lock_class_key i_alloc_sem_key;
 };