f2fs: fix to avoid data update racing between GC and DIO

From: Chao Yu <[email protected]>

Datas in file can be operated by GC and DIO simultaneously, so we will face race case as below:

For write case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- dio_bio_submit
update user data to old block address

For read case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_balance_fs
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- write_checkpoint
- do_checkpoint
- clear_prefree_segments
- f2fs_issue_discard discard old block adress
- dio_bio_submit
update user buffer from obsolete block address

In order to fix this, for one file, we should let DIO and GC getting exclusion against with each other.

Signed-off-by Chao Yu <[email protected]>

---
v3: use semaphore to avoid racing in between read dio and write dio.

 fs/f2fs/data.c  |  4 ++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    | 13 +++++++++++++
 fs/f2fs/super.c |  1 +
 4 files changed, 19 insertions(+)

fs/f2fs/data.c +4 -0

index b6fd5bd..19197bb 100644
@@ -1712,6 +1712,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_inode_info *fi = F2FS_I(inode);
 	size_t count = iov_iter_count(iter);
 	loff_t offset = iocb->ki_pos;
 	int err;

@@ -1727,7 +1728,10 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
+	down_read(&fi->dio_rwsem);
 	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
+	up_read(&fi->dio_rwsem);
+
 	if (iov_iter_rw(iter) == WRITE) {
 		if (err > 0)
 			set_inode_flag(inode, FI_UPDATE_WRITE);

fs/f2fs/f2fs.h +1 -0

index bf9a13a..2e439ec 100644
@@ -474,6 +474,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
+	struct rw_semaphore dio_rwsem;	/* avoid racing between dio and gc */
 };
 
 static inline void get_extent_info(struct extent_info *ext,

fs/f2fs/gc.c +13 -0

index c612137..a9bfb8d 100644
@@ -755,12 +755,25 @@ next_step:
 		/* phase 3 */
 		inode = find_gc_inode(gc_list, dni.ino);
 		if (inode) {
+			struct f2fs_inode_info *fi = F2FS_I(inode);
+			bool locked = false;
+
+			if (S_ISREG(inode->i_mode)) {
+				if (!down_write_trylock(&fi->dio_rwsem))
+					continue;
+				locked = true;
+			}
+
 			start_bidx = start_bidx_of_node(nofs, inode)
 								+ ofs_in_node;
 			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
 				move_encrypted_block(inode, start_bidx);
 			else
 				move_data_page(inode, start_bidx, gc_type);
+
+			if (locked)
+				up_write(&fi->dio_rwsem);
+
 			stat_inc_data_blk_count(sbi, 1, gc_type);
 		}
 	}

fs/f2fs/super.c +1 -0

index edd1b35..dde57fb 100644
@@ -579,6 +579,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	INIT_LIST_HEAD(&fi->inmem_pages);
 	mutex_init(&fi->inmem_lock);
+	init_rwsem(&fi->dio_rwsem);
 
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;

Signature · 1 line

2.7.2

Hi Chao,

Could you take a look at this in xfstests/generic/013?

[  502.480850] ======================================================
[  502.480864] [ INFO: possible circular locking dependency detected ]
[  502.480877] 4.7.0-rc1+ #124 Tainted: G           OE  
[  502.480886] -------------------------------------------------------
[  502.480897] fsstress/10729 is trying to acquire lock:
[  502.480906]  (&sb->s_type->i_mutex_key#18){+.+.+.}, at: [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.480948] 
[  502.480948] but task is already holding lock:
[  502.480959]  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481003] 
[  502.481003] which lock already depends on the new lock.
[  502.481003] 
[  502.481018] 
[  502.481018] the existing dependency chain (in reverse order) is:
[  502.481030] 
[  502.481030] -> #1 (&fi->dio_rwsem){.+.+.+}:
[  502.481054]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481071]        [<ffffffff818d1921>] down_read+0x51/0xa0
[  502.481089]        [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481114]        [<ffffffff811c34c7>] generic_file_direct_write+0xa7/0x160
[  502.481133]        [<ffffffff811c363d>] __generic_file_write_iter+0xbd/0x1e0
[  502.481149]        [<ffffffffc080437b>] f2fs_file_write_iter+0xdb/0x100 [f2fs]
[  502.481173]        [<ffffffff81253a88>] __vfs_write+0xc8/0x140
[  502.481190]        [<ffffffff81254c55>] vfs_write+0xb5/0x1b0
[  502.481205]        [<ffffffff81255fe9>] SyS_write+0x49/0xa0
[  502.481220]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481236] 
[  502.481236] -> #0 (&sb->s_type->i_mutex_key#18){+.+.+.}:
[  502.481264]        [<ffffffff810e481c>] __lock_acquire+0x161c/0x1940
[  502.481280]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481296]        [<ffffffff818d1b9a>] down_write+0x5a/0xc0
[  502.481312]        [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.481328]        [<ffffffff8129bdaa>] __blockdev_direct_IO+0x3a/0x40
[  502.481344]        [<ffffffffc081e2e4>] f2fs_direct_IO+0x104/0x3d0 [f2fs]
[  502.481368]        [<ffffffff811c40a9>] generic_file_read_iter+0x689/0x7e0
[  502.481384]        [<ffffffff812545d1>] __vfs_read+0xc1/0x130
[  502.481399]        [<ffffffff81254af1>] vfs_read+0x91/0x140
[  502.481414]        [<ffffffff81255f49>] SyS_read+0x49/0xa0
[  502.481429]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481445] 
[  502.481445] other info that might help us debug this:
[  502.481445] 

[  502.481459]  Possible unsafe locking scenario:
[  502.481459] 
[  502.481726]        CPU0                    CPU1
[  502.481987]        ----                    ----
[  502.482242]   lock(&fi->dio_rwsem);
[  502.482501]                                lock(&sb->s_type->i_mutex_key#18);
[  502.482765]                                lock(&fi->dio_rwsem);
[  502.483025]   lock(&sb->s_type->i_mutex_key#18);

[  502.483285] 
[  502.483285]  *** DEADLOCK ***
[  502.483285] 
[  502.484018] 1 lock held by fsstress/10729:
[  502.484262]  #0:  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]

Thanks,

Quoted 132 lines · Chao Yu

From: Chao Yu <[email protected]>

Datas in file can be operated by GC and DIO simultaneously, so we will face race case as below:

For write case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- dio_bio_submit
update user data to old block address

For read case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_balance_fs
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- write_checkpoint
- do_checkpoint
- clear_prefree_segments
- f2fs_issue_discard discard old block adress
- dio_bio_submit
update user buffer from obsolete block address

In order to fix this, for one file, we should let DIO and GC getting exclusion against with each other.

Signed-off-by Chao Yu <[email protected]>

---
v3: use semaphore to avoid racing in between read dio and write dio.

 fs/f2fs/data.c  |  4 ++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    | 13 +++++++++++++
 fs/f2fs/super.c |  1 +
 4 files changed, 19 insertions(+)

fs/f2fs/data.c +4 -0

index b6fd5bd..19197bb 100644
@@ -1712,6 +1712,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_inode_info *fi = F2FS_I(inode);
 	size_t count = iov_iter_count(iter);
 	loff_t offset = iocb->ki_pos;
 	int err;

@@ -1727,7 +1728,10 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
+	down_read(&fi->dio_rwsem);
 	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
+	up_read(&fi->dio_rwsem);
+
 	if (iov_iter_rw(iter) == WRITE) {
 		if (err > 0)
 			set_inode_flag(inode, FI_UPDATE_WRITE);

fs/f2fs/f2fs.h +1 -0

index bf9a13a..2e439ec 100644
@@ -474,6 +474,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
+	struct rw_semaphore dio_rwsem;	/* avoid racing between dio and gc */
 };
 
 static inline void get_extent_info(struct extent_info *ext,

fs/f2fs/gc.c +13 -0

index c612137..a9bfb8d 100644
@@ -755,12 +755,25 @@ next_step:
 		/* phase 3 */
 		inode = find_gc_inode(gc_list, dni.ino);
 		if (inode) {
+			struct f2fs_inode_info *fi = F2FS_I(inode);
+			bool locked = false;
+
+			if (S_ISREG(inode->i_mode)) {
+				if (!down_write_trylock(&fi->dio_rwsem))
+					continue;
+				locked = true;
+			}
+
 			start_bidx = start_bidx_of_node(nofs, inode)
 								+ ofs_in_node;
 			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
 				move_encrypted_block(inode, start_bidx);
 			else
 				move_data_page(inode, start_bidx, gc_type);
+
+			if (locked)
+				up_write(&fi->dio_rwsem);
+
 			stat_inc_data_blk_count(sbi, 1, gc_type);
 		}
 	}

fs/f2fs/super.c +1 -0

index edd1b35..dde57fb 100644
@@ -579,6 +579,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	INIT_LIST_HEAD(&fi->inmem_pages);
 	mutex_init(&fi->inmem_lock);
+	init_rwsem(&fi->dio_rwsem);
 

Quoted 4 lines

fs/f2fs/super.c +0 -0

 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;

Signature · 1 line

2.7.2

Hi Jaegeuk,

Quoted 46 lines · Jaegeuk Kim

Hi Chao,

Could you take a look at this in xfstests/generic/013?

[  502.480850] ======================================================
[  502.480864] [ INFO: possible circular locking dependency detected ]
[  502.480877] 4.7.0-rc1+ #124 Tainted: G           OE  
[  502.480886] -------------------------------------------------------
[  502.480897] fsstress/10729 is trying to acquire lock:
[  502.480906]  (&sb->s_type->i_mutex_key#18){+.+.+.}, at: [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.480948] 
[  502.480948] but task is already holding lock:
[  502.480959]  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481003] 
[  502.481003] which lock already depends on the new lock.
[  502.481003] 
[  502.481018] 
[  502.481018] the existing dependency chain (in reverse order) is:
[  502.481030] 
[  502.481030] -> #1 (&fi->dio_rwsem){.+.+.+}:
[  502.481054]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481071]        [<ffffffff818d1921>] down_read+0x51/0xa0
[  502.481089]        [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481114]        [<ffffffff811c34c7>] generic_file_direct_write+0xa7/0x160
[  502.481133]        [<ffffffff811c363d>] __generic_file_write_iter+0xbd/0x1e0
[  502.481149]        [<ffffffffc080437b>] f2fs_file_write_iter+0xdb/0x100 [f2fs]
[  502.481173]        [<ffffffff81253a88>] __vfs_write+0xc8/0x140
[  502.481190]        [<ffffffff81254c55>] vfs_write+0xb5/0x1b0
[  502.481205]        [<ffffffff81255fe9>] SyS_write+0x49/0xa0
[  502.481220]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481236] 
[  502.481236] -> #0 (&sb->s_type->i_mutex_key#18){+.+.+.}:
[  502.481264]        [<ffffffff810e481c>] __lock_acquire+0x161c/0x1940
[  502.481280]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481296]        [<ffffffff818d1b9a>] down_write+0x5a/0xc0
[  502.481312]        [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.481328]        [<ffffffff8129bdaa>] __blockdev_direct_IO+0x3a/0x40
[  502.481344]        [<ffffffffc081e2e4>] f2fs_direct_IO+0x104/0x3d0 [f2fs]
[  502.481368]        [<ffffffff811c40a9>] generic_file_read_iter+0x689/0x7e0
[  502.481384]        [<ffffffff812545d1>] __vfs_read+0xc1/0x130
[  502.481399]        [<ffffffff81254af1>] vfs_read+0x91/0x140
[  502.481414]        [<ffffffff81255f49>] SyS_read+0x49/0xa0
[  502.481429]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481445] 
[  502.481445] other info that might help us debug this:
[  502.481445] 

Quoted 8 lines

[  502.481459]  Possible unsafe locking scenario:
[  502.481459] 
[  502.481726]        CPU0                    CPU1
[  502.481987]        ----                    ----
[  502.482242]   lock(&fi->dio_rwsem);
[  502.482501]                                lock(&sb->s_type->i_mutex_key#18);
[  502.482765]                                lock(&fi->dio_rwsem);
[  502.483025]   lock(&sb->s_type->i_mutex_key#18);

Seems we will suffer ABBA deadlock:

writer reader
- f2fs_file_write_iter
- down_write(&inode->i_rwsem)
- __generic_file_write_iter
- generic_file_direct_write
- f2fs_direct_IO
- generic_file_read_iter
- f2fs_direct_IO
- down_read(&fi->dio_rwsem)
- __blockdev_direct_IO
- do_blockdev_direct_IO
- down_write(&inode->i_rwsem)

- down_read(&fi->dio_rwsem)

What about splitting dio_rwsem to rdio_rwsem/wdio_rwsem for reader/writer to avoid deadlock?

Thanks,

Quoted 145 lines

[  502.483285] 
[  502.483285]  *** DEADLOCK ***
[  502.483285] 
[  502.484018] 1 lock held by fsstress/10729:
[  502.484262]  #0:  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]

Thanks,

Quoted 136 lines · Chao Yu

From: Chao Yu <[email protected]>

Datas in file can be operated by GC and DIO simultaneously, so we will face race case as below:

For write case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- dio_bio_submit
update user data to old block address

For read case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_balance_fs
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- write_checkpoint
- do_checkpoint
- clear_prefree_segments
- f2fs_issue_discard discard old block adress
- dio_bio_submit
update user buffer from obsolete block address

In order to fix this, for one file, we should let DIO and GC getting exclusion against with each other.

Signed-off-by Chao Yu <[email protected]>

---
v3: use semaphore to avoid racing in between read dio and write dio.

 fs/f2fs/data.c  |  4 ++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    | 13 +++++++++++++
 fs/f2fs/super.c |  1 +
 4 files changed, 19 insertions(+)

fs/f2fs/data.c +4 -0

index b6fd5bd..19197bb 100644
@@ -1712,6 +1712,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_inode_info *fi = F2FS_I(inode);
 	size_t count = iov_iter_count(iter);
 	loff_t offset = iocb->ki_pos;
 	int err;

@@ -1727,7 +1728,10 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
+	down_read(&fi->dio_rwsem);
 	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
+	up_read(&fi->dio_rwsem);
+
 	if (iov_iter_rw(iter) == WRITE) {
 		if (err > 0)
 			set_inode_flag(inode, FI_UPDATE_WRITE);

fs/f2fs/f2fs.h +1 -0

index bf9a13a..2e439ec 100644
@@ -474,6 +474,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
+	struct rw_semaphore dio_rwsem;	/* avoid racing between dio and gc */
 };
 
 static inline void get_extent_info(struct extent_info *ext,

fs/f2fs/gc.c +13 -0

index c612137..a9bfb8d 100644
@@ -755,12 +755,25 @@ next_step:
 		/* phase 3 */
 		inode = find_gc_inode(gc_list, dni.ino);
 		if (inode) {
+			struct f2fs_inode_info *fi = F2FS_I(inode);
+			bool locked = false;
+
+			if (S_ISREG(inode->i_mode)) {
+				if (!down_write_trylock(&fi->dio_rwsem))
+					continue;
+				locked = true;
+			}
+
 			start_bidx = start_bidx_of_node(nofs, inode)
 								+ ofs_in_node;
 			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
 				move_encrypted_block(inode, start_bidx);
 			else
 				move_data_page(inode, start_bidx, gc_type);
+
+			if (locked)
+				up_write(&fi->dio_rwsem);
+
 			stat_inc_data_blk_count(sbi, 1, gc_type);
 		}
 	}

fs/f2fs/super.c +1 -0

index edd1b35..dde57fb 100644
@@ -579,6 +579,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	INIT_LIST_HEAD(&fi->inmem_pages);
 	mutex_init(&fi->inmem_lock);
+	init_rwsem(&fi->dio_rwsem);
 
 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;

Signature · 1 line

2.7.2

Quoted 73 lines · Chao Yu

Hi Jaegeuk,

Quoted 46 lines · Jaegeuk Kim

Hi Chao,

Could you take a look at this in xfstests/generic/013?

[  502.480850] ======================================================
[  502.480864] [ INFO: possible circular locking dependency detected ]
[  502.480877] 4.7.0-rc1+ #124 Tainted: G           OE  
[  502.480886] -------------------------------------------------------
[  502.480897] fsstress/10729 is trying to acquire lock:
[  502.480906]  (&sb->s_type->i_mutex_key#18){+.+.+.}, at: [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.480948] 
[  502.480948] but task is already holding lock:
[  502.480959]  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481003] 
[  502.481003] which lock already depends on the new lock.
[  502.481003] 
[  502.481018] 
[  502.481018] the existing dependency chain (in reverse order) is:
[  502.481030] 
[  502.481030] -> #1 (&fi->dio_rwsem){.+.+.+}:
[  502.481054]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481071]        [<ffffffff818d1921>] down_read+0x51/0xa0
[  502.481089]        [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481114]        [<ffffffff811c34c7>] generic_file_direct_write+0xa7/0x160
[  502.481133]        [<ffffffff811c363d>] __generic_file_write_iter+0xbd/0x1e0
[  502.481149]        [<ffffffffc080437b>] f2fs_file_write_iter+0xdb/0x100 [f2fs]
[  502.481173]        [<ffffffff81253a88>] __vfs_write+0xc8/0x140
[  502.481190]        [<ffffffff81254c55>] vfs_write+0xb5/0x1b0
[  502.481205]        [<ffffffff81255fe9>] SyS_write+0x49/0xa0
[  502.481220]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481236] 
[  502.481236] -> #0 (&sb->s_type->i_mutex_key#18){+.+.+.}:
[  502.481264]        [<ffffffff810e481c>] __lock_acquire+0x161c/0x1940
[  502.481280]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481296]        [<ffffffff818d1b9a>] down_write+0x5a/0xc0
[  502.481312]        [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.481328]        [<ffffffff8129bdaa>] __blockdev_direct_IO+0x3a/0x40
[  502.481344]        [<ffffffffc081e2e4>] f2fs_direct_IO+0x104/0x3d0 [f2fs]
[  502.481368]        [<ffffffff811c40a9>] generic_file_read_iter+0x689/0x7e0
[  502.481384]        [<ffffffff812545d1>] __vfs_read+0xc1/0x130
[  502.481399]        [<ffffffff81254af1>] vfs_read+0x91/0x140
[  502.481414]        [<ffffffff81255f49>] SyS_read+0x49/0xa0
[  502.481429]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481445] 
[  502.481445] other info that might help us debug this:
[  502.481445] 

Quoted 8 lines

[  502.481459]  Possible unsafe locking scenario:
[  502.481459] 
[  502.481726]        CPU0                    CPU1
[  502.481987]        ----                    ----
[  502.482242]   lock(&fi->dio_rwsem);
[  502.482501]                                lock(&sb->s_type->i_mutex_key#18);
[  502.482765]                                lock(&fi->dio_rwsem);
[  502.483025]   lock(&sb->s_type->i_mutex_key#18);

Seems we will suffer ABBA deadlock:

writer reader
- f2fs_file_write_iter
- down_write(&inode->i_rwsem)
- __generic_file_write_iter
- generic_file_direct_write
- f2fs_direct_IO
- generic_file_read_iter
- f2fs_direct_IO
- down_read(&fi->dio_rwsem)
- __blockdev_direct_IO
- do_blockdev_direct_IO
- down_write(&inode->i_rwsem)

Quoted 4 lines

- down_read(&fi->dio_rwsem)

What about splitting dio_rwsem to rdio_rwsem/wdio_rwsem for reader/writer to avoid deadlock?

Hmm, how about inode_trylock in GC?

Quoted 145 lines

Thanks,

Quoted 141 lines

[  502.483285] 
[  502.483285]  *** DEADLOCK ***
[  502.483285] 
[  502.484018] 1 lock held by fsstress/10729:
[  502.484262]  #0:  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]

Thanks,

Quoted 132 lines · Chao Yu

From: Chao Yu <[email protected]>

Datas in file can be operated by GC and DIO simultaneously, so we will face race case as below:

For write case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- dio_bio_submit
update user data to old block address

For read case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_balance_fs
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- write_checkpoint
- do_checkpoint
- clear_prefree_segments
- f2fs_issue_discard discard old block adress
- dio_bio_submit
update user buffer from obsolete block address

In order to fix this, for one file, we should let DIO and GC getting exclusion against with each other.

Signed-off-by Chao Yu <[email protected]>

---
v3: use semaphore to avoid racing in between read dio and write dio.

 fs/f2fs/data.c  |  4 ++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    | 13 +++++++++++++
 fs/f2fs/super.c |  1 +
 4 files changed, 19 insertions(+)

fs/f2fs/data.c +4 -0

index b6fd5bd..19197bb 100644
@@ -1712,6 +1712,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_inode_info *fi = F2FS_I(inode);
 	size_t count = iov_iter_count(iter);
 	loff_t offset = iocb->ki_pos;
 	int err;

@@ -1727,7 +1728,10 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
+	down_read(&fi->dio_rwsem);
 	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
+	up_read(&fi->dio_rwsem);
+
 	if (iov_iter_rw(iter) == WRITE) {
 		if (err > 0)
 			set_inode_flag(inode, FI_UPDATE_WRITE);

fs/f2fs/f2fs.h +1 -0

index bf9a13a..2e439ec 100644
@@ -474,6 +474,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
+	struct rw_semaphore dio_rwsem;	/* avoid racing between dio and gc */
 };
 
 static inline void get_extent_info(struct extent_info *ext,

fs/f2fs/gc.c +13 -0

index c612137..a9bfb8d 100644
@@ -755,12 +755,25 @@ next_step:
 		/* phase 3 */
 		inode = find_gc_inode(gc_list, dni.ino);
 		if (inode) {
+			struct f2fs_inode_info *fi = F2FS_I(inode);
+			bool locked = false;
+
+			if (S_ISREG(inode->i_mode)) {
+				if (!down_write_trylock(&fi->dio_rwsem))
+					continue;
+				locked = true;
+			}
+
 			start_bidx = start_bidx_of_node(nofs, inode)
 								+ ofs_in_node;
 			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
 				move_encrypted_block(inode, start_bidx);
 			else
 				move_data_page(inode, start_bidx, gc_type);
+
+			if (locked)
+				up_write(&fi->dio_rwsem);
+
 			stat_inc_data_blk_count(sbi, 1, gc_type);
 		}
 	}

fs/f2fs/super.c +1 -0

index edd1b35..dde57fb 100644
@@ -579,6 +579,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	INIT_LIST_HEAD(&fi->inmem_pages);
 	mutex_init(&fi->inmem_lock);
+	init_rwsem(&fi->dio_rwsem);
 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

fs/f2fs/super.c +0 -0

 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;

Signature · 1 line

2.7.2

.

Quoted 79 lines · Jaegeuk Kim

Quoted 77 lines · Chao Yu

Hi Jaegeuk,

Quoted 46 lines · Jaegeuk Kim

Hi Chao,

Could you take a look at this in xfstests/generic/013?

[  502.480850] ======================================================
[  502.480864] [ INFO: possible circular locking dependency detected ]
[  502.480877] 4.7.0-rc1+ #124 Tainted: G           OE  
[  502.480886] -------------------------------------------------------
[  502.480897] fsstress/10729 is trying to acquire lock:
[  502.480906]  (&sb->s_type->i_mutex_key#18){+.+.+.}, at: [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.480948] 
[  502.480948] but task is already holding lock:
[  502.480959]  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481003] 
[  502.481003] which lock already depends on the new lock.
[  502.481003] 
[  502.481018] 
[  502.481018] the existing dependency chain (in reverse order) is:
[  502.481030] 
[  502.481030] -> #1 (&fi->dio_rwsem){.+.+.+}:
[  502.481054]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481071]        [<ffffffff818d1921>] down_read+0x51/0xa0
[  502.481089]        [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]
[  502.481114]        [<ffffffff811c34c7>] generic_file_direct_write+0xa7/0x160
[  502.481133]        [<ffffffff811c363d>] __generic_file_write_iter+0xbd/0x1e0
[  502.481149]        [<ffffffffc080437b>] f2fs_file_write_iter+0xdb/0x100 [f2fs]
[  502.481173]        [<ffffffff81253a88>] __vfs_write+0xc8/0x140
[  502.481190]        [<ffffffff81254c55>] vfs_write+0xb5/0x1b0
[  502.481205]        [<ffffffff81255fe9>] SyS_write+0x49/0xa0
[  502.481220]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481236] 
[  502.481236] -> #0 (&sb->s_type->i_mutex_key#18){+.+.+.}:
[  502.481264]        [<ffffffff810e481c>] __lock_acquire+0x161c/0x1940
[  502.481280]        [<ffffffff810e51c3>] lock_acquire+0xd3/0x220
[  502.481296]        [<ffffffff818d1b9a>] down_write+0x5a/0xc0
[  502.481312]        [<ffffffff81299c3b>] do_blockdev_direct_IO+0x1db/0x2310
[  502.481328]        [<ffffffff8129bdaa>] __blockdev_direct_IO+0x3a/0x40
[  502.481344]        [<ffffffffc081e2e4>] f2fs_direct_IO+0x104/0x3d0 [f2fs]
[  502.481368]        [<ffffffff811c40a9>] generic_file_read_iter+0x689/0x7e0
[  502.481384]        [<ffffffff812545d1>] __vfs_read+0xc1/0x130
[  502.481399]        [<ffffffff81254af1>] vfs_read+0x91/0x140
[  502.481414]        [<ffffffff81255f49>] SyS_read+0x49/0xa0
[  502.481429]        [<ffffffff818d4100>] entry_SYSCALL_64_fastpath+0x23/0xc1
[  502.481445] 
[  502.481445] other info that might help us debug this:
[  502.481445] 

Quoted 8 lines

[  502.481459]  Possible unsafe locking scenario:
[  502.481459] 
[  502.481726]        CPU0                    CPU1
[  502.481987]        ----                    ----
[  502.482242]   lock(&fi->dio_rwsem);
[  502.482501]                                lock(&sb->s_type->i_mutex_key#18);
[  502.482765]                                lock(&fi->dio_rwsem);
[  502.483025]   lock(&sb->s_type->i_mutex_key#18);

Seems we will suffer ABBA deadlock:

writer reader
- f2fs_file_write_iter
- down_write(&inode->i_rwsem)
- __generic_file_write_iter
- generic_file_direct_write
- f2fs_direct_IO
- generic_file_read_iter
- f2fs_direct_IO
- down_read(&fi->dio_rwsem)
- __blockdev_direct_IO
- do_blockdev_direct_IO
- down_write(&inode->i_rwsem)

- down_read(&fi->dio_rwsem)

What about splitting dio_rwsem to rdio_rwsem/wdio_rwsem for reader/writer to avoid deadlock?

Quoted 1 line

Hmm, how about inode_trylock in GC?

If we reuse inode->i_rwsem here, we will suffer the same issue when we remove i_rwsem lock in dio writer or dio reader for better concurrency.

So I think it's better to use separate lock to just fix this issue.

Thanks,

Quoted 152 lines

Quoted 145 lines

Thanks,

Quoted 141 lines

[  502.483285] 
[  502.483285]  *** DEADLOCK ***
[  502.483285] 
[  502.484018] 1 lock held by fsstress/10729:
[  502.484262]  #0:  (&fi->dio_rwsem){.+.+.+}, at: [<ffffffffc081e2b1>] f2fs_direct_IO+0xd1/0x3d0 [f2fs]

Thanks,

Quoted 132 lines · Chao Yu

From: Chao Yu <[email protected]>

Datas in file can be operated by GC and DIO simultaneously, so we will face race case as below:

For write case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- dio_bio_submit
update user data to old block address

For read case:
Thread A Thread B
- generic_file_direct_write
- invalidate_inode_pages2_range
- f2fs_direct_IO
- do_blockdev_direct_IO
- do_direct_IO
- get_more_blocks
- f2fs_balance_fs
- f2fs_gc
- do_garbage_collect
- gc_data_segment
- move_data_page
- do_write_data_page migrate data block to new block address
- write_checkpoint
- do_checkpoint
- clear_prefree_segments
- f2fs_issue_discard discard old block adress
- dio_bio_submit
update user buffer from obsolete block address

In order to fix this, for one file, we should let DIO and GC getting exclusion against with each other.

Signed-off-by Chao Yu <[email protected]>

---
v3: use semaphore to avoid racing in between read dio and write dio.

 fs/f2fs/data.c  |  4 ++++
 fs/f2fs/f2fs.h  |  1 +
 fs/f2fs/gc.c    | 13 +++++++++++++
 fs/f2fs/super.c |  1 +
 4 files changed, 19 insertions(+)

fs/f2fs/data.c +4 -0

index b6fd5bd..19197bb 100644
@@ -1712,6 +1712,7 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct f2fs_inode_info *fi = F2FS_I(inode);
 	size_t count = iov_iter_count(iter);
 	loff_t offset = iocb->ki_pos;
 	int err;

@@ -1727,7 +1728,10 @@ static ssize_t f2fs_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 
 	trace_f2fs_direct_IO_enter(inode, offset, count, iov_iter_rw(iter));
 
+	down_read(&fi->dio_rwsem);
 	err = blockdev_direct_IO(iocb, inode, iter, get_data_block_dio);
+	up_read(&fi->dio_rwsem);
+
 	if (iov_iter_rw(iter) == WRITE) {
 		if (err > 0)
 			set_inode_flag(inode, FI_UPDATE_WRITE);

fs/f2fs/f2fs.h +1 -0

index bf9a13a..2e439ec 100644
@@ -474,6 +474,7 @@ struct f2fs_inode_info {
 	struct list_head inmem_pages;	/* inmemory pages managed by f2fs */
 	struct mutex inmem_lock;	/* lock for inmemory pages */
 	struct extent_tree *extent_tree;	/* cached extent_tree entry */
+	struct rw_semaphore dio_rwsem;	/* avoid racing between dio and gc */
 };
 
 static inline void get_extent_info(struct extent_info *ext,

fs/f2fs/gc.c +13 -0

index c612137..a9bfb8d 100644
@@ -755,12 +755,25 @@ next_step:
 		/* phase 3 */
 		inode = find_gc_inode(gc_list, dni.ino);
 		if (inode) {
+			struct f2fs_inode_info *fi = F2FS_I(inode);
+			bool locked = false;
+
+			if (S_ISREG(inode->i_mode)) {
+				if (!down_write_trylock(&fi->dio_rwsem))
+					continue;
+				locked = true;
+			}
+
 			start_bidx = start_bidx_of_node(nofs, inode)
 								+ ofs_in_node;
 			if (f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode))
 				move_encrypted_block(inode, start_bidx);
 			else
 				move_data_page(inode, start_bidx, gc_type);
+
+			if (locked)
+				up_write(&fi->dio_rwsem);
+
 			stat_inc_data_blk_count(sbi, 1, gc_type);
 		}
 	}

fs/f2fs/super.c +1 -0

index edd1b35..dde57fb 100644
@@ -579,6 +579,7 @@ static struct inode *f2fs_alloc_inode(struct super_block *sb)
 	INIT_LIST_HEAD(&fi->gdirty_list);
 	INIT_LIST_HEAD(&fi->inmem_pages);
 	mutex_init(&fi->inmem_lock);
+	init_rwsem(&fi->dio_rwsem);
 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

fs/f2fs/super.c +0 -0

 	/* Will be used by directory only */
 	fi->i_dir_level = F2FS_SB(sb)->dir_level;

Signature · 1 line

2.7.2

.