From: Louis Rilling <Louis.Rilling@kerlabs.com>
To: Joel.Becker@oracle.com
Cc: ocfs2-devel@oss.oracle.com, linux-kernel@vger.kernel.org
Subject: [Ocfs2-devel] [RFC][PATCH 4/4] configfs: Make multiple default_group destructions lockdep friendly
Date: Fri, 23 May 2008 18:44:58 +0200 [thread overview]
Message-ID: <4836F48A.70008@kerlabs.com> (raw)
In-Reply-To: <20080522114947.927196541@kerlabs.com>
Louis Rilling a ?crit :
> When destroying a config_group having multiple (nested or not) default groups,
> lockdep raises a warning because multiple locks of class I_MUTEX_NORMAL are
> taken in configfs_detach_prep().
>
> This patch makes such default group destructions lockdep-friendly by increasing
> the i_mutex sub-class from I_MUTEX_CHILD + 1 onwards for each default group
> locked under a being-destroyed config_group.
I discovered two bugs, as described below, and fixed in the attached
version of the patch. Sorry for the noise.
[...]
>
> Index: b/fs/configfs/dir.c
> ===================================================================
> --- a/fs/configfs/dir.c 2008-05-22 12:38:02.000000000 +0200
> +++ b/fs/configfs/dir.c 2008-05-22 12:49:08.000000000 +0200
[...]
> @@ -383,7 +395,15 @@ static int configfs_detach_prep(struct d
> if (sd->s_type & CONFIGFS_NOT_PINNED)
> continue;
> if (sd->s_type & CONFIGFS_USET_DEFAULT) {
> - mutex_lock(&sd->s_dentry->d_inode->i_mutex);
> + lock_level = set_dirent_lock_level(parent_sd, sd);
> + if (lock_level < 0) {
> + /* Bad bad bad! We cannot remove a directory
> + * that we let be created! */
> + ret = -ELOOP;
> + break;
> + }
> + mutex_lock_nested(&sd->s_dentry->d_inode->i_mutex,
> + I_MUTEX_CHILD + lock_level);
> /* Mark that we've taken i_mutex */
> sd->s_type |= CONFIGFS_USET_DROPPING;
>
Here setting lock_level before acquiring the mutex may race with mkdir
(and thus configfs_attach_group()) in the default group. A corrected
version of the patch is attached.
[...]
> @@ -1206,7 +1230,11 @@ static int configfs_rmdir(struct inode *
> return -EINVAL;
> }
>
> + /* The inode of the config_item being removed is already locked by
> + * vfs_rmdir() with subclass I_MUTEX_CHILD. Account for it. */
> + set_dirent_lock_level(parent_item->ci_dentry->d_fsdata, sd);
> ret = configfs_detach_prep(dentry);
> + reset_dirent_lock_level(sd);
> if (ret) {
> configfs_detach_rollback(dentry);
> config_item_put(parent_item);
lock_level should be reset on rollback, since mkdir may be called again
after a failure of rmdir. Again, fixed in the new version of the patch
in attachment.
Louis
--
Dr Louis Rilling Kerlabs
Skype: louis.rilling Batiment Germanium
Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
http://www.kerlabs.com/ 35700 Rennes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: configfs-make-default-group-destruction-lockdep-friendly-2.patch
Type: text/x-diff
Size: 7137 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080523/53738fb6/attachment.bin
WARNING: multiple messages have this Message-ID (diff)
From: Louis Rilling <Louis.Rilling@kerlabs.com>
To: Joel.Becker@oracle.com
Cc: ocfs2-devel@oss.oracle.com, linux-kernel@vger.kernel.org
Subject: Re: [RFC][PATCH 4/4] configfs: Make multiple default_group destructions lockdep friendly
Date: Fri, 23 May 2008 18:44:58 +0200 [thread overview]
Message-ID: <4836F48A.70008@kerlabs.com> (raw)
In-Reply-To: <20080522114947.927196541@kerlabs.com>
[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]
Louis Rilling a écrit :
> When destroying a config_group having multiple (nested or not) default groups,
> lockdep raises a warning because multiple locks of class I_MUTEX_NORMAL are
> taken in configfs_detach_prep().
>
> This patch makes such default group destructions lockdep-friendly by increasing
> the i_mutex sub-class from I_MUTEX_CHILD + 1 onwards for each default group
> locked under a being-destroyed config_group.
I discovered two bugs, as described below, and fixed in the attached
version of the patch. Sorry for the noise.
[...]
>
> Index: b/fs/configfs/dir.c
> ===================================================================
> --- a/fs/configfs/dir.c 2008-05-22 12:38:02.000000000 +0200
> +++ b/fs/configfs/dir.c 2008-05-22 12:49:08.000000000 +0200
[...]
> @@ -383,7 +395,15 @@ static int configfs_detach_prep(struct d
> if (sd->s_type & CONFIGFS_NOT_PINNED)
> continue;
> if (sd->s_type & CONFIGFS_USET_DEFAULT) {
> - mutex_lock(&sd->s_dentry->d_inode->i_mutex);
> + lock_level = set_dirent_lock_level(parent_sd, sd);
> + if (lock_level < 0) {
> + /* Bad bad bad! We cannot remove a directory
> + * that we let be created! */
> + ret = -ELOOP;
> + break;
> + }
> + mutex_lock_nested(&sd->s_dentry->d_inode->i_mutex,
> + I_MUTEX_CHILD + lock_level);
> /* Mark that we've taken i_mutex */
> sd->s_type |= CONFIGFS_USET_DROPPING;
>
Here setting lock_level before acquiring the mutex may race with mkdir
(and thus configfs_attach_group()) in the default group. A corrected
version of the patch is attached.
[...]
> @@ -1206,7 +1230,11 @@ static int configfs_rmdir(struct inode *
> return -EINVAL;
> }
>
> + /* The inode of the config_item being removed is already locked by
> + * vfs_rmdir() with subclass I_MUTEX_CHILD. Account for it. */
> + set_dirent_lock_level(parent_item->ci_dentry->d_fsdata, sd);
> ret = configfs_detach_prep(dentry);
> + reset_dirent_lock_level(sd);
> if (ret) {
> configfs_detach_rollback(dentry);
> config_item_put(parent_item);
lock_level should be reset on rollback, since mkdir may be called again
after a failure of rmdir. Again, fixed in the new version of the patch
in attachment.
Louis
--
Dr Louis Rilling Kerlabs
Skype: louis.rilling Batiment Germanium
Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
http://www.kerlabs.com/ 35700 Rennes
[-- Attachment #2: configfs-make-default-group-destruction-lockdep-friendly-2.patch --]
[-- Type: text/x-diff, Size: 7342 bytes --]
configfs: Make multiple default_group destructions lockdep friendly
When destroying a config_group having multiple (nested or not) default groups,
lockdep raises a warning because multiple locks of class I_MUTEX_NORMAL are
taken in configfs_detach_prep().
This patch makes such default group destructions lockdep-friendly by increasing
the i_mutex sub-class from I_MUTEX_CHILD + 1 onwards for each default group
locked under a being-destroyed config_group.
With this patch and lockdep compiled-in, the number of default groups (whatever
their depth in the tree) under a user-created config_group (resp. registered
subsystem) is limited to MAX_LOCKDEP_SUBCLASSES - I_MUTEX_CHILD - 1 == 3. This
limit is removed when not compiling lockdep.
=============================================
[ INFO: possible recursive locking detected ]
2.6.26-rc3 #5
---------------------------------------------
rmdir/1499 is trying to acquire lock:
(&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
but task is already holding lock:
(&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff80296513>] vfs_rmdir+0x49/0xac
other info that might help us debug this:
2 locks held by rmdir/1499:
#0: (&sb->s_type->i_mutex_key#3/1){--..}, at: [<ffffffff80298138>] do_rmdir+0x82/0x108
#1: (&sb->s_type->i_mutex_key#11){--..}, at: [<ffffffff80296513>] vfs_rmdir+0x49/0xac
stack backtrace:
Pid: 1499, comm: rmdir Not tainted 2.6.26-rc3 #5
Call Trace:
[<ffffffff8024afe9>] __lock_acquire+0x8d2/0xc78
[<ffffffff80266399>] filemap_fault+0x1cf/0x332
[<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
[<ffffffff8024b762>] lock_acquire+0x51/0x6c
[<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
[<ffffffff80248331>] debug_mutex_lock_common+0x16/0x23
[<ffffffff805d6244>] mutex_lock_nested+0xcd/0x23b
[<ffffffff802d2529>] configfs_detach_prep+0x54/0x89
[<ffffffff802d3656>] configfs_rmdir+0xb8/0x1c3
[<ffffffff80296535>] vfs_rmdir+0x6b/0xac
[<ffffffff8029816d>] do_rmdir+0xb7/0x108
[<ffffffff8024a2a2>] trace_hardirqs_on+0xef/0x113
[<ffffffff805d7364>] trace_hardirqs_on_thunk+0x35/0x3a
[<ffffffff8020b0cb>] system_call_after_swapgs+0x7b/0x80
Signed-off-by: Louis Rilling <Louis.Rilling@kerlabs.com>
---
fs/configfs/dir.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 66 insertions(+), 8 deletions(-)
Index: b/fs/configfs/dir.c
===================================================================
--- a/fs/configfs/dir.c 2008-05-22 13:36:06.000000000 +0200
+++ b/fs/configfs/dir.c 2008-05-23 16:46:43.000000000 +0200
@@ -37,16 +37,32 @@
DECLARE_RWSEM(configfs_rename_sem);
#ifdef CONFIG_LOCKDEP
+static inline int __future_dirent_lock_level(struct configfs_dirent *prev_sd,
+ struct configfs_dirent *sd)
+{
+ int lock_level = prev_sd->s_lock_level + 1;
+ return (lock_level + I_MUTEX_CHILD < MAX_LOCKDEP_SUBCLASSES) ?
+ lock_level : -ELOOP;
+}
+
+static inline void __set_dirent_lock_level(struct configfs_dirent *sd,
+ int lock_level)
+{
+ sd->s_lock_level = lock_level;
+}
+
static inline int set_dirent_lock_level(struct configfs_dirent *prev_sd,
struct configfs_dirent *sd)
{
- int lock_level = prev_sd->s_lock_level + 1;
- if (lock_level + I_MUTEX_CHILD < MAX_LOCKDEP_SUBCLASSES) {
- sd->s_lock_level = lock_level;
- return lock_level;
- }
- sd->s_lock_level = -1;
- return -ELOOP;
+ int lock_level = __future_dirent_lock_level(prev_sd, sd);
+ __set_dirent_lock_level(sd, lock_level < 0 ? -1 : lock_level);
+ return lock_level;
+}
+
+static inline void copy_dirent_lock_level(struct configfs_dirent *from,
+ struct configfs_dirent *to)
+{
+ to->s_lock_level = from->s_lock_level;
}
static inline void reset_dirent_lock_level(struct configfs_dirent *sd)
@@ -56,12 +72,28 @@ static inline void reset_dirent_lock_lev
#else /* CONFIG_LOCKDEP */
+static inline int __future_dirent_lock_level(struct configfs_dirent *prev_sd,
+ struct configfs_dirent *sd)
+{
+ return 0;
+}
+
+static inline void __set_dirent_lock_level(struct configfs_dirent *sd,
+ int lock_level)
+{
+}
+
static inline int set_dirent_lock_level(struct configfs_dirent *prev_sd,
struct configfs_dirent *sd)
{
return 0;
}
+static inline void copy_dirent_lock_level(struct configfs_dirent *from,
+ struct configfs_dirent *to)
+{
+}
+
static inline void reset_dirent_lock_level(struct configfs_dirent *sd)
{
}
@@ -372,6 +404,7 @@ static int configfs_detach_prep(struct d
{
struct configfs_dirent *parent_sd = dentry->d_fsdata;
struct configfs_dirent *sd;
+ int lock_level;
int ret;
ret = -EBUSY;
@@ -383,7 +416,19 @@ static int configfs_detach_prep(struct d
if (sd->s_type & CONFIGFS_NOT_PINNED)
continue;
if (sd->s_type & CONFIGFS_USET_DEFAULT) {
- mutex_lock(&sd->s_dentry->d_inode->i_mutex);
+ /* Do not set lock_level before we acquire the mutex,
+ * otherwise a racing mkdir in sd could start from a
+ * too high lock_level */
+ lock_level = __future_dirent_lock_level(parent_sd, sd);
+ if (lock_level < 0) {
+ /* Bad bad bad! We cannot remove a directory
+ * that we let be created! */
+ ret = -ELOOP;
+ break;
+ }
+ mutex_lock_nested(&sd->s_dentry->d_inode->i_mutex,
+ I_MUTEX_CHILD + lock_level);
+ __set_dirent_lock_level(sd, lock_level);
/* Mark that we've taken i_mutex */
sd->s_type |= CONFIGFS_USET_DROPPING;
@@ -392,6 +437,10 @@ static int configfs_detach_prep(struct d
* deep nesting of default_groups
*/
ret = configfs_detach_prep(sd->s_dentry);
+ /* Update parent's lock_level so that remaining
+ * sibling children keep on globally increasing
+ * lock_level */
+ copy_dirent_lock_level(sd, parent_sd);
if (!ret)
continue;
} else
@@ -419,6 +468,7 @@ static void configfs_detach_rollback(str
if (sd->s_type & CONFIGFS_USET_DROPPING) {
sd->s_type &= ~CONFIGFS_USET_DROPPING;
+ reset_dirent_lock_level(sd);
mutex_unlock(&sd->s_dentry->d_inode->i_mutex);
}
}
@@ -1206,9 +1256,14 @@ static int configfs_rmdir(struct inode *
return -EINVAL;
}
+ /* The inode of the config_item being removed is already locked by
+ * vfs_rmdir() with subclass I_MUTEX_CHILD. Account for it. */
+ set_dirent_lock_level(parent_item->ci_dentry->d_fsdata, sd);
ret = configfs_detach_prep(dentry);
+ reset_dirent_lock_level(sd);
if (ret) {
configfs_detach_rollback(dentry);
+ reset_dirent_lock_level(sd);
config_item_put(parent_item);
return ret;
}
@@ -1492,10 +1547,13 @@ void configfs_unregister_subsystem(struc
mutex_lock_nested(&configfs_sb->s_root->d_inode->i_mutex,
I_MUTEX_PARENT);
+ /* Sets subsys's configfs_dirent lock_level to 0 */
+ set_dirent_lock_level(configfs_sb->s_root->d_fsdata, dentry->d_fsdata);
mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD);
if (configfs_detach_prep(dentry)) {
printk(KERN_ERR "configfs: Tried to unregister non-empty subsystem!\n");
}
+ reset_dirent_lock_level(dentry->d_fsdata);
configfs_detach_group(&group->cg_item);
dentry->d_inode->i_flags |= S_DEAD;
mutex_unlock(&dentry->d_inode->i_mutex);
next prev parent reply other threads:[~2008-05-23 16:44 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-05-22 11:40 [Ocfs2-devel] [RFC][PATCH 0/4] configfs: Make nested default groups lockdep-friendly v2 Louis Rilling
2008-05-22 11:40 ` Louis Rilling
2008-05-22 11:40 ` [Ocfs2-devel] [RFC][PATCH 1/4] Prepare i_mutex lockdep subclasses for locking of variable path lengths Louis Rilling
2008-05-22 11:40 ` Louis Rilling
2008-05-22 11:40 ` [Ocfs2-devel] [RFC][PATCH 2/4] Prepare vfs_rmdir() for further nested i_mutex locking Louis Rilling
2008-05-22 11:40 ` Louis Rilling
2008-05-22 11:40 ` [Ocfs2-devel] [RFC][PATCH 3/4] configfs: Make nested default groups creations lockdep-friendly Louis Rilling
2008-05-22 11:40 ` Louis Rilling
2008-05-22 11:40 ` [Ocfs2-devel] [RFC][PATCH 4/4] configfs: Make multiple default_group destructions lockdep friendly Louis Rilling
2008-05-22 11:40 ` Louis Rilling
2008-05-23 16:44 ` Louis Rilling [this message]
2008-05-23 16:44 ` Louis Rilling
2008-06-02 23:07 ` [Ocfs2-devel] " Joel Becker
2008-06-02 23:07 ` Joel Becker
2008-06-03 16:00 ` [Ocfs2-devel] " Louis Rilling
2008-06-03 16:00 ` Louis Rilling
2008-06-06 23:01 ` [Ocfs2-devel] " Joel Becker
2008-06-06 23:01 ` Joel Becker
2008-06-09 11:03 ` [Ocfs2-devel] " Louis Rilling
2008-06-09 11:03 ` Louis Rilling
2008-06-09 12:54 ` [Ocfs2-devel] [BUG] deadlock between configfs_rmdir() and sys_rename() (WAS Re: [RFC][PATCH 4/4] configfs: Make multiple default_group) " Louis Rilling
2008-06-09 12:54 ` Louis Rilling
2008-06-10 1:58 ` [Ocfs2-devel] " Joel Becker
2008-06-10 1:58 ` Joel Becker
2008-06-10 10:14 ` [Ocfs2-devel] " Louis Rilling
2008-06-10 10:14 ` Louis Rilling
2008-06-10 17:36 ` [Ocfs2-devel] " Joel Becker
2008-06-10 17:36 ` Joel Becker
2008-06-11 14:10 ` [Ocfs2-devel] " Louis Rilling
2008-06-11 14:10 ` Louis Rilling
2008-06-11 17:30 ` [Ocfs2-devel] " Louis Rilling
2008-06-11 17:30 ` Louis Rilling
2008-06-11 21:15 ` [Ocfs2-devel] " Joel Becker
2008-06-11 21:15 ` Joel Becker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4836F48A.70008@kerlabs.com \
--to=louis.rilling@kerlabs.com \
--cc=Joel.Becker@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ocfs2-devel@oss.oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.