Ocfs2 devel - May 2008 - [RFC][PATCH 0/3] 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.

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

An embedded and charset-unspecified text was scrubbed...
Name: configfs-tell-configfs_attach_group-if-default-group.patch
Url:
http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080520/d826024b/attachment.pl

An embedded and charset-unspecified text was scrubbed...
Name: configfs-silence-lockdep-with-default-group-creation.patch
Url:
http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080520/e3813e8b/attachment.pl

An embedded and charset-unspecified text was scrubbed...
Name: configfs-silence-lockdep-with-default-group-destruction.patch
Url:
http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080520/1eb92a52/attachment.pl

On Tue, 20 May 2008 18:33:20 +0200
Louis Rilling <Louis.Rilling at 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.

Arjan van de Ven a ?crit :
> On Tue, 20 May 2008 18:33:20 +0200
> Louis Rilling <Louis.Rilling at 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

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 at oracle.com
Phone: (650) 506-8127

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 at 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 at oracle.com
Phone: (650) 506-8127