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

[Ocfs2-devel] [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

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

[Louis Rilling]

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 

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

[Louis Rilling]

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 

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

[Louis Rilling]

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 

[Ocfs2-devel] [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.

[Ocfs2-devel] [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

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

[Ocfs2-devel] [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

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

[Ocfs2-devel] [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"..

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

[Ocfs2-devel] [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.

[Ocfs2-devel] [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

[Ocfs2-devel] [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-----

[Ocfs2-devel] [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.

[Ocfs2-devel] [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

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

[Ocfs2-devel] [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/

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