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

[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

[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