Message-Id: <201002100955.52493.dmitry.torokhov@gmail.com>
Date:	Wed, 10 Feb 2010 09:55:51 -0800
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Neil Brown <neilb@...e.de>, Tejun Heo <tj@...nel.org>,
	"Greg Kroah-Hartman" <gregkh@...e.de>,
	linux-kernel@...r.kernel.org,
	Américo Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH] sysfs: differentiate  between locking links and non-links

On Wednesday 10 February 2010 09:36:32 am Eric W. Biederman wrote:
> Neil Brown <neilb@...e.de> writes:
> > Hi,
> >  I've just spent a while sorting out some lockdep complaints triggered
> >  by the recent addition of the "s_active" lockdep annotation in sysfs
> >   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> >
> >  Some of them are genuine and I have submitted a fix for those.
> >  Some are, I think, debatable and I get to that is a minute.  I've
> >  submitted a fix for them anyway.
> >  But some are to my mind clearly bogus and I'm hoping that can be
> >  fixed by the change below (or similar).
> >  The 'bogus' ones are triggered by writing to a sysfs attribute file
> >  for which the handler tries to delete a symlink from sysfs.
> >  This appears to be a recursion on s_active as s_active is held while
> >  the handler runs and is again needed to effect the delete.  However
> >  as the thing being deleted is a symlink, it is very clearly a
> >  different object to the thing triggering the delete, so there is no
> >  real loop.
> >
> >  The following patch splits the lockdep context in two - one for
> >  symlink and one for everything else.  This removes the apparent loop.
> >  (An example report can be seen in
> >      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> >
> >  The "debatable" dependency loops happen when writing to one attribute
> >  causes a different attribute to be deleted.  In my (md) case this can
> >  actually cause a deadlock as both the attributes take the same lock
> >  while the handler is running.  This is because deleting the attribute
> >  will block until the all accesses of that attribute have completed (I
> >  think).
> 
> You are correct.  Not until the file handles are closed but until all
> users of the underyling methods are complete.
> 
> >  However it should be possible to delete a name from sysfs while there
> >  are still accesses pending (it works for normal files!!).  So if
> >  sysfs could be changed to simply unlink the file and leave deletion to
> >  happen when the refcount become zero it would certainly make my life
> >  a lot easier, and allow the removal of some ugly code from md.c.
> >  I don't know sysfs well enough to suggest a patch though.
> 
> Thanks for this.
> 
> Separating out symlinks and treating them differently because they can not
> cause problems is definitely worth doing.  We never take an active
>  reference in the symlink code so we can never block waiting for symlinks
>  to be deleted.
> 
> 
> We block when deleting files in sysfs (and proc and sysctl).  If we
> did not block we could follow pointers into modules that are being
> deleted, or those methods that are running could access data
> structures that we want to tear down (perhaps there is a lock we want
> to kfree).  Blocking in sysfs is to simplify the life of the callers.
> Unfortunately for a handful of callers it complicates things.

Exactly. Before Tejun changed sysfs to provide guarantee that no
show/store methods are still running, nor new references to the
corresponding kobject will be acquired through sysfs after
sysfs_remove_file() returns, you had to jump through million of
hoops at subsystem level to work with lifetime rules and work around
the fact that kobjects could outlive your module.

I was glad to see bunch of ugly code in serio, gameport and input go
and I do not want it coming back ;)

> 
> If you want to compare this to regular files think of what sysfs is
> doing as a combined remove and revoke.  The remove is easy.  Revoke
> is just plane difficult.
> 
> Eric

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists