[3/3] jffs2: put directories f->sem on a separate lockdep class

Message ID d36fa783db66ec430565a065245ebcacfea39e13.1524823321.git.h.grohne@intenta.de
State New
Delegated to: David Woodhouse
Headers show
Series
  • jffs2: fix lockdep warning
Related show

Commit Message

Helmut Grohne April 27, 2018, 10:30 a.m.
We get a lockdep warning if we read a directory (e.g. ls) and fault a page on a
regular file:

[  181.206207] mmaptest/392 is trying to acquire lock:
[  181.211066]  (&f->sem){+.+.}, at: [<c01ff7d8>] jffs2_readpage+0x30/0x5c
[  181.217663]
[  181.217663] but task is already holding lock:
[  181.223477]  (&mm->mmap_sem){++++}, at: [<c04928e8>] do_page_fault+0xa8/0x454
[  181.230596]
[  181.230596] which lock already depends on the new lock.
[  181.230596]
[  181.238755]
[  181.238755] the existing dependency chain (in reverse order) is:
[  181.246219]
[  181.246219] -> #1 (&mm->mmap_sem){++++}:
[  181.251614]        __might_fault+0x90/0xc0
[  181.255694]        filldir+0xa4/0x1f4
[  181.259334]        jffs2_readdir+0xd8/0x1d4
[  181.263499]        iterate_dir+0x78/0x128
[  181.267490]        SyS_getdents+0x8c/0x12c
[  181.271576]        ret_fast_syscall+0x0/0x28
[  181.275818]
[  181.275818] -> #0 (&f->sem){+.+.}:
[  181.280690]        lock_acquire+0xfc/0x2b0
[  181.284763]        __mutex_lock+0x8c/0xb0c
[  181.288842]        mutex_lock_nested+0x2c/0x34
[  181.293272]        jffs2_readpage+0x30/0x5c
[  181.297438]        filemap_fault+0x600/0x73c
[  181.301690]        __do_fault+0x28/0xb8
[  181.305510]        handle_mm_fault+0x854/0xec0
[  181.309937]        do_page_fault+0x384/0x454
[  181.314188]        do_DataAbort+0x4c/0xc8
[  181.318181]        __dabt_usr+0x44/0x60
[  181.321999]        0xb6dffb02

Like with inode->i_rwsem, we recognize that this can never result in a dead
lock, because we cannot page fault directory inodes and we cannot call getdents
on non-directories. Thus we need different lockdep classes depending on the
mode. This mirrors what lockdep_annotate_inode_mutex_key does to inodes.

Link: https://www.spinics.net/lists/linux-rt-users/msg07644.html
Signed-off-by: Helmut Grohne <h.grohne@intenta.de>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 fs/jffs2/fs.c    | 12 ++++++++++++
 fs/jffs2/super.c |  3 +++
 2 files changed, 15 insertions(+)

Comments

David Woodhouse April 27, 2018, 12:54 p.m. | #1
On Fri, 2018-04-27 at 12:30 +0200, Helmut Grohne wrote:
> 
> @@ -60,6 +62,7 @@ static void jffs2_i_init_once(void *foo)
>         struct jffs2_inode_info *f = foo;
>  
>         mutex_init(&f->sem);
> +       lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key);
>         inode_init_once(&f->vfs_inode);
>  }
>  

The point in init_once is that inode structures can get reused. So if
it gets used the first time for a directory, then re-used for a file,
nothing ever sets the lockdep class back to jffs2_inode_info_sem_key.

Other than that, this all looks good; thanks!

Patch

diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
index 89a10b398d00..1717c464a21a 100644
--- a/fs/jffs2/fs.c
+++ b/fs/jffs2/fs.c
@@ -26,6 +26,8 @@ 
 #include <linux/crc32.h>
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_dir_key;
+
 static int jffs2_flash_setup(struct jffs2_sb_info *c);
 
 int jffs2_do_setattr (struct inode *inode, struct iattr *iattr)
@@ -277,6 +279,11 @@  struct inode *jffs2_iget(struct super_block *sb, unsigned long ino)
 
 	inode->i_mode = jemode_to_cpu(latest_node.mode);
 
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	if (S_ISDIR(inode->i_mode))
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_dir_key);
 	mutex_lock(&f->sem);
 
 	i_uid_write(inode, je16_to_cpu(latest_node.uid));
@@ -439,6 +446,11 @@  struct inode *jffs2_new_inode (struct inode *dir_i, umode_t mode, struct jffs2_r
 
 	f = JFFS2_INODE_INFO(inode);
 	jffs2_init_inode_info(f);
+	/* Mirror lock class of inode->i_rwsem, see
+	 * lockdep_annotate_inode_mutex_key.
+	 */
+	if (S_ISDIR(mode))
+		lockdep_set_class(&f->sem, &jffs2_inode_info_sem_dir_key);
 	mutex_lock(&f->sem);
 
 	memset(ri, 0, sizeof(*ri));
diff --git a/fs/jffs2/super.c b/fs/jffs2/super.c
index 87bdf0f4cba1..f338a4f4bec5 100644
--- a/fs/jffs2/super.c
+++ b/fs/jffs2/super.c
@@ -30,6 +30,8 @@ 
 #include "compr.h"
 #include "nodelist.h"
 
+static struct lock_class_key jffs2_inode_info_sem_key;
+
 static void jffs2_put_super(struct super_block *);
 
 static struct kmem_cache *jffs2_inode_cachep;
@@ -60,6 +62,7 @@  static void jffs2_i_init_once(void *foo)
 	struct jffs2_inode_info *f = foo;
 
 	mutex_init(&f->sem);
+	lockdep_set_class(&f->sem, &jffs2_inode_info_sem_key);
 	inode_init_once(&f->vfs_inode);
 }