[Cluster-devel] Re: Make lockdep happy with configfs

[Cluster-devel] Re: Make lockdep happy with configfs

[Joel Becker]

On Mon, Jan 26, 2009 at 12:51:52PM +0100, Louis Rilling wrote:
> Are you going to take one of these approaches to make lockdep and configfs
> cohabit?

	I'm going to look at your patches now and do some review - sorry
it took me so long.
	I do think our discussion with Peter elsewhere may have even
better fruit, so I'm going to hold of on including these just yet.

Joel


-- 

Life's Little Instruction Book #510

	"Count your blessings."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 1/2] configfs: Silence lockdep on mkdir() and rmdir()

[Joel Becker]

On Thu, Dec 18, 2008 at 07:00:17PM +0100, Louis Rilling wrote:
> >From inside configfs it is not possible to serialize those recursive
> locking with a top-level one, because mkdir() and rmdir() are already
> called with inodes locked by the VFS. So using some
> mutex_lock_nest_lock() is not an option.
> 
> I am proposing two solutions:
> 1) one that wraps recursive mutex_lock()s with
>    lockdep_off()/lockdep_on().
> 2) (as suggested earlier by Peter Zijlstra) one that puts the
>    i_mutexes recursively locked in different classes based on their
>    depth from the top-level config_group created. This
>    induces an arbitrary limit (MAX_LOCK_DEPTH - 2 == 46) on the
>    nesting of configfs default groups whenever lockdep is activated
>    but this limit looks reasonably high. Unfortunately, this also
>    isolates VFS operations on configfs default groups from the others
>    and thus lowers the chances to detect locking issues.
> 
> Nobody likes solution 1), what I can understand.

	Me too :-P

> This patch implements solution 2). However lockdep is still not happy with
> configfs_depend_item(). Next patch reworks the locking of
> configfs_depend_item() and finally makes lockdep happy.

<snip>

>  #define CONFIGFS_ROOT		0x0001
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 8e93341..f21be74 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -94,6 +94,9 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
>  	INIT_LIST_HEAD(&sd->s_links);
>  	INIT_LIST_HEAD(&sd->s_children);
>  	sd->s_element = element;
> +#ifdef CONFIG_LOCKDEP
> +	sd->s_depth = -1;
> +#endif
>  	spin_lock(&configfs_dirent_lock);
>  	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
>  		spin_unlock(&configfs_dirent_lock);
> @@ -176,6 +179,17 @@ static int init_symlink(struct inode * inode)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LOCKDEP
> +static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
> +					  struct configfs_dirent *sd)
> +{
> +	int parent_depth = parent_sd->s_depth;
> +
> +	if (parent_depth >= 0)
> +		sd->s_depth = parent_depth + 1;
> +}
> +#endif
> +
>  static int create_dir(struct config_item * k, struct dentry * p,
>  		      struct dentry * d)
>  {
> @@ -187,6 +201,9 @@ static int create_dir(struct config_item * k, struct dentry * p,
>  		error = configfs_make_dirent(p->d_fsdata, d, k, mode,
>  					     CONFIGFS_DIR | CONFIGFS_USET_CREATING);
>  	if (!error) {
> +#ifdef CONFIG_LOCKDEP
> +		configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
> +#endif
>  		error = configfs_create(d, mode, init_dir);
>  		if (!error) {
>  			inc_nlink(p->d_inode);

	Can you change this to provide non-lockdep versions of
functions?  We don't want "#ifdef CONFIG_LOCKDEP" everywhere.  What we
want is the code to call functions unconditionally, and the functions to
do nothing if lockdep is not enabled.  Like:

#ifdef CONFIG_LOCKDEP
static inline void configfs_init_dir_dirent_depth(dirent)
{
    dirent->s_depth = -1;
}

static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
					  struct configfs_dirent *sd)
{
	int parent_depth = parent_sd->s_depth;

	if (parent_depth >= 0)
		sd->s_depth = parent_depth + 1;
}
#else
static inline void configfs_init_dir_dirent_depth(dirent)
{
}

static void configfs_set_dir_dirent_depth(struct configfs_dirent *parent_sd,
					  struct configfs_dirent *sd)
{
}
#endif

	This makes the callsites much nicer to read:

@@ -94,6 +94,9 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
 	INIT_LIST_HEAD(&sd->s_links);
 	INIT_LIST_HEAD(&sd->s_children);
 	sd->s_element = element;
+	configfs_init_dir_dirent_depth(sd);
 	spin_lock(&configfs_dirent_lock);
 	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
 		spin_unlock(&configfs_dirent_lock);
@@ -187,6 +201,7 @@ static int create_dir(struct config_item * k, struct dentry * p,
 		error = configfs_make_dirent(p->d_fsdata, d, k, mode,
 					     CONFIGFS_DIR | CONFIGFS_USET_CREATING);
 	if (!error) {
+		configfs_set_dir_dirent_depth(p->d_fsdata, d->d_fsdata);
 		error = configfs_create(d, mode, init_dir);
 		if (!error) {
 			inc_nlink(p->d_inode);

	Otherwise, this patch seems pretty straightforward.

Joel

-- 

Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy

[Joel Becker]

On Thu, Dec 18, 2008 at 07:00:18PM +0100, Louis Rilling wrote:
> configfs_depend_item() recursively locks all inodes mutex from configfs root to
> the target item, which makes lockdep unhappy. The purpose of this recursive
> locking is to ensure that the item tree can be safely parsed and that the target
> item, if found, is not about to leave.
> 
> This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
> Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
> protects tagging of items to be removed, this lock can be used instead of the
> inodes mutex lock chain.
> This needs that the check for dependents be done atomically with
> CONFIGFS_USET_DROPPING tagging.
> 
> Now lockdep looks happy with configfs.

	This looks almost, but not quite right.
	In the create path, we do configfs_new_dirent() before we set
sd->s_type.  But configfs_new_dirent() attaches sd->s_sibling.  So, in
aonther thread, configfs_depend_prep() can traverse this s_sibling
without CONFIGFS_USET_CREATING being set.  This turns out to be safe
because CONFIGFS_DIR is also not set - but boy I'd like a comment about
that.
	What if we're in mkdir(2) in one thread and another thread is  
trying to pin the parent directory?  That is, we are inside
configfs_mkdir(parent, new_dentry, mode).  The other thread is doing
configfs_depend_item(subsys, parent).  With this patch, the other thread
will not take parent->i_mutex.  It will happily determine that
parent is part of the tree and bump its s_dependent with no locking.  Is
this OK?
	If it is - isn't this patch good without any other reason?  That
is, aside from the issues of lockdep, isn't it better for
configfs_depend_item() to never have to worry about the VFS locks other
than the configfs root?

Joel


-- 

 The zen have a saying:
 "When you learn how to listen, ANYONE can be your teacher."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy

[Joel Becker]

On Wed, Jan 28, 2009 at 07:18:33PM +0100, Louis Rilling wrote:
> configfs_depend_item() recursively locks all inodes mutex from configfs root to
> the target item, which makes lockdep unhappy. The purpose of this recursive
> locking is to ensure that the item tree can be safely parsed and that the target
> item, if found, is not about to leave.
> 
> This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
> Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
> protects tagging of items to be removed, this lock can be used instead of the
> inodes mutex lock chain.
> This needs that the check for dependents be done atomically with
> CONFIGFS_USET_DROPPING tagging.

	These patches are now in the 'lockdep' branch of the configfs
tree.  I'm planning to send them in the next merge window.  I've made
one change.

> + * Note: items in the middle of attachment start with s_type = 0
> + * (configfs_new_dirent()), and configfs_make_dirent() (called from
> + * create_dir()) sets s_type = CONFIGFS_DIR|CONFIGFS_USET_CREATING. In both
> + * cases the item is ignored. Since s_type is an int, we rely on the CPU to
> + * atomically update the value, without making configfs_make_dirent() take
> + * configfs_dirent_lock.

	I've added configfs_dirent_lock in configfs_make_dirent(),
because it is not safe at all to rely on the fact that s_type is an int.
It's an atomic set on one CPU, but there's no guarantee that it's seen
correctly on other CPUs.  Plus, there's no real need for speed here.  So
we properly take configfs_dirent_lock around s_type in
configfs_make_dirent(), and that ensures we see things correctly on SMP.

Joel

-- 

"Three o'clock is always too late or too early for anything you
 want to do."
        - Jean-Paul Sartre

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy

[Joel Becker]

On Thu, Apr 30, 2009 at 11:18:28AM +0200, Louis Rilling wrote:
> On 29/04/09 11:52 -0700, Joel Becker wrote:
> > On Wed, Jan 28, 2009 at 07:18:33PM +0100, Louis Rilling wrote:
> > > configfs_depend_item() recursively locks all inodes mutex from configfs root to
> > > the target item, which makes lockdep unhappy. The purpose of this recursive
> > > locking is to ensure that the item tree can be safely parsed and that the target
> > > item, if found, is not about to leave.
> > > 
> > > This patch reworks configfs_depend_item() locking using configfs_dirent_lock.
> > > Since configfs_dirent_lock protects all changes to the configfs_dirent tree, and
> > > protects tagging of items to be removed, this lock can be used instead of the
> > > inodes mutex lock chain.
> > > This needs that the check for dependents be done atomically with
> > > CONFIGFS_USET_DROPPING tagging.
> > 
> > 	These patches are now in the 'lockdep' branch of the configfs
> > tree.  I'm planning to send them in the next merge window.  I've made
> > one change.
> > 
> > > + * Note: items in the middle of attachment start with s_type = 0
> > > + * (configfs_new_dirent()), and configfs_make_dirent() (called from
> > > + * create_dir()) sets s_type = CONFIGFS_DIR|CONFIGFS_USET_CREATING. In both
> > > + * cases the item is ignored. Since s_type is an int, we rely on the CPU to
> > > + * atomically update the value, without making configfs_make_dirent() take
> > > + * configfs_dirent_lock.
> > 
> > 	I've added configfs_dirent_lock in configfs_make_dirent(),
> > because it is not safe at all to rely on the fact that s_type is an int.
> > It's an atomic set on one CPU, but there's no guarantee that it's seen
> > correctly on other CPUs.  Plus, there's no real need for speed here.  So
> > we properly take configfs_dirent_lock around s_type in
> > configfs_make_dirent(), and that ensures we see things correctly on SMP.
> 
> Agreed, I was going to suggest something like this. Actually I'd push the
> initialization of s_type down to configfs_new_dirent(), so that s_type either
> is always NULL, or always shows the correct type of object.

	0, not "NULL", but yeah I think that's a good plan.

Joel

-- 

"If at first you don't succeed, cover all traces that you tried."
                                                        -Unknown

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Cluster-devel] Re: [PATCH 2/2] configfs: Rework configfs_depend_item() locking and make lockdep happy

[Joel Becker]

On Thu, Apr 30, 2009 at 10:20:14AM -0700, Joel Becker wrote:
> On Thu, Apr 30, 2009 at 11:18:28AM +0200, Louis Rilling wrote:
> > Agreed, I was going to suggest something like this. Actually I'd push the
> > initialization of s_type down to configfs_new_dirent(), so that s_type either
> > is always NULL, or always shows the correct type of object.
> 
> 	0, not "NULL", but yeah I think that's a good plan.

	Like this.  Please review the comment change mostly.


diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 63d8815..8e48b52 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -167,8 +167,8 @@ configfs_adjust_dir_dirent_depth_after_populate(struct configfs_dirent *sd)
 /*
  * Allocates a new configfs_dirent and links it to the parent configfs_dirent
  */
-static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * parent_sd,
-						void * element)
+static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent *parent_sd,
+						   void *element, int type)
 {
 	struct configfs_dirent * sd;
 
@@ -180,6 +180,7 @@ static struct configfs_dirent *configfs_new_dirent(struct configfs_dirent * pare
 	INIT_LIST_HEAD(&sd->s_links);
 	INIT_LIST_HEAD(&sd->s_children);
 	sd->s_element = element;
+	sd->s_type = type;
 	configfs_init_dirent_depth(sd);
 	spin_lock(&configfs_dirent_lock);
 	if (parent_sd->s_type & CONFIGFS_USET_DROPPING) {
@@ -225,19 +226,12 @@ int configfs_make_dirent(struct configfs_dirent * parent_sd,
 {
 	struct configfs_dirent * sd;
 
-	sd = configfs_new_dirent(parent_sd, element);
+	sd = configfs_new_dirent(parent_sd, element, type);
 	if (IS_ERR(sd))
 		return PTR_ERR(sd);
 
-	/*
-	 * We need configfs_dirent_lock so that configfs_depend_prep()
-	 * can see s_type accurately on other CPUs.
-	 */
-	spin_lock(&configfs_dirent_lock);
 	sd->s_mode = mode;
-	sd->s_type = type;
 	sd->s_dentry = dentry;
-	spin_unlock(&configfs_dirent_lock);
 	if (dentry) {
 		dentry->d_fsdata = configfs_get(sd);
 		dentry->d_op = &configfs_dentry_ops;
@@ -1034,11 +1028,10 @@ static int configfs_dump(struct configfs_dirent *sd, int level)
  * dead, as well as items in the middle of attachment since they virtually
  * do not exist yet. This completes the locking out of racing mkdir() and
  * rmdir().
- * Note: items in the middle of attachment start with s_type = 0
- * (configfs_new_dirent()), and configfs_make_dirent() (called from
- * create_dir()) sets s_type = CONFIGFS_DIR|CONFIGFS_USET_CREATING. In both
- * cases the item is ignored.  configfs_make_dirent() is locked out from
- * updating s_type by configfs_dirent_lock.
+ * Note: subdirectories in the middle of attachment start with s_type =
+ * CONFIGFS_DIR|CONFIGFS_USET_CREATING set by create_dir().  When
+ * CONFIGFS_USET_CREATING is set, we ignore the item.  The actual set of
+ * s_type is in configfs_new_dirent(), which has configfs_dirent_lock.
  *
  * If the target is not found, -ENOENT is bubbled up.
  *
@@ -1514,7 +1507,7 @@ static int configfs_dir_open(struct inode *inode, struct file *file)
 	 */
 	err = -ENOENT;
 	if (configfs_dirent_is_ready(parent_sd)) {
-		file->private_data = configfs_new_dirent(parent_sd, NULL);
+		file->private_data = configfs_new_dirent(parent_sd, NULL, 0);
 		if (IS_ERR(file->private_data))
 			err = PTR_ERR(file->private_data);
 		else

-- 

"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