Skip to contentSkip to conversation

Containers: Avoid lockdep warning

PATCH In review Paul Menage 3 participants 3 messages Aug 22, 2007 – Aug 23, 2007

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Containers: Avoid lockdep warning

I think this is the right way to handle the lockdep false-positive in the current containers patches, but I'm not that familiar with lockdep so any suggestions for a better approach are welcomed.

In order to avoid a false-positive lockdep warning, we lock the root inode of a new filesystem mount prior to taking container_mutex, to preserve the invariant that container_mutex nests inside inode->i_mutex. In order to prevent a lockdep false positive when locking i_mutex on a newly-created container directory inode we use mutex_lock_nested(), with a nesting level of I_MUTEX_CHILD since the new inode will ultimately be a child directory of the parent whose i_mutex is nested outside of container_mutex.

---
 kernel/container.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
kernel/container.c +7 -10
Index: container-2.6.23-rc3-mm1/kernel/container.c
===================================================================
@@ -966,13 +966,16 @@ static int container_get_sb(struct file_
 	} else {
 		/* New superblock */
 		struct container *cont = &root->top_container;
+		struct inode *inode;
 
 		BUG_ON(sb->s_root != NULL);
 
 		ret = container_get_rootdir(sb);
 		if (ret)
 			goto drop_new_super;
+		inode = sb->s_root->d_inode;
 
+		mutex_lock(&inode->i_mutex);
 		mutex_lock(&container_mutex);
 
 		/*
@@ -985,12 +988,14 @@ static int container_get_sb(struct file_
 		ret = allocate_cg_links(css_group_count, &tmp_cg_links);
 		if (ret) {
 			mutex_unlock(&container_mutex);
+			mutex_unlock(&inode->i_mutex);
 			goto drop_new_super;
 		}
 
 		ret = rebind_subsystems(root, root->subsys_bits);
 		if (ret == -EBUSY) {
 			mutex_unlock(&container_mutex);
+			mutex_unlock(&inode->i_mutex);
 			goto drop_new_super;
 		}
 
@@ -1030,16 +1035,8 @@ static int container_get_sb(struct file_
 		BUG_ON(!list_empty(&cont->children));
 		BUG_ON(root->number_of_containers != 1);
 
-		/*
-		 * I believe that it's safe to nest i_mutex inside
-		 * container_mutex in this case, since no-one else can
-		 * be accessing this directory yet. But we still need
-		 * to teach lockdep that this is the case - currently
-		 * a containerfs remount triggers a lockdep warning
-		 */
-		mutex_lock(&cont->dentry->d_inode->i_mutex);
 		container_populate_dir(cont);
-		mutex_unlock(&cont->dentry->d_inode->i_mutex);
+		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&container_mutex);
 	}
 
