[PATCH] f2fs: use f2fs_iget_nowait again

[PATCH] f2fs: use f2fs_iget_nowait again

[Jaegeuk Kim]

Previously, I removed f2fs_iget_nowait() through the commit:
d4686d56ec912d55fd8a9d6d509de50de24e90ab
f2fs: avoid balanc_fs during evict_inode.

But, there is another error scenario as follows.

Thread 1:
- writeback_sb_inodes
 - do_writepages
  - f2fs_write_data_pages
   - write_cache_pages
    - f2fs_write_data_page
     - f2fs_balance_fs
      - wait mutex_lock(gc_mutex)

Thread 2:
- f2fs_balance_fs
 - mutex_lock(gc_mutex)
 - f2fs_gc
  - f2fs_iget
   - wait iget_locked(inode->i_lock)

Thread 3:
- do_unlinkat
 - iput
  - lock(inode->i_lock)
   - evict
    - inode_wait_for_writeback

The above three threads are able to incur deadlock condition.
In order to address this issue, let's use f2fs_iget_nowait again in f2fs_gc so
that thread 2 doesn't need to wait for inode->i_lock.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    |  2 +-
 fs/f2fs/inode.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 20aab02..6fd872f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -893,6 +893,7 @@ long f2fs_compat_ioctl(struct file *, unsigned int, unsigned long);
  * inode.c
  */
 void f2fs_set_inode_flags(struct inode *);
+struct inode *f2fs_iget_nowait(struct super_block *, unsigned long);
 struct inode *f2fs_iget(struct super_block *, unsigned long);
 void update_inode(struct inode *, struct page *);
 int update_inode_page(struct inode *);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 25a1f7e..4b43b3f 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -580,7 +580,7 @@ next_step:
 		ofs_in_node = le16_to_cpu(entry->ofs_in_node);
 
 		if (phase == 2) {
-			inode = f2fs_iget(sb, dni.ino);
+			inode = f2fs_iget_nowait(sb, dni.ino);
 			if (IS_ERR(inode))
 				continue;
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 91ac7f9..a0879c3 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -18,6 +18,11 @@
 
 #include <trace/events/f2fs.h>
 
+struct f2fs_iget_args {
+	u64 ino;
+	int on_free;
+};
+
 void f2fs_set_inode_flags(struct inode *inode)
 {
 	unsigned int flags = F2FS_I(inode)->i_flags;
@@ -37,6 +42,34 @@ void f2fs_set_inode_flags(struct inode *inode)
 		inode->i_flags |= S_DIRSYNC;
 }
 
+static int f2fs_iget_test(struct inode *inode, void *data)
+{
+	struct f2fs_iget_args *args = data;
+
+	if (inode->i_ino != args->ino)
+		return 0;
+	if (inode->i_state & (I_FREEING | I_WILL_FREE)) {
+		args->on_free = 1;
+		return 0;
+	}
+	return 1;
+}
+
+struct inode *f2fs_iget_nowait(struct super_block *sb, unsigned long ino)
+{
+	struct f2fs_iget_args args = {
+		.ino = ino,
+		.on_free = 0
+	};
+	struct inode *inode = ilookup5(sb, ino, f2fs_iget_test, &args);
+
+	if (inode)
+		return inode;
+	if (!args.on_free)
+		return f2fs_iget(sb, ino);
+	return ERR_PTR(-ENOENT);
+}
+
 static int do_read_inode(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_SB(inode->i_sb);
-- 
1.8.1.3.566.gaa39828

Re: [PATCH] f2fs: use f2fs_iget_nowait again

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

change log from v1:
 o found another deadlock case, so change the approach

From 145b6ee0b2523bad0e28c09f4ac5bb050cd22085 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Date: Tue, 30 Apr 2013 11:33:27 +0900
Subject: [PATCH] f2fs: avoid deadlock during evict after f2fs_gc
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net

o Deadlock case #1

Thread 1:
- writeback_sb_inodes
 - do_writepages
  - f2fs_write_data_pages
   - write_cache_pages
    - f2fs_write_data_page
     - f2fs_balance_fs
      - wait mutex_lock(gc_mutex)

Thread 2:
- f2fs_balance_fs
 - mutex_lock(gc_mutex)
 - f2fs_gc
  - f2fs_iget
   - wait iget_locked(inode->i_lock)

Thread 3:
- do_unlinkat
 - iput
  - lock(inode->i_lock)
   - evict
    - inode_wait_for_writeback

o Deadlock case #2

Thread 1:
- __writeback_single_inode
 : set I_SYNC
  - do_writepages
   - f2fs_write_data_page
    - f2fs_balance_fs
     - f2fs_gc
      - iput
       - evict
        - inode_wait_for_writeback(I_SYNC)

In order to avoid this, even though iput is called with the
zero-reference
count, we need to stop the eviction procedure if the inode is on
writeback.
So this patch links f2fs_drop_inode which checks the I_SYNC flag.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/super.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index b015b6c..e1c54e4 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -99,6 +99,13 @@ static struct inode *f2fs_alloc_inode(struct
super_block *sb)
 	return &fi->vfs_inode;
 }
 
+static int f2fs_drop_inode(struct inode *inode)
+{
+	if (inode->i_state & I_SYNC)
+		return 0;
+	return generic_drop_inode(inode);
+}
+
 static void f2fs_i_callback(struct rcu_head *head)
 {
 	struct inode *inode = container_of(head, struct inode, i_rcu);
@@ -233,6 +240,7 @@ static int f2fs_show_options(struct seq_file *seq,
struct dentry *root)
 
 static struct super_operations f2fs_sops = {
 	.alloc_inode	= f2fs_alloc_inode,
+	.drop_inode	= f2fs_drop_inode,
 	.destroy_inode	= f2fs_destroy_inode,
 	.write_inode	= f2fs_write_inode,
 	.show_options	= f2fs_show_options,
-- 
1.8.1.3.566.gaa39828



-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] f2fs: use f2fs_iget_nowait again

[Jaegeuk Kim]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

change log from v2:
  o slighly change the previous approach

From 42e49df33bacafc20434e39fc127c9722c9f8691 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Date: Tue, 30 Apr 2013 11:33:27 +0900
Subject: [PATCH] f2fs: avoid deadlock during evict after f2fs_gc
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net

o Deadlock case #1

Thread 1:
- writeback_sb_inodes
 - do_writepages
  - f2fs_write_data_pages
   - write_cache_pages
    - f2fs_write_data_page
     - f2fs_balance_fs
      - wait mutex_lock(gc_mutex)

Thread 2:
- f2fs_balance_fs
 - mutex_lock(gc_mutex)
 - f2fs_gc
  - f2fs_iget
   - wait iget_locked(inode->i_lock)

Thread 3:
- do_unlinkat
 - iput
  - lock(inode->i_lock)
   - evict
    - inode_wait_for_writeback

o Deadlock case #2

Thread 1:
- __writeback_single_inode
 : set I_SYNC
  - do_writepages
   - f2fs_write_data_page
    - f2fs_balance_fs
     - f2fs_gc
      - iput
       - evict
        - inode_wait_for_writeback(I_SYNC)

In order to avoid this, even though iput is called with the
zero-reference
count, we need to stop the eviction procedure if the inode is on
writeback.
So this patch links f2fs_drop_inode which checks the I_SYNC flag.

Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
---
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    |  1 +
 fs/f2fs/super.c | 19 +++++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 20aab02..beb3f06 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -846,6 +846,7 @@ enum {
 	FI_INC_LINK,		/* need to increment i_nlink */
 	FI_ACL_MODE,		/* indicate acl mode */
 	FI_NO_ALLOC,		/* should not allocate any blocks */
+	FI_ON_GC,		/* indicate inode is on gc */
 };
 
 static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 1496159..258b786 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -345,6 +345,7 @@ repeat:
 	}
 	new_ie->inode = inode;
 	list_add_tail(&new_ie->list, ilist);
+	set_inode_flag(F2FS_I(inode), FI_ON_GC);
 }
 
 static void put_gc_inode(struct list_head *ilist)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 5835aaf..6ac1f42 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -98,6 +98,24 @@ static struct inode *f2fs_alloc_inode(struct
super_block *sb)
 	return &fi->vfs_inode;
 }
 
+static int f2fs_drop_inode(struct inode *inode)
+{
+	int drop = generic_drop_inode(inode);
+
+	/*
+	 * This is to avoid a deadlock condition like below.
+	 * writeback_single_inode(inode)
+	 *  - f2fs_write_data_page
+	 *    - f2fs_gc -> iput -> evict
+	 *       - inode_wait_for_writeback(inode)
+	 */
+	if (inode->i_state & I_SYNC &&
+			is_inode_flag_set(F2FS_I(inode), FI_ON_GC))
+		drop = 0;
+	clear_inode_flag(F2FS_I(inode), FI_ON_GC);
+	return drop;
+}
+
 static void f2fs_i_callback(struct rcu_head *head)
 {
 	struct inode *inode = container_of(head, struct inode, i_rcu);
@@ -232,6 +250,7 @@ static int f2fs_show_options(struct seq_file *seq,
struct dentry *root)
 
 static struct super_operations f2fs_sops = {
 	.alloc_inode	= f2fs_alloc_inode,
+	.drop_inode	= f2fs_drop_inode,
 	.destroy_inode	= f2fs_destroy_inode,
 	.write_inode	= f2fs_write_inode,
 	.show_options	= f2fs_show_options,
-- 
1.8.1.3.566.gaa39828



-- 
Jaegeuk Kim
Samsung

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]