Re: [PATCH] configfs: Silence lockdep on mkdir(), rmdir() andconfigfs_depend_item()

[Louis Rilling]

On 26/01/09 15:19 +0100, Peter Zijlstra wrote:
> On Mon, 2009-01-26 at 15:00 +0100, Louis Rilling wrote:
> 
> > > Its not a locking correctness thing, but simply not being able to do it
> > > from the vfs calls because those assume locks held?
> > > 
> > > Can't you simply punt the work to a worklet once you've created/removed
> > > the non-default group, which can be done from within the vfs callback ?
> > 
> > I'm not sure to understand your suggestion. Is this:
> > 1) for mkdir(), create the non-default group, but without its default groups,
> > and defer their creation to a worker which won't have constraints on locks held
> > by any caller;
> > 2) for rmdir(), unlink the non-default group, but without unlinking its default
> > groups, and defer the recursive work to a lock-free context?
> > 
> > For mkdir(), this may work. Maybe a bit confusing for userspace, since mkdir(A)
> > returns as soon as A is created, but A may be populated later and userspace may
> > rely on A being populated as soon as it is created (current behavior). As a
> > configfs user, this makes my life harder...
> 
> Right, so that is the whole crux of the matter?

Probably not. I'm not the maintainer of configfs, but I guess that Joel is a bit
reluctant to deeply rework parts of something that actually works (conflicts
with lockdep excepted).

> 
> Initially I understood the whole recursive locking issue to be about
> having to serialize mkdir vs rmdir so that you would know the default
> groups to be empty etc.
> 
> You could create the subtree before you link it in. i_op->mkdir() only
> has the parent i_mutex held, so you should be able to create your inode,
> and all default groups (some of who will have the non-default group as
> parent, but that's ok, as we don't have that locked yet).
> 
> Once you've constructed this, you could connect the non-default group to
> its parent.
> 
> Also, you don't _need_ to have any i_mutex's locked here, because non of
> these inodes are reachable.

True. I already suggested this to Joel (while fixing a race condition), but this
raises other issues (see http://marc.info/?l=linux-kernel&m=121438776626316&w=2
for a previous discussion on this).

> 
> > For rmdir(), is this safe to unlink a non-empty directory, and to empty it
> > afterwards? This looks like going back to the unmount problem.
> 
> Dunno :-), I think it should be safe. The only guarantee you need is
> that there are no refs to inodes in the decoupled sub-tree (other than
> your own of course.)
> 
> So you'd only need to punt the rmdir cleanup to eventd or something.

May be. Anyway I can't investigate this right now, and that's why I'm asking
Joel if he is going to accept one of the temporary solutions that I provided
(Note that my second solution
http://marc.info/?l=linux-kernel&m=122962334723834&w=2 does not turn off
lockdep!). Of course it's better if someone can just do this rework :)

Thanks,

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

Attachment: signature.asc
Description: Digital signature