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

From: Louis Rilling
Date: Wed May 21 2008 - 04:13:55 EST


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@xxxxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/