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

[Louis Rilling <Louis.Rilling@kerlabs.com>]

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/