f2fs: clear nlink if fail to add_link

PATCH In review Chao Yu 3 participants 12 messages Oct 11, 2016 – Oct 13, 2016 F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2016-10-11 14:59 UTC #0 lore raw

PATCH 1/8 f2fs: clear nlink if fail to add_link

From: Chao Yu <[email protected]>

We don't need to keep incomplete created inode in cache, so if we fail to add link into directory during new inode creation, it's better to set nlink of inode to zero, then we can evict inode immediately. Otherwise release of nid belong to inode will be delayed until inode cache is being shrunk, it may cause a seemingly endless loop while allocating free nids in time of testing generic/269 case of fstest suit.

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

---
 fs/f2fs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

fs/f2fs/inode.c +8 -0

index d736989..34ae03c 100644
@@ -384,6 +384,8 @@ retry:
 		f2fs_lock_op(sbi);
 		err = remove_inode_page(inode);
 		f2fs_unlock_op(sbi);
+		if (err == -ENOENT)
+			err = 0;
 	}
 
 	/* give more chances, if ENOMEM case */

@@ -424,6 +426,12 @@ void handle_failed_inode(struct inode *inode)
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
 
+	/*
+	 * clear nlink of inode in order to release resource of inode
+	 * immediately.
+	 */
+	clear_nlink(inode);
+
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #1 lore raw

PATCH 2/8 f2fs: fix to release discard entries during checkpoint

From: Chao Yu <[email protected]>

In f2fs_fill_super, if there is any IO error occurs during recovery, cached discard entries will be leaked, in order to avoid this, make write_checkpoint() handle memory release by itself, besides, move clear_prefree_segments to write_checkpoint for readability.

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

---
 fs/f2fs/checkpoint.c | 9 ++++++---
 fs/f2fs/super.c      | 1 -
 2 files changed, 6 insertions(+), 4 deletions(-)

fs/f2fs/checkpoint.c +6 -3

index eacc697..654f5d7 100644
@@ -1184,7 +1184,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	if (unlikely(f2fs_cp_error(sbi)))
 		return -EIO;
 
-	clear_prefree_segments(sbi, cpc);
 	clear_sbi_flag(sbi, SBI_IS_DIRTY);
 	clear_sbi_flag(sbi, SBI_NEED_CP);

@@ -1261,8 +1260,12 @@ int write_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 
 	/* unlock all the fs_lock[] in do_checkpoint() */
 	err = do_checkpoint(sbi, cpc);
-
-	f2fs_wait_all_discard_bio(sbi);
+	if (err) {
+		release_discard_addrs(sbi);
+	} else {
+		clear_prefree_segments(sbi, cpc);
+		f2fs_wait_all_discard_bio(sbi);
+	}
 
 	unblock_operations(sbi);
 	stat_inc_cp_count(sbi->stat_info);

fs/f2fs/super.c +0 -1

index 6132b4c..bfa4414 100644
@@ -738,7 +738,6 @@ static void f2fs_put_super(struct super_block *sb)
 	 * In addition, EIO will skip do checkpoint, we need this as well.
 	 */
 	release_ino_entry(sbi, true);
-	release_discard_addrs(sbi);
 
 	f2fs_leave_shrinker(sbi);
 	mutex_unlock(&sbi->umount_mutex);

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #2 lore raw

PATCH 3/8 f2fs: give a chance to detach from dirty list

In reply to Chao Yu

From: Chao Yu <[email protected]>

If there is no dirty pages in inode, we should give a chance to detach the inode from global dirty list, otherwise it needs to call another unnecessary .writepages for detaching.

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

---
 fs/f2fs/data.c    | 8 +++++---
 fs/f2fs/dir.c     | 1 +
 fs/f2fs/gc.c      | 4 +++-
 fs/f2fs/inline.c  | 4 +++-
 fs/f2fs/node.c    | 1 +
 fs/f2fs/segment.c | 4 +++-
 6 files changed, 16 insertions(+), 6 deletions(-)

fs/f2fs/data.c +5 -3

index 0d0177c..1818781 100644
@@ -1785,12 +1785,14 @@ void f2fs_invalidate_page(struct page *page, unsigned int offset,
 		return;
 
 	if (PageDirty(page)) {
-		if (inode->i_ino == F2FS_META_INO(sbi))
+		if (inode->i_ino == F2FS_META_INO(sbi)) {
 			dec_page_count(sbi, F2FS_DIRTY_META);
-		else if (inode->i_ino == F2FS_NODE_INO(sbi))
+		} else if (inode->i_ino == F2FS_NODE_INO(sbi)) {
 			dec_page_count(sbi, F2FS_DIRTY_NODES);
-		else
+		} else {
 			inode_dec_dirty_pages(inode);
+			remove_dirty_inode(inode);
+		}
 	}
 
 	/* This is atomic written page, keep Private */

fs/f2fs/dir.c +1 -0

index cbf85f6..d7c0eb3 100644
@@ -742,6 +742,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
 		ClearPagePrivate(page);
 		ClearPageUptodate(page);
 		inode_dec_dirty_pages(dir);
+		remove_dirty_inode(dir);
 	}
 	f2fs_put_page(page, 1);
 }

fs/f2fs/gc.c +3 -1

index 93985c6..e48142f 100644
@@ -673,8 +673,10 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 retry:
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
-		if (clear_page_dirty_for_io(page))
+		if (clear_page_dirty_for_io(page)) {
 			inode_dec_dirty_pages(inode);
+			remove_dirty_inode(inode);
+		}
 
 		set_cold_data(page);

fs/f2fs/inline.c +3 -1

index 34234d8..1cf7a74 100644
@@ -137,8 +137,10 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	fio.old_blkaddr = dn->data_blkaddr;
 	write_data_page(dn, &fio);
 	f2fs_wait_on_page_writeback(page, DATA, true);
-	if (dirty)
+	if (dirty) {
 		inode_dec_dirty_pages(dn->inode);
+		remove_dirty_inode(dn->inode);
+	}
 
 	/* this converted inline_data should be recovered. */
 	set_inode_flag(dn->inode, FI_APPEND_WRITE);

fs/f2fs/node.c +1 -0

index 0a9692e..3a393e3 100644
@@ -1204,6 +1204,7 @@ static void flush_inline_data(struct f2fs_sb_info *sbi, nid_t ino)
 
 	ret = f2fs_write_inline_data(inode, page);
 	inode_dec_dirty_pages(inode);
+	remove_dirty_inode(inode);
 	if (ret)
 		set_page_dirty(page);
 page_out:

fs/f2fs/segment.c +3 -1

index 7f62dd0..bc190cf 100644
@@ -274,8 +274,10 @@ static int __commit_inmem_pages(struct inode *inode,
 
 			set_page_dirty(page);
 			f2fs_wait_on_page_writeback(page, DATA, true);
-			if (clear_page_dirty_for_io(page))
+			if (clear_page_dirty_for_io(page)) {
 				inode_dec_dirty_pages(inode);
+				remove_dirty_inode(inode);
+			}
 
 			fio.page = page;
 			err = do_write_data_page(&fio);

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #3 lore raw

PATCH 4/8 f2fs: add missing f2fs_balance_fs in f2fs_zero_range

In reply to Chao Yu

From: Chao Yu <[email protected]>

f2fs_balance_fs should be called in between node page updating, otherwise node page count will exceeded far beyond watermark of triggering foreground garbage collection, result in facing high risk of hitting LFS allocation failure.

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

---
 fs/f2fs/file.c | 3 +++
 1 file changed, 3 insertions(+)

fs/f2fs/file.c +3 -0

index 8f48fdd..0081c79 100644
@@ -1260,6 +1260,9 @@ static int f2fs_zero_range(struct inode *inode, loff_t offset, loff_t len,
 			ret = f2fs_do_zero_range(&dn, index, end);
 			f2fs_put_dnode(&dn);
 			f2fs_unlock_op(sbi);
+
+			f2fs_balance_fs(sbi, dn.node_changed);
+
 			if (ret)
 				goto out;

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #4 lore raw

PATCH 5/8 f2fs: don't miss any f2fs_balance_fs cases

In reply to Chao Yu

From: Chao Yu <[email protected]>

In f2fs_map_blocks, let f2fs_balance_fs detects node page modification with dn.node_changed to avoid miss some corner cases.

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

---
 fs/f2fs/data.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

fs/f2fs/data.c +3 -8

index 1818781..edfd2de 100644
@@ -676,7 +676,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map,
 	unsigned int ofs_in_node, last_ofs_in_node;
 	blkcnt_t prealloc;
 	struct extent_info ei;
-	bool allocated = false;
 	block_t blkaddr;
 
 	if (!maxblocks)

@@ -735,10 +734,8 @@ next_block:
 				}
 			} else {
 				err = __allocate_data_block(&dn);
-				if (!err) {
+				if (!err)
 					set_inode_flag(inode, FI_APPEND_WRITE);
-					allocated = true;
-				}
 			}
 			if (err)
 				goto sync_out;

@@ -793,7 +790,6 @@ skip:
 		err = reserve_new_blocks(&dn, prealloc);
 		if (err)
 			goto sync_out;
-		allocated = dn.node_changed;
 
 		map->m_len += dn.ofs_in_node - ofs_in_node;
 		if (prealloc && dn.ofs_in_node != last_ofs_in_node + 1) {

@@ -812,9 +808,8 @@ skip:
 
 	if (create) {
 		f2fs_unlock_op(sbi);
-		f2fs_balance_fs(sbi, allocated);
+		f2fs_balance_fs(sbi, dn.node_changed);
 	}
-	allocated = false;
 	goto next_dnode;
 
 sync_out:

@@ -822,7 +817,7 @@ sync_out:
 unlock_out:
 	if (create) {
 		f2fs_unlock_op(sbi);
-		f2fs_balance_fs(sbi, allocated);
+		f2fs_balance_fs(sbi, dn.node_changed);
 	}
 out:
 	trace_f2fs_map_blocks(inode, map, err);

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #5 lore raw

PATCH 6/8 f2fs: be aware of extent beyond EOF in fiemap

In reply to Chao Yu

From: Chao Yu <[email protected]>

f2fs can support fallocating blocks beyond file size without changing the size, but ->fiemap of f2fs was restricted and can't detect these extents fallocated past EOF, now relieve the restriction.

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

---
 fs/f2fs/data.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

fs/f2fs/data.c +4 ~~-14~~

index edfd2de..983aa54e 100644
@@ -886,7 +886,6 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 	struct buffer_head map_bh;
 	sector_t start_blk, last_blk;
 	pgoff_t next_pgofs;
-	loff_t isize;
 	u64 logical = 0, phys = 0, size = 0;
 	u32 flags = 0;
 	int ret = 0;

@@ -903,13 +902,6 @@ int f2fs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 
 	inode_lock(inode);
 
-	isize = i_size_read(inode);
-	if (start >= isize)
-		goto out;
-
-	if (start + len > isize)
-		len = isize - start;
-
 	if (logical_to_blk(inode, len) == 0)
 		len = blk_to_logical(inode, 1);

@@ -928,13 +920,11 @@ next:
 	/* HOLE */
 	if (!buffer_mapped(&map_bh)) {
 		start_blk = next_pgofs;
-		/* Go through holes util pass the EOF */
-		if (blk_to_logical(inode, start_blk) < isize)
+
+		if (blk_to_logical(inode, start_blk) < blk_to_logical(inode,
+					F2FS_I_SB(inode)->max_file_blocks))
 			goto prep_next;
-		/* Found a hole beyond isize means no more extents.
-		 * Note that the premise is that filesystems don't
-		 * punch holes beyond isize and keep size unchanged.
-		 */
+
 		flags |= FIEMAP_EXTENT_LAST;
 	}

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #6 lore raw

PATCH 7/8 f2fs: fix to update largest extent under lock

In reply to Chao Yu

From: Chao Yu <[email protected]>

In order to avoid racing problem, make largest extent cache being updated under lock.

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

---
 fs/f2fs/inode.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

fs/f2fs/inode.c +7 -4

index 34ae03c..d9cd458 100644
@@ -252,6 +252,7 @@ retry:
 int update_inode(struct inode *inode, struct page *node_page)
 {
 	struct f2fs_inode *ri;
+	struct extent_tree *et = F2FS_I(inode)->extent_tree;
 
 	f2fs_inode_synced(inode);

@@ -267,11 +268,13 @@ int update_inode(struct inode *inode, struct page *node_page)
 	ri->i_size = cpu_to_le64(i_size_read(inode));
 	ri->i_blocks = cpu_to_le64(inode->i_blocks);
 
-	if (F2FS_I(inode)->extent_tree)
-		set_raw_extent(&F2FS_I(inode)->extent_tree->largest,
-							&ri->i_ext);
-	else
+	if (et) {
+		read_lock(&et->lock);
+		set_raw_extent(&et->largest, &ri->i_ext);
+		read_unlock(&et->lock);
+	} else {
 		memset(&ri->i_ext, 0, sizeof(ri->i_ext));
+	}
 	set_raw_inline(inode, ri);
 
 	ri->i_atime = cpu_to_le64(inode->i_atime.tv_sec);

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-11 14:59 UTC #7 lore raw

PATCH 8/8 f2fs: fix error handling in fsync_node_pages

In reply to Chao Yu

From: Chao Yu <[email protected]>

In fsync_node_pages, if f2fs was taged with CP_ERROR_FLAG, make sure bio cache was flushed before return.

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

---
 fs/f2fs/node.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

fs/f2fs/node.c +3 -2

index 3a393e3..14f137d 100644
@@ -1339,7 +1339,8 @@ retry:
 			if (unlikely(f2fs_cp_error(sbi))) {
 				f2fs_put_page(last_page, 0);
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			if (!IS_DNODE(page) || !is_cold_node(page))

@@ -1412,7 +1413,7 @@ continue_unlock:
 		unlock_page(last_page);
 		goto retry;
 	}
-
+out:
 	if (nwritten)
 		f2fs_submit_merged_bio_cond(sbi, NULL, NULL, ino, NODE, WRITE);
 	return ret ? -EIO: 0;

Signature · 6 lines

2.10.1


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Jaegeuk Kim

maintainer

2016-10-11 22:19 UTC #8 lore raw

In reply to Chao Yu

Hi Chao,

Quoted 28 lines · Chao Yu

From: Chao Yu <[email protected]>
We don't need to keep incomplete created inode in cache, so if we fail to add link into directory during new inode creation, it's better to set nlink of inode to zero, then we can evict inode immediately. Otherwise release of nid belong to inode will be delayed until inode cache is being shrunk, it may cause a seemingly endless loop while allocating free nids in time of testing generic/269 case of fstest suit.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/f2fs/inode.c +2 -0
index d736989..34ae03c 100644
@@ -384,6 +384,8 @@ retry:
 		f2fs_lock_op(sbi);
 		err = remove_inode_page(inode);
 		f2fs_unlock_op(sbi);
+		if (err == -ENOENT)
+			err = 0;
 	}
 
 	/* give more chances, if ENOMEM case */

fs/f2fs/inode.c void handle_failed_inode(struct inode *inode) · line 433

Quoted 8 lines

fs/f2fs/inode.c +5 -0

@@ -424,6 +426,12 @@ void handle_failed_inode(struct inode *inode)
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
 
+	/*
+	 * clear nlink of inode in order to release resource of inode
+	 * immediately.
+	 */
+	clear_nlink(inode);

We must call update_inode_page() here to avoid kernel panic. Otherwise, this inode is kept in the gdirty list, resulting in kernel panic when flushg dirty inodes, since it was already evicted.

I fixed this and started a round of tests.

Thanks,

Quoted 6 lines

fs/f2fs/inode.c +1 -0

+
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);

Signature · 1 line

2.10.1

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Chao Yu

author maintainer

2016-10-12 15:24 UTC #9 lore raw

Hi Jaegeuk,

Quoted 34 lines · Jaegeuk Kim

Hi Chao,
Quoted 31 lines · Chao Yu
From: Chao Yu <[email protected]>
We don't need to keep incomplete created inode in cache, so if we fail to add link into directory during new inode creation, it's better to set nlink of inode to zero, then we can evict inode immediately. Otherwise release of nid belong to inode will be delayed until inode cache is being shrunk, it may cause a seemingly endless loop while allocating free nids in time of testing generic/269 case of fstest suit.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/f2fs/inode.c +2 -0
index d736989..34ae03c 100644
@@ -384,6 +384,8 @@ retry:
 		f2fs_lock_op(sbi);
 		err = remove_inode_page(inode);
 		f2fs_unlock_op(sbi);
+		if (err == -ENOENT)
+			err = 0;
 	}
 
 	/* give more chances, if ENOMEM case */
@@ -424,6 +426,12 @@ void handle_failed_inode(struct inode *inode)
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
 

Quoted 9 lines

fs/f2fs/inode.c void handle_failed_inode(struct inode *inode) · line 433
Quoted 5 lines
fs/f2fs/inode.c +5 -0
+	/*
+	 * clear nlink of inode in order to release resource of inode
+	 * immediately.
+	 */
+	clear_nlink(inode);
We must call update_inode_page() here to avoid kernel panic. Otherwise, this inode is kept in the gdirty list, resulting in kernel panic when flushg dirty inodes, since it was already evicted.

Thanks for fixing this, is this panic produced with fault injection?

Thanks,

Quoted 11 lines

I fixed this and started a round of tests.
Thanks,
Quoted 6 lines
fs/f2fs/inode.c +1 -0
+
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);
 
Signature · 1 line
2.10.1

Jaegeuk Kim

maintainer

2016-10-12 17:22 UTC #10 lore raw

Quoted 47 lines · Chao Yu

Hi Jaegeuk,
Quoted 34 lines · Jaegeuk Kim
Hi Chao,
Quoted 31 lines · Chao Yu
From: Chao Yu <[email protected]>
We don't need to keep incomplete created inode in cache, so if we fail to add link into directory during new inode creation, it's better to set nlink of inode to zero, then we can evict inode immediately. Otherwise release of nid belong to inode will be delayed until inode cache is being shrunk, it may cause a seemingly endless loop while allocating free nids in time of testing generic/269 case of fstest suit.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/inode.c | 8 ++++++++
 1 file changed, 8 insertions(+)
fs/f2fs/inode.c +2 -0
index d736989..34ae03c 100644
@@ -384,6 +384,8 @@ retry:
 		f2fs_lock_op(sbi);
 		err = remove_inode_page(inode);
 		f2fs_unlock_op(sbi);
+		if (err == -ENOENT)
+			err = 0;
 	}
 
 	/* give more chances, if ENOMEM case */
@@ -424,6 +426,12 @@ void handle_failed_inode(struct inode *inode)
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	struct node_info ni;
 
Quoted 9 lines
fs/f2fs/inode.c void handle_failed_inode(struct inode *inode) · line 433
Quoted 5 lines
fs/f2fs/inode.c +5 -0
+	/*
+	 * clear nlink of inode in order to release resource of inode
+	 * immediately.
+	 */
+	clear_nlink(inode);
We must call update_inode_page() here to avoid kernel panic. Otherwise, this inode is kept in the gdirty list, resulting in kernel panic when flushg dirty inodes, since it was already evicted.

Quoted 1 line

Thanks for fixing this, is this panic produced with fault injection?

No, it happened xfstests/027 under encrypted mode only.

Thanks,

Quoted 14 lines

Thanks,
Quoted 11 lines
I fixed this and started a round of tests.
Thanks,
Quoted 6 lines
fs/f2fs/inode.c +1 -0
+
 	/* don't make bad inode, since it becomes a regular file. */
 	unlock_new_inode(inode);
 
Signature · 1 line
2.10.1

Chao Yu

2016-10-13 10:31 UTC #11 lore raw

Quoted 1 line · Jaegeuk Kim

No, it happened xfstests/027 under encrypted mode only.

Got it. :)

Thanks,