[RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Louis Rilling]

Hi all,

The following patches fix lockdep warnings resulting from (correct) recursive
locking in configfs.

Current lockdep annotations for inode mutexes in configfs are lockdep-friendly
provided that:
1/ config_groups have at most one level of default groups (see
   configfs_attach_group()),
2/ config_groups having default groups are never removed (see
   configfs_detach_prep()).

Since lockdep does not handle such correct recursion, the idea is to insert
lockdep_off()/lockdep_on() for inode mutexes as soon as the level of recursion
of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency pattern increases.

The patches apply to latest configfs in linux-2.6.git (
commit 8033c6e9736c29cce5f0d0abbca9a44dffb20c39 for instance ), and were
successfully tested.

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[RFC][PATCH 1/3] configfs: set CONFIGFS_USET_DEFAULT earlier in configfs_attach_group()

[Louis Rilling]

[-- Attachment #1: configfs-tell-configfs_attach_group-if-default-group.patch --]
[-- Type: text/plain, Size: 3153 bytes --]

When creating a config_group, the CONFIGFS_USET_DEFAULT flag of default
sub-groups is only known after create_default_group() finishes its work, that is
after having creating the whole sub-hierarchy. In order to track which lock to
hide from lockdep, we need to known whether a config_group is default earlier,
that is before creating the sub-hierarchy.

This patch adds a def_group flag to configfs_attach_group(), which allows
configfs_attach_group to set the CONFIGFS_USET_DEFAULT flag before calling
populate_groups().

Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com>
---
 fs/configfs/dir.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)


Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c	2008-05-20 17:13:18.000000000 +0200
+++ b/fs/configfs/dir.c	2008-05-20 18:19:35.000000000 +0200
@@ -445,7 +445,8 @@ static int populate_attrs(struct config_
 
 static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
-				 struct dentry *dentry);
+				 struct dentry *dentry,
+				 int def_group);
 static void configfs_detach_group(struct config_item *item);
 
 static void detach_groups(struct config_group *group)
@@ -500,7 +501,6 @@ static int create_default_group(struct c
 {
 	int ret;
 	struct qstr name;
-	struct configfs_dirent *sd;
 	/* We trust the caller holds a reference to parent */
 	struct dentry *child, *parent = parent_group->cg_item.ci_dentry;
 
@@ -516,11 +516,9 @@ static int create_default_group(struct c
 		d_add(child, NULL);
 
 		ret = configfs_attach_group(&parent_group->cg_item,
-					    &group->cg_item, child);
-		if (!ret) {
-			sd = child->d_fsdata;
-			sd->s_type |= CONFIGFS_USET_DEFAULT;
-		} else {
+					    &group->cg_item, child,
+					    1);
+		if (ret) {
 			d_delete(child);
 			dput(child);
 		}
@@ -692,7 +690,8 @@ static void configfs_detach_item(struct 
 
 static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
-				 struct dentry *dentry)
+				 struct dentry *dentry,
+				 int def_group)
 {
 	int ret;
 	struct configfs_dirent *sd;
@@ -701,6 +700,8 @@ static int configfs_attach_group(struct 
 	if (!ret) {
 		sd = dentry->d_fsdata;
 		sd->s_type |= CONFIGFS_USET_DIR;
+		if (def_group)
+			sd->s_type |= CONFIGFS_USET_DEFAULT;
 
 		ret = populate_groups(to_config_group(item));
 		if (ret) {
@@ -1094,7 +1095,7 @@ static int configfs_mkdir(struct inode *
 	module_got = 1;
 
 	if (group)
-		ret = configfs_attach_group(parent_item, item, dentry);
+		ret = configfs_attach_group(parent_item, item, dentry, 0);
 	else
 		ret = configfs_attach_item(parent_item, item, dentry);
 
@@ -1418,7 +1419,8 @@ int configfs_register_subsystem(struct c
 		d_add(dentry, NULL);
 
 		err = configfs_attach_group(sd->s_element, &group->cg_item,
-					    dentry);
+					    dentry,
+					    0);
 		if (err) {
 			d_delete(dentry);
 			dput(dentry);

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[RFC][PATCH 2/3] configfs: Silence lockdep when creating nested default groups

[Louis Rilling]

[-- Attachment #1: configfs-silence-lockdep-with-default-group-creation.patch --]
[-- Type: text/plain, Size: 2420 bytes --]

When default groups are nested, lockdep raises a warning since it sees a
lock recursion of class I_MUTEX_CHILD in populate_groups().
However, this lock recursion is just a variant of the I_MUTEX_PARENT ->
I_MUTEX_CHILD dependency, which is ok in the VFS, and is already checked when
creating the first level of default groups.

This patch silences lockdep with nested default groups, by hiding the mutex
locks of populate_groups() from lockdep when the considered config_group is
itself a default group. The mutex is not hidden for a non-default group, so that
the lock dependency remains checked, in case things change in some future.

Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com>
---
 fs/configfs/dir.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)


Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c	2008-05-20 18:19:35.000000000 +0200
+++ b/fs/configfs/dir.c	2008-05-20 18:19:41.000000000 +0200
@@ -535,6 +535,8 @@ static int populate_groups(struct config
 	int i;
 
 	if (group->default_groups) {
+		struct configfs_dirent *sd = dentry->d_fsdata;
+		int turn_lockdep_off = (sd->s_type & CONFIGFS_USET_DEFAULT);
 		/*
 		 * FYI, we're faking mkdir here
 		 * I'm not sure we need this semaphore, as we're called
@@ -544,7 +546,18 @@ static int populate_groups(struct config
 		 * That said, taking our i_mutex is closer to mkdir
 		 * emulation, and shouldn't hurt.
 		 */
+		/* For a default group, we hide this mutex from lockdep since:
+		 * 1/ This is a case of I_MUTEX_PARENT -> I_MUTEX_CHILD
+		 *    dependency;
+		 * 2/ This dependency was already checked when creating the
+		 *    parent of this group;
+		 * 3/ Lockdep does not handle such safe recursion.
+		 */
+		if (turn_lockdep_off)
+			lockdep_off();
 		mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD);
+		if (turn_lockdep_off)
+			lockdep_on();
 
 		for (i = 0; group->default_groups[i]; i++) {
 			new_group = group->default_groups[i];
@@ -554,7 +567,11 @@ static int populate_groups(struct config
 				break;
 		}
 
+		if (turn_lockdep_off)
+			lockdep_off();
 		mutex_unlock(&dentry->d_inode->i_mutex);
+		if (turn_lockdep_off)
+			lockdep_on();
 	}
 
 	if (ret)

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

[RFC][PATCH 3/3] configfs: Silence lockdep when destroying default groups

[Louis Rilling]

[-- Attachment #1: configfs-silence-lockdep-with-default-group-destruction.patch --]
[-- Type: text/plain, Size: 3852 bytes --]

When destroying a config_group having default groups, lockdep raises a warning
because multiple locks of class I_MUTEX_NORMAL are taken in
configfs_detach_prep() (the first one being in vfs_rmdir()). However this
recursive locking is another instance of I_MUTEX_PARENT -> I_MUTEX_CHILD
dependency, which was already checked correct when creating the config_group,
and which lockdep cannot handle cleanly because of the variable depth.

This patch silences lockdep when destroying default groups by simply hiding the
inode mutex locks from lockdep in configfs_detach_prep(),
configfs_detach_rollback(), and detach_groups().

=============================================
[ INFO: possible recursive locking detected ]
2.6.26-rc3 #5
---------------------------------------------
rmdir/1499 is trying to acquire lock:
 (&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff802d2529>] configfs_detach_prep+0x54/0x89

but task is already holding lock:
 (&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff80296513>] vfs_rmdir+0x49/0xac

other info that might help us debug this:
2 locks held by rmdir/1499:
 #0:  (&sb->s_type->i_mutex_key#3/1){--..}, at: [<ffffffff80298138>] do_rmdir+0x82/0x108
 #1:  (&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff80296513>] vfs_rmdir+0x49/0xac

stack backtrace:
Pid: 1499, comm: rmdir Not tainted 2.6.26-rc3 #5

Call Trace:
 [<ffffffff8024afe9>] __lock_acquire+0x8d2/0xc78
 [<ffffffff80266399>] filemap_fault+0x1cf/0x332
 [<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
 [<ffffffff8024b762>] lock_acquire+0x51/0x6c
 [<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
 [<ffffffff80248331>] debug_mutex_lock_common+0x16/0x23
 [<ffffffff805d6244>] mutex_lock_nested+0xcd/0x23b
 [<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
 [<ffffffff802d3656>] configfs_rmdir+0xb8/0x1c3
 [<ffffffff80296535>] vfs_rmdir+0x6b/0xac
 [<ffffffff8029816d>] do_rmdir+0xb7/0x108
 [<ffffffff8024a2a2>] trace_hardirqs_on+0xef/0x113
 [<ffffffff805d7364>] trace_hardirqs_on_thunk+0x35/0x3a
 [<ffffffff8020b0cb>] system_call_after_swapgs+0x7b/0x80


Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com>
---
 fs/configfs/dir.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)


Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c	2008-05-20 18:19:41.000000000 +0200
+++ b/fs/configfs/dir.c	2008-05-20 18:19:45.000000000 +0200
@@ -352,7 +352,12 @@ static int configfs_detach_prep(struct d
 		if (sd->s_type & CONFIGFS_NOT_PINNED)
 			continue;
 		if (sd->s_type & CONFIGFS_USET_DEFAULT) {
+			/* Hide this mutex from lockdep for the same reasons as in
+			 * populate_groups()
+			 */
+			lockdep_off();
 			mutex_lock(&sd->s_dentry->d_inode->i_mutex);
+			lockdep_on();
 			/* Mark that we've taken i_mutex */
 			sd->s_type |= CONFIGFS_USET_DROPPING;
 
@@ -388,7 +393,12 @@ static void configfs_detach_rollback(str
 
 			if (sd->s_type & CONFIGFS_USET_DROPPING) {
 				sd->s_type &= ~CONFIGFS_USET_DROPPING;
+				/* This mutex was hidden from lockdep in
+				 * configfs_detach_prep()
+				 */
+				lockdep_off();
 				mutex_unlock(&sd->s_dentry->d_inode->i_mutex);
+				lockdep_on();
 			}
 		}
 	}
@@ -475,8 +485,14 @@ static void detach_groups(struct config_
 		 * has taken our i_mutex for us.  Drop it.
 		 * From mkdir/register cleanup, there is no sem held.
 		 */
-		if (sd->s_type & CONFIGFS_USET_DROPPING)
+		if (sd->s_type & CONFIGFS_USET_DROPPING) {
+			/* This mutex was hidden from lockdep in
+			 * configfs_detach_prep()
+			 */
+			lockdep_off();
 			mutex_unlock(&child->d_inode->i_mutex);
+			lockdep_on();
+		}
 
 		d_delete(child);
 		dput(child);

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Arjan van de Ven]

On Tue, 20 May 2008 18:33:20 +0200
Louis Rilling <Louis.Rilling@kerlabs.com> wrote:

> Hi all,
> 
> The following patches fix lockdep warnings resulting from (correct)
> recursive locking in configfs.
> 
> Current lockdep annotations for inode mutexes in configfs are
> lockdep-friendly provided that:
> 1/ config_groups have at most one level of default groups (see
>    configfs_attach_group()),
> 2/ config_groups having default groups are never removed (see
>    configfs_detach_prep()).
> 
> Since lockdep does not handle such correct recursion, the idea is to
> insert lockdep_off()/lockdep_on() for inode mutexes as soon as the
> level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency
> pattern increases.

I'm... not entirely happy with such a solution ;(

there must be a better one.

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Louis Rilling]

Arjan van de Ven a écrit :
> On Tue, 20 May 2008 18:33:20 +0200
> Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
> 
>> Hi all,
>>
>> The following patches fix lockdep warnings resulting from (correct)
>> recursive locking in configfs.
>>
>> Current lockdep annotations for inode mutexes in configfs are
>> lockdep-friendly provided that:
>> 1/ config_groups have at most one level of default groups (see
>>    configfs_attach_group()),
>> 2/ config_groups having default groups are never removed (see
>>    configfs_detach_prep()).
>>
>> Since lockdep does not handle such correct recursion, the idea is to
>> insert lockdep_off()/lockdep_on() for inode mutexes as soon as the
>> level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency
>> pattern increases.
> 
> I'm... not entirely happy with such a solution ;(
> 
> there must be a better one.

Hmm, to me there are three solutions:

1/ keep lockdep and configfs like they are, and use this patchset

2/ enhance lockdep to handle wariable-depth but correct recursion:
seems uncertain...


3/ remove this recursive locking from configfs:
unfortunately, it seems that there are a good reasons for doing
recursive inode locking, at least when removing a config_group with
default groups. So, seems uncertain too...

Other ideas?

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Re: [Ocfs2-devel] [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Joel Becker]

On Tue, May 20, 2008 at 06:33:20PM +0200, Louis Rilling wrote:
> The following patches fix lockdep warnings resulting from (correct) recursive
> locking in configfs.
> 
> Current lockdep annotations for inode mutexes in configfs are lockdep-friendly
> provided that:
> 1/ config_groups have at most one level of default groups (see
>    configfs_attach_group()),
> 2/ config_groups having default groups are never removed (see
>    configfs_detach_prep()).
> 
> Since lockdep does not handle such correct recursion, the idea is to insert
> lockdep_off()/lockdep_on() for inode mutexes as soon as the level of recursion
> of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency pattern increases.

	Hmm, this is definitely a more readable solution than the
previous, but I'm also with Arjan that it's scary :-)

Joel

-- 

"Ninety feet between bases is perhaps as close as man has ever come
 to perfection."
	- Red Smith

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Joel Becker]

On Tue, May 20, 2008 at 09:58:10AM -0700, Arjan van de Ven wrote:
> On Tue, 20 May 2008 18:33:20 +0200
> Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
> 
> > The following patches fix lockdep warnings resulting from (correct)
> > recursive locking in configfs.
> >
> > ...
> >
> > Since lockdep does not handle such correct recursion, the idea is to
> > insert lockdep_off()/lockdep_on() for inode mutexes as soon as the
> > level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency
> > pattern increases.
> 
> I'm... not entirely happy with such a solution ;(
> 
> there must be a better one.

	We're trying to find it.  I really appreciate Louis taking the
time to approach the issue.  His first pass was to add 1 to MUTEX_CHILD
for each level of recursion.  This has a very tight limit (4 or 5
levels), but probably covers all users that exist and perhaps all that
ever will exist.  However, it means passing the lockdep annotation level
throughout the entire call chain across multiple files.  It was
definitely less readable.
	This approach takes a different tack - it's very readable, but
it assumes that the currently correct locking will always remain so - a
particular invariant that lockdep exists to verify :-)
	Louis, what about sticking the recursion level on
configfs_dirent?  That is, you could add sd->s_level and then use it
when needed.  THis would hopefully avoid having to pass the level as an
argument to every function.  Then we can go back to your original
scheme.  If they recurse too much and hit the lockdep limit, just rewind
everything and return -ELOOP.

Joel

-- 

Dort wo man Bücher verbrennt, verbrennt man am Ende auch Mensch.
(Wherever they burn books, they will also end up burning people.)
	- Heinrich Heine

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Arjan van de Ven]

On Tue, 20 May 2008 14:56:39 -0700
Joel Becker <Joel.Becker@oracle.com> wrote:

> On Tue, May 20, 2008 at 09:58:10AM -0700, Arjan van de Ven wrote:
> > On Tue, 20 May 2008 18:33:20 +0200
> > Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
> > 
> > > The following patches fix lockdep warnings resulting from
> > > (correct) recursive locking in configfs.
> > >
> > > ...
> > >
> > > Since lockdep does not handle such correct recursion, the idea is
> > > to insert lockdep_off()/lockdep_on() for inode mutexes as soon as
> > > the level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD
> > > dependency pattern increases.
> > 
> > I'm... not entirely happy with such a solution ;(
> > 
> > there must be a better one.
> 
> 	We're trying to find it.  I really appreciate Louis taking the
> time to approach the issue.  His first pass was to add 1 to
> MUTEX_CHILD for each level of recursion.  This has a very tight limit
> (4 or 5 levels), but probably covers all users that exist and perhaps
> all that ever will exist.  However, it means passing the lockdep
> annotation level throughout the entire call chain across multiple
> files.  It was definitely less readable.
> 	This approach takes a different tack - it's very readable, but
> it assumes that the currently correct locking will always remain so -
> a particular invariant that lockdep exists to verify :-)
> 	Louis, what about sticking the recursion level on
> configfs_dirent?  That is, you could add sd->s_level and then use it
> when needed.  THis would hopefully avoid having to pass the level as
> an argument to every function.  Then we can go back to your original
> scheme.  If they recurse too much and hit the lockdep limit, just
> rewind everything and return -ELOOP.

you can also make a new lockdep key for each level... not pretty but it
works

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Joel Becker]

On Tue, May 20, 2008 at 03:13:41PM -0700, Arjan van de Ven wrote:
> > 	Louis, what about sticking the recursion level on
> > configfs_dirent?  That is, you could add sd->s_level and then use it
> > when needed.  THis would hopefully avoid having to pass the level as
> > an argument to every function.  Then we can go back to your original
> > scheme.  If they recurse too much and hit the lockdep limit, just
> > rewind everything and return -ELOOP.
> 
> you can also make a new lockdep key for each level... not pretty but it
> works

	I think that's what we're talking about here.  The toplevel is
I_MUTEX_PARENT, then each child has a class of (I_MUTEX_CHILD + depth),
where depth is the value of s_level.  His original try passed depth
everywhere.  I'm asking him to attach it to the configfs_dirent so that
the code stays readable.  We run into a depth limit at
(MAX_LOCKDEP_SUBCLASS - I_MUTEX_PARENT - 1 == 5), which I think is
probably sane.
	Do you mean something else?  Perhaps not starting from
I_MUTEX_PARENT/CHILD and instead creating CONFIGFS_MUTEX_XXX?

Joel

-- 

"Copy from one, it's plagiarism; copy from two, it's research."
        - Wilson Mizner

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Arjan van de Ven]

On Tue, 20 May 2008 15:27:02 -0700
Joel Becker <Joel.Becker@oracle.com> wrote:

> On Tue, May 20, 2008 at 03:13:41PM -0700, Arjan van de Ven wrote:
> > > 	Louis, what about sticking the recursion level on
> > > configfs_dirent?  That is, you could add sd->s_level and then use
> > > it when needed.  THis would hopefully avoid having to pass the
> > > level as an argument to every function.  Then we can go back to
> > > your original scheme.  If they recurse too much and hit the
> > > lockdep limit, just rewind everything and return -ELOOP.
> > 
> > you can also make a new lockdep key for each level... not pretty
> > but it works
> 
> 	I think that's what we're talking about here.  The toplevel is
> I_MUTEX_PARENT, then each child has a class of (I_MUTEX_CHILD +
> depth), where depth is the value of s_level.  His original try passed
> depth everywhere.  I'm asking him to attach it to the configfs_dirent
> so that the code stays readable.  We run into a depth limit at
> (MAX_LOCKDEP_SUBCLASS - I_MUTEX_PARENT - 1 == 5), which I think is
> probably sane.
> 	Do you mean something else?  Perhaps not starting from
> I_MUTEX_PARENT/CHILD and instead creating CONFIGFS_MUTEX_XXX?

not quite what I meant; what I meant is more like how sched.c deals
with per cpu queues:

(from sched.c)

                spin_lock_init(&rq->lock);
                lockdep_set_class(&rq->lock, &rq->rq_lock_key); 


eg you can override the class (not just add a subclass) for a lock
based on a "key"..

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Joel Becker]

On Tue, May 20, 2008 at 03:35:43PM -0700, Arjan van de Ven wrote:
> not quite what I meant; what I meant is more like how sched.c deals
> with per cpu queues:
> 
> (from sched.c)
> 
>                 spin_lock_init(&rq->lock);
>                 lockdep_set_class(&rq->lock, &rq->rq_lock_key); 

	Looking at this, it's taking the address of the struct
lock_class_key as the actual key.  Thus, if we tie one of these guys to
the structure we're representing, we get lock safety...except that we're
talking about i_mutex here, and we want to interact with the VFS's use
thereof.

Joel

-- 

"There is no sincerer love than the love of food."  
         - George Bernard Shaw 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Louis Rilling]

Sorry for answering late, it seems that we are working in very different
timezones :)

Joel Becker a écrit :
> On Tue, May 20, 2008 at 09:58:10AM -0700, Arjan van de Ven wrote:
>> On Tue, 20 May 2008 18:33:20 +0200
>> Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
>>
>>> The following patches fix lockdep warnings resulting from (correct)
>>> recursive locking in configfs.
>>>
>>> ...
>>>
>>> Since lockdep does not handle such correct recursion, the idea is to
>>> insert lockdep_off()/lockdep_on() for inode mutexes as soon as the
>>> level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD dependency
>>> pattern increases.
>> I'm... not entirely happy with such a solution ;(
>>
>> there must be a better one.
> 
> 	We're trying to find it.  I really appreciate Louis taking the
> time to approach the issue.  His first pass was to add 1 to MUTEX_CHILD
> for each level of recursion.  This has a very tight limit (4 or 5
> levels), but probably covers all users that exist and perhaps all that
> ever will exist.  However, it means passing the lockdep annotation level
> throughout the entire call chain across multiple files.  It was
> definitely less readable.

The former approach limits the level of recursion, but also the total
number of default groups (whole tree) under a created config_group. I
have use cases for which this limit is too low.

> 	This approach takes a different tack - it's very readable, but
> it assumes that the currently correct locking will always remain so - a
> particular invariant that lockdep exists to verify :-)

Note that I keep lockdep on for the first level of recursion, which lets
lockdep prove that the assumption is correct.

> 	Louis, what about sticking the recursion level on
> configfs_dirent?  That is, you could add sd->s_level and then use it
> when needed.  THis would hopefully avoid having to pass the level as an
> argument to every function.  Then we can go back to your original
> scheme.  If they recurse too much and hit the lockdep limit, just rewind
> everything and return -ELOOP.

I can do this. However, the original approach should be modified since
I_MUTEX_CHILD + 1 == I_MUTEX_XATTR and I_MUTEX_CHILD + 2 ==
I_MUTEX_QUOTA. For instance we could redefine inode_i_mutex_lock_class as

enum inode_i_mutex_lock_class
{
        I_MUTEX_NORMAL,
        I_MUTEX_XATTR,
        I_MUTEX_QUOTA,
        I_MUTEX_PARENT,
        I_MUTEX_CHILD,
};

 ... which lets room for only three levels of recursion, and as many
default groups under any created config_group. Unless we increase
MAX_LOCKDEP_SUBCLASS, I'm afraid that this limit is far too low.

I'll send the patch based on sd->s_level, and we'll see...

Louis

-- 
Dr Louis Rilling			Kerlabs - IRISA
Skype: louis.rilling			Campus Universitaire de Beaulieu
Phone: (+33|0) 2 99 84 71 52		Avenue du General Leclerc
Fax: (+33|0) 2 99 84 71 71		35042 Rennes CEDEX - France
http://www.kerlabs.com/

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Peter Zijlstra]

On Tue, 2008-05-20 at 16:51 -0700, Joel Becker wrote:
> On Tue, May 20, 2008 at 03:35:43PM -0700, Arjan van de Ven wrote:
> > not quite what I meant; what I meant is more like how sched.c deals
> > with per cpu queues:
> > 
> > (from sched.c)
> > 
> >                 spin_lock_init(&rq->lock);
> >                 lockdep_set_class(&rq->lock, &rq->rq_lock_key); 
> 
> 	Looking at this, it's taking the address of the struct
> lock_class_key as the actual key.  Thus, if we tie one of these guys to
> the structure we're representing, we get lock safety...except that we're
> talking about i_mutex here, and we want to interact with the VFS's use
> thereof.

Also bear in mind that the lock_class_key structure must be in static
storage.

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Peter Zijlstra]

On Tue, 2008-05-20 at 15:13 -0700, Arjan van de Ven wrote:
> On Tue, 20 May 2008 14:56:39 -0700
> Joel Becker <Joel.Becker@oracle.com> wrote:
> 
> > On Tue, May 20, 2008 at 09:58:10AM -0700, Arjan van de Ven wrote:
> > > On Tue, 20 May 2008 18:33:20 +0200
> > > Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
> > > 
> > > > The following patches fix lockdep warnings resulting from
> > > > (correct) recursive locking in configfs.
> > > >
> > > > ...
> > > >
> > > > Since lockdep does not handle such correct recursion, the idea is
> > > > to insert lockdep_off()/lockdep_on() for inode mutexes as soon as
> > > > the level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD
> > > > dependency pattern increases.
> > > 
> > > I'm... not entirely happy with such a solution ;(
> > > 
> > > there must be a better one.
> > 
> > 	We're trying to find it.  I really appreciate Louis taking the
> > time to approach the issue.  His first pass was to add 1 to
> > MUTEX_CHILD for each level of recursion.  This has a very tight limit
> > (4 or 5 levels), but probably covers all users that exist and perhaps
> > all that ever will exist.  However, it means passing the lockdep
> > annotation level throughout the entire call chain across multiple
> > files.  It was definitely less readable.
> > 	This approach takes a different tack - it's very readable, but
> > it assumes that the currently correct locking will always remain so -
> > a particular invariant that lockdep exists to verify :-)
> > 	Louis, what about sticking the recursion level on
> > configfs_dirent?  That is, you could add sd->s_level and then use it
> > when needed.  THis would hopefully avoid having to pass the level as
> > an argument to every function.  Then we can go back to your original
> > scheme.  If they recurse too much and hit the lockdep limit, just
> > rewind everything and return -ELOOP.
> 
> you can also make a new lockdep key for each level... not pretty but it
> works

Yeah, that is what I've done in the past for trees:

http://programming.kicks-ass.net/kernel-patches/concurrent-pagecache/23-rc1-rt/radix-concurrent-lockdep.patch

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Louis Rilling]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Peter Zijlstra a écrit :
> On Tue, 2008-05-20 at 15:13 -0700, Arjan van de Ven wrote:
>> On Tue, 20 May 2008 14:56:39 -0700
>> Joel Becker <Joel.Becker@oracle.com> wrote:
>>
>>> On Tue, May 20, 2008 at 09:58:10AM -0700, Arjan van de Ven wrote:
>>>> On Tue, 20 May 2008 18:33:20 +0200
>>>> Louis Rilling <Louis.Rilling@kerlabs.com> wrote:
>>>>
>>>>> The following patches fix lockdep warnings resulting from
>>>>> (correct) recursive locking in configfs.
>>>>>
>>>>> ...
>>>>>
>>>>> Since lockdep does not handle such correct recursion, the idea is
>>>>> to insert lockdep_off()/lockdep_on() for inode mutexes as soon as
>>>>> the level of recursion of the I_MUTEX_PARENT -> I_MUTEX_CHILD
>>>>> dependency pattern increases.
>>>> I'm... not entirely happy with such a solution ;(
>>>>
>>>> there must be a better one.
>>> 	We're trying to find it.  I really appreciate Louis taking the
>>> time to approach the issue.  His first pass was to add 1 to
>>> MUTEX_CHILD for each level of recursion.  This has a very tight limit
>>> (4 or 5 levels), but probably covers all users that exist and perhaps
>>> all that ever will exist.  However, it means passing the lockdep
>>> annotation level throughout the entire call chain across multiple
>>> files.  It was definitely less readable.
>>> 	This approach takes a different tack - it's very readable, but
>>> it assumes that the currently correct locking will always remain so -
>>> a particular invariant that lockdep exists to verify :-)
>>> 	Louis, what about sticking the recursion level on
>>> configfs_dirent?  That is, you could add sd->s_level and then use it
>>> when needed.  THis would hopefully avoid having to pass the level as
>>> an argument to every function.  Then we can go back to your original
>>> scheme.  If they recurse too much and hit the lockdep limit, just
>>> rewind everything and return -ELOOP.
>> you can also make a new lockdep key for each level... not pretty but it
>> works
> 
> Yeah, that is what I've done in the past for trees:
> 
> http://programming.kicks-ass.net/kernel-patches/concurrent-pagecache/23-rc1-rt/radix-concurrent-lockdep.patch

Thanks for pointing this out.

Yes this could solve part of the issue, at the price of duplicating the
inode mutex class. However, this still does not solve the issue when
deleting config_groups, since in that case all nodes of the tree are
locked. Thinking about adding lockdep support for concurrent locking of
the direct children of a node in a tree...

Louis

- --
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFIM/ifVKcRuvQ9Q1QRAmahAJ9n07/o3pgOteOgRnMR9a0iGDEFWQCgurOH
9MZ5E9iytPgn/v7QAxDTFrM=
=Q+7A
-----END PGP SIGNATURE-----

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Peter Zijlstra]

On Wed, 2008-05-21 at 12:25 +0200, Louis Rilling wrote:

> > http://programming.kicks-ass.net/kernel-patches/concurrent-pagecache/23-rc1-rt/radix-concurrent-lockdep.patch
> 
> Thanks for pointing this out.
> 
> Yes this could solve part of the issue, at the price of duplicating the
> inode mutex class. However, this still does not solve the issue when
> deleting config_groups, since in that case all nodes of the tree are
> locked. Thinking about adding lockdep support for concurrent locking of
> the direct children of a node in a tree...

Why doesn't sysfs have this problem? - the code says configfs was
derived from sysfs.

Also, do you really need to hold all locks when removing something?
sound like a bit overdone. Also realise there is a maximum number of
held locks - various people have already requested it to be increased or
made dynamic. We're reluctant in doing so because we feel lock chains
should not be of unlimited length. The deeper the chains the bigger the
PI overhead etc..

As to modifying lockdep - it currently doesn't know about trees and
teaching it about them isn't easy.

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Louis Rilling]

Peter Zijlstra a écrit :
> On Wed, 2008-05-21 at 12:25 +0200, Louis Rilling wrote:
> 
>>> http://programming.kicks-ass.net/kernel-patches/concurrent-pagecache/23-rc1-rt/radix-concurrent-lockdep.patch
>> Thanks for pointing this out.
>>
>> Yes this could solve part of the issue, at the price of duplicating the
>> inode mutex class. However, this still does not solve the issue when
>> deleting config_groups, since in that case all nodes of the tree are
>> locked. Thinking about adding lockdep support for concurrent locking of
>> the direct children of a node in a tree...
> 
> Why doesn't sysfs have this problem? - the code says configfs was
> derived from sysfs.

Perhaps because sysfs is driven from the kernel, where behaviors can be
controlled, while in configfs only userspace creates/removes directories.

> 
> Also, do you really need to hold all locks when removing something?
> sound like a bit overdone. Also realise there is a maximum number of
> held locks - various people have already requested it to be increased or
> made dynamic. We're reluctant in doing so because we feel lock chains
> should not be of unlimited length. The deeper the chains the bigger the
> PI overhead etc..

I did not write configfs, so I can only observe that a whole inode tree
is locked when removing a directory hierarchy. I suspect that this is
intended to provide userspace and client sub-systems with some atomic
semantics...

> 
> As to modifying lockdep - it currently doesn't know about trees and
> teaching it about them isn't easy.

That was my guess.

-- 
Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Re: [RFC][PATCH 0/3] configfs: Make nested default groups lockdep-friendly

[Joel Becker]

On Wed, May 21, 2008 at 12:59:37PM +0200, Peter Zijlstra wrote:
> On Wed, 2008-05-21 at 12:25 +0200, Louis Rilling wrote:
> > Yes this could solve part of the issue, at the price of duplicating the
> > inode mutex class. However, this still does not solve the issue when
> > deleting config_groups, since in that case all nodes of the tree are
> > locked. Thinking about adding lockdep support for concurrent locking of
> > the direct children of a node in a tree...
> 
> Why doesn't sysfs have this problem? - the code says configfs was
> derived from sysfs.

	sysfs objects are created from kernel code.  Things register
kobjects one at a time, so it has the usual parent->child dentry
relationships.  This
is handled by I_MUTEX_PARENT/CHILD just fine.
	configfs item creation is triggered from userspace.  mkdir(2)
creates an item in a group.  If that item is a group itself, it can have
child groups that automatically exist ("default_groups").  All of the
default_groups must appear before the userspace-invoked mkdir(2)
returns.  Worse, rmdir(2) must happen atomically as well for these
default_groups (user-created children fail rmdir(2) with ENOTEMPTY as
expected).  For safety, the entire tree under the group being removed
must be locked out.  Hence the recursive locking of the default_group
subdirs. 

> Also, do you really need to hold all locks when removing something?
> sound like a bit overdone. Also realise there is a maximum number of
> held locks - various people have already requested it to be increased or
> made dynamic. We're reluctant in doing so because we feel lock chains
> should not be of unlimited length. The deeper the chains the bigger the
> PI overhead etc..

	Because we need to make sure that all default_group children
(and their default_group children, etc) are locked and safe to remove
from the dcache, we need to hold all of these locks.  I haven't come up
with a safe way to do it atomically while rolling the locks up and down
the tree.

> As to modifying lockdep - it currently doesn't know about trees and
> teaching it about them isn't easy.

	Yeah, it might not be practical.  Hence Louis' most recent patch
to disable lockdep when we start walking one of these trees.  But Arjan
is right that we shouldn't reject it out of hand - lockdep would be nice
here if we can find a way to make it work.

Joel

-- 

	f/8 and be there.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127