@@ -1529,7 +1526,7 @@ static int container_create_file(struct 
 
 		/* start with the directory inode held, so that we can
 		 * populate it without racing with another mkdir */
-		mutex_lock(&inode->i_mutex);
+		mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
 	} else if (S_ISREG(mode)) {
 		inode->i_size = 0;
 		inode->i_fop = &container_file_operations;
#1 lore raw
Quoted 6 lines · Paul Menage

I think this is the right way to handle the lockdep false-positive in the current containers patches, but I'm not that familiar with lockdep so any suggestions for a better approach are welcomed.

Quoted 8 lines

In order to avoid a false-positive lockdep warning, we lock the root inode of a new filesystem mount prior to taking container_mutex, to preserve the invariant that container_mutex nests inside inode->i_mutex. In order to prevent a lockdep false positive when locking i_mutex on a newly-created container directory inode we use mutex_lock_nested(), with a nesting level of I_MUTEX_CHILD since the new inode will ultimately be a child directory of the parent whose i_mutex is nested outside of container_mutex.

Hi Paul,

Just tried it out, and it works for me.

Signature · 4 lines
regards,
Dhaval

I would like to change the world but they don't give me the source code!
Acked-by on v1 1/1
#2 lore raw
In reply to Paul Menage
Quoted 5 lines · Paul Menage

I think this is the right way to handle the lockdep false-positive in the current containers patches, but I'm not that familiar with lockdep so any suggestions for a better approach are welcomed.

Quoted 8 lines

In order to avoid a false-positive lockdep warning, we lock the root inode of a new filesystem mount prior to taking container_mutex, to preserve the invariant that container_mutex nests inside inode->i_mutex. In order to prevent a lockdep false positive when locking i_mutex on a newly-created container directory inode we use mutex_lock_nested(), with a nesting level of I_MUTEX_CHILD since the new inode will ultimately be a child directory of the parent whose i_mutex is nested outside of container_mutex.

So the normal order is:

inode->i_mutex
container_mutex (I_MUTEX_NORMAL)

and there is a one off reversal where we take container_mutex before inode->i_mutex, which is safe because the inode is fresh, so nobody could have possibly taken it to form the deadlock.

mutex_lock_nested() is indeed the proper annotation.

Using I_MUTEX_CHILD forms the chain:

container_mutex

    inode->i_mutex (I_MUTEX_CHILD)

In order for this to become a problem there would need to be a:

inode->i_mutex (I_MUTEX_CHILD)
inode->i_mutex (I_MUTEX_NORMAL)

chain, which I suppose is precluded to exists on basis of common sense :-)

Quoted 1 line
Quoted 60 lines
---
 kernel/container.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Index: container-2.6.23-rc3-mm1/kernel/container.c
===================================================================

kernel/container.c +6 -9
@@ -966,13 +966,16 @@ static int container_get_sb(struct file_
 	} else {
 		/* New superblock */
 		struct container *cont = &root->top_container;
+		struct inode *inode;
 
 		BUG_ON(sb->s_root != NULL);
 
 		ret = container_get_rootdir(sb);
 		if (ret)
 			goto drop_new_super;
+		inode = sb->s_root->d_inode;
 
+		mutex_lock(&inode->i_mutex);
 		mutex_lock(&container_mutex);
 
 		/*
@@ -985,12 +988,14 @@ static int container_get_sb(struct file_
 		ret = allocate_cg_links(css_group_count, &tmp_cg_links);
 		if (ret) {
 			mutex_unlock(&container_mutex);
+			mutex_unlock(&inode->i_mutex);
 			goto drop_new_super;
 		}
 
 		ret = rebind_subsystems(root, root->subsys_bits);
 		if (ret == -EBUSY) {
 			mutex_unlock(&container_mutex);
+			mutex_unlock(&inode->i_mutex);
 			goto drop_new_super;
 		}
 
@@ -1030,16 +1035,8 @@ static int container_get_sb(struct file_
 		BUG_ON(!list_empty(&cont->children));
 		BUG_ON(root->number_of_containers != 1);
 
-		/*
-		 * I believe that it's safe to nest i_mutex inside
-		 * container_mutex in this case, since no-one else can
-		 * be accessing this directory yet. But we still need
-		 * to teach lockdep that this is the case - currently
-		 * a containerfs remount triggers a lockdep warning
-		 */
-		mutex_lock(&cont->dentry->d_inode->i_mutex);
 		container_populate_dir(cont);
-		mutex_unlock(&cont->dentry->d_inode->i_mutex);
+		mutex_unlock(&inode->i_mutex);
 		mutex_unlock(&container_mutex);
 	}
 

kernel/container.c static int container_create_file(struct · line 1532

Quoted 8 lines
kernel/container.c +1 -1
@@ -1529,7 +1526,7 @@ static int container_create_file(struct 
 
 		/* start with the directory inode held, so that we can
 		 * populate it without racing with another mkdir */
-		mutex_lock(&inode->i_mutex);
+		mutex_lock_nested(&inode->i_mutex, I_MUTEX_CHILD);
 	} else if (S_ISREG(mode)) {
 		inode->i_size = 0;
 		inode->i_fop = &container_file_operations;

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers