Recursive ->i_mutex lockdep complaint

Recursive ->i_mutex lockdep complaint

[Alexey Dobriyan]

On Mon, Mar 26, 2007 at 11:35:42PM -0800, akpm@linux-foundation.org wrote:
> The patch titled
>      add file position info to proc
> has been added to the -mm tree.  Its filename is
>      add-file-position-info-to-proc.patch

I tried to stress-test it with the following program and script and
lockdep barfs on me reasonably quickly:
--------------------------
#include <sys/types.h>
#include <unistd.h>
#include <stdio.h>

int main(void)
{
	int fd[2];

	printf("%u\n", getpid());
	while (1) {
		pipe(fd);
		close(fd[0]);
		close(fd[1]);
	}
	return 0;
}
----------------------------
#!/bin/sh
while true; do find /proc -type f 2>/dev/null   | \
grep -v '/proc/bus/pci'                         | \
xargs cat >/dev/null 2>/dev/null; done
----------------------------

=============================================
[ INFO: possible recursive locking detected ]
2.6.21-rc5-mm4 #1
---------------------------------------------
find/15348 is trying to acquire lock:
 (&inode->i_mutex){--..}, at: [<c016656e>] pipe_read_fasync+0x22/0x53

but task is already holding lock:
 (&inode->i_mutex){--..}, at: [<c016bdd2>] vfs_readdir+0x41/0x85

other info that might help us debug this:
1 lock held by find/15348:
 #0:  (&inode->i_mutex){--..}, at: [<c016bdd2>] vfs_readdir+0x41/0x85

stack backtrace:
 [<c0131b62>] __lock_acquire+0xbc1/0x1021
 [<c012ec07>] lockdep_init_map+0x31/0x45e
 [<c013202a>] lock_acquire+0x68/0x82
 [<c016656e>] pipe_read_fasync+0x22/0x53
 [<c01ca2d0>] _atomic_dec_and_lock+0x10/0x50
 [<c02e0ef3>] __mutex_lock_slowpath+0x6a/0x2c1
 [<c016656e>] pipe_read_fasync+0x22/0x53
 [<c015d33a>] kmem_cache_free+0xa2/0xd8
 [<c016656e>] pipe_read_fasync+0x22/0x53
 [<c01668f7>] pipe_read_release+0x12/0x24
 [<c0161ee1>] __fput+0x4e/0x12f
 [<c015fa14>] filp_close+0x3e/0x62
 [<c0118d33>] put_files_struct+0xb2/0xe0
 [<c01ce901>] snprintf+0x1f/0x23
 [<c018e812>] proc_readfd_common+0x173/0x286
 [<c018f57c>] proc_fdinfo_instantiate+0x0/0x64
 [<c02e0fe0>] __mutex_lock_slowpath+0x157/0x2c1
 [<c016bbe4>] filldir64+0x0/0xf2
 [<c016bbe4>] filldir64+0x0/0xf2
 [<c018e934>] proc_readfdinfo+0xf/0x13
 [<c018f57c>] proc_fdinfo_instantiate+0x0/0x64
 [<c016bbe4>] filldir64+0x0/0xf2
 [<c016be01>] vfs_readdir+0x70/0x85
 [<c016be7c>] sys_getdents64+0x66/0xa9
 [<c0130a8a>] trace_hardirqs_on+0xbe/0x15d
 [<c0103eaa>] sysenter_past_esp+0x5f/0x99
 =======================


It seems that lockdep is unhappy about ->i_mutex taken in
->release/pipe_read_release()/pipe_read_fasync()
which is triggered from put_files_struct() in proc_readfd_common()

Now checking if giving pipe's i_mutex its own lockdep class with fix
things.

Re: Recursive ->i_mutex lockdep complaint

[Miklos Szeredi]

On Tue, 2007-04-03 at 18:21 +0400, Alexey Dobriyan wrote:
> On Mon, Mar 26, 2007 at 11:35:42PM -0800, akpm@linux-foundation.org wrote:
> > The patch titled
> >      add file position info to proc
> > has been added to the -mm tree.  Its filename is
> >      add-file-position-info-to-proc.patch
> 
> I tried to stress-test it with the following program and script and
> lockdep barfs on me reasonably quickly:

Ugh.  As I see it, this is independent from the above patch, as the same
can happen with /proc/PID/fd.  Or did I miss something?

And it seems quite benign, one of the mutexes is for the proc inode, the
other is for the pipe inode, and hopefully they haven't got anything
else to do with each other.

> It seems that lockdep is unhappy about ->i_mutex taken in
> ->release/pipe_read_release()/pipe_read_fasync()
> which is triggered from put_files_struct() in proc_readfd_common()
> 
> Now checking if giving pipe's i_mutex its own lockdep class with fix
> things.

Should help.  Although I'm wondering if it's worth bothering with, as it
doesn't seem to happen in real life for lockdep users.

Thanks,
Miklos

Re: Recursive ->i_mutex lockdep complaint

[Alexey Dobriyan]

On Tue, Apr 03, 2007 at 05:05:20PM +0200, Miklos Szeredi wrote:
> On Tue, 2007-04-03 at 18:21 +0400, Alexey Dobriyan wrote:
> > On Mon, Mar 26, 2007 at 11:35:42PM -0800, akpm@linux-foundation.org wrote:
> > > The patch titled
> > >      add file position info to proc
> > > has been added to the -mm tree.  Its filename is
> > >      add-file-position-info-to-proc.patch
> >
> > I tried to stress-test it with the following program and script and
> > lockdep barfs on me reasonably quickly:
>
> Ugh.  As I see it, this is independent from the above patch, as the same
> can happen with /proc/PID/fd.  Or did I miss something?

Yeah, it's independent from /proc/*/fdinfo stuff.

> And it seems quite benign, one of the mutexes is for the proc inode, the
> other is for the pipe inode, and hopefully they haven't got anything
> else to do with each other.
>
> > It seems that lockdep is unhappy about ->i_mutex taken in
> > ->release/pipe_read_release()/pipe_read_fasync()
> > which is triggered from put_files_struct() in proc_readfd_common()
> >
> > Now checking if giving pipe's i_mutex its own lockdep class with fix
> > things.
>
> Should help.  Although I'm wondering if it's worth bothering with, as it
> doesn't seem to happen in real life for lockdep users.

The following patch makes lockdep happy but I wonder if it's correct
way.

--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -908,6 +908,8 @@ static struct dentry_operations pipefs_d
 	.d_dname	= pipefs_dname,
 };
 
+static struct lock_class_key pipe_inode_imutex_key;
+
 static struct inode * get_pipe_inode(void)
 {
 	struct inode *inode = new_inode(pipe_mnt->mnt_sb);
@@ -936,6 +938,8 @@ static struct inode * get_pipe_inode(voi
 	inode->i_gid = current->fsgid;
 	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 
+	lockdep_set_class(&inode->i_mutex, &pipe_inode_imutex_key);
+
 	return inode;
 
 fail_iput: