Skip to contentSkip to conversation

configfs: Make nested default groups lockdep-friendly

RFC PATCH In review Louis Rilling 4 participants 19 messages May 20, 2008 – May 21, 2008 OCFS2

lore
Share & tools
lore

Louis Rilling

author
#0 lore raw

cover configfs: Make nested default groups lockdep-friendly

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.

Signature · 4 lines
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
#4 lore raw
In reply to Louis Rilling
Quoted 12 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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()).

Quoted 4 lines

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.

author
#5 lore raw

Arjan van de Ven a ?crit :

Quoted 15 lines
Quoted 12 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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()).

Quoted 8 lines
Quoted 4 lines

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?

Signature · 4 lines
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
#6 lore raw
In reply to Arjan van de Ven
Quoted 8 lines · Arjan van de Ven
Quoted 5 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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

...

Quoted 8 lines
Quoted 4 lines

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

Signature · 9 lines
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 at oracle.com
Phone: (650) 506-8127
#7 lore raw
Quoted 18 lines · On Tue, 20 May 2008 14:56:39 -0700 Joel Becker <[email protected]> wrote:
Quoted 8 lines · Arjan van de Ven
Quoted 5 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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

...

Quoted 8 lines
Quoted 4 lines

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.

Quoted 16 lines

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

#8 lore raw
Quoted 9 lines · Arjan van de Ven
Quoted 6 lines

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

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

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
#9 lore raw
Quoted 11 lines · On Tue, 20 May 2008 15:27:02 -0700 Joel Becker <[email protected]> wrote:
Quoted 9 lines · Arjan van de Ven
Quoted 6 lines

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

Quoted 9 lines

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"..

#10 lore raw
Quoted 7 lines · Arjan van de Ven

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

Signature · 8 lines
"There is no sincerer love than the love of food."  
         - George Bernard Shaw 

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
#11 lore raw
Quoted 6 lines · Joel Becker
Quoted 5 lines · Arjan van de Ven

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

(from sched.c)

Quoted 8 lines
Quoted 2 lines
                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.

#12 lore raw
In reply to Arjan van de Ven
Quoted 38 lines · Arjan van de Ven
Quoted 18 lines · On Tue, 20 May 2008 14:56:39 -0700 Joel Becker <[email protected]> wrote:
Quoted 8 lines · Arjan van de Ven
Quoted 5 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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

...

Quoted 8 lines
Quoted 4 lines

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.

Quoted 16 lines

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.

Quoted 2 lines

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

author
#13 lore raw

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

Peter Zijlstra a ?crit :

Quoted 36 lines · Arjan van de Ven
Quoted 35 lines
Quoted 32 lines · On Tue, 20 May 2008 14:56:39 -0700 Joel Becker <[email protected]> wrote:
Quoted 8 lines · Arjan van de Ven
Quoted 5 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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

...

Quoted 7 lines
Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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-----

#14 lore raw
Quoted 9 lines · Louis Rilling
Quoted 1 line

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.

author
#15 lore raw

Peter Zijlstra a ?crit :

Quoted 5 lines · Louis Rilling
Quoted 4 lines
Quoted 2 lines

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

Thanks for pointing this out.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 7 lines

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...

Quoted 3 lines

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

That was my guess.

Signature · 4 lines
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
#16 lore raw
In reply to Peter Zijlstra
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Louis Rilling

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.

Quoted 6 lines

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.

Quoted 2 lines

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

Signature · 7 lines
	f/8 and be there.

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
author
#17 lore raw
In reply to Joel Becker

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

Joel Becker a ?crit :

Quoted 17 lines · Arjan van de Ven
Quoted 8 lines
Quoted 5 lines · On Tue, 20 May 2008 18:33:20 +0200 Louis Rilling <[email protected]>…

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

...

Quoted 7 lines
Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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

Signature · 5 lines
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/
#18 lore raw
In reply to Louis Rilling
Quoted 10 lines · Louis Rilling

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()).

Quoted 3 lines

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

Signature · 9 lines
"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 at oracle.com
Phone: (650) 506-8127