Containers: Avoid lockdep warning
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
@@ -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;
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 thatcontainer_mutexnests insideinode->i_mutex. In order to prevent a lockdep false positive when lockingi_mutexon a newly-created container directory inode we usemutex_lock_nested(), with a nesting level ofI_MUTEX_CHILDsince the new inode will ultimately be a child directory of the parent whosei_mutexis nested outside ofcontainer_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!
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 thatcontainer_mutexnests insideinode->i_mutex. In order to prevent a lockdep false positive when lockingi_mutexon a newly-created container directory inode we usemutex_lock_nested(), with a nesting level ofI_MUTEX_CHILDsince the new inode will ultimately be a child directory of the parent whosei_mutexis nested outside ofcontainer_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
Signed-off-by Paul Menage <[email protected]>
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