f2fs: propagate error given by f2fs_find_entry

PATCH In review Jaegeuk Kim 2 participants 6 messages May 27, 2016 – May 30, 2016

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Jaegeuk Kim

author maintainer

2016-05-27 00:25 UTC #0 lore raw

PATCH 1/4 f2fs: propagate error given by f2fs_find_entry

If we get ENOMEM or EIO in f2fs_find_entry, we should stop right away. Otherwise, for example, we can get duplicate directory entry by ->chash and ->clevel.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/dir.c    | 23 ++++++++++++++++-------
 fs/f2fs/inline.c |  4 +++-
 fs/f2fs/namei.c  |  5 +++++
 3 files changed, 24 insertions(+), 8 deletions(-)

fs/f2fs/dir.c +16 -7

index 24d1308..ae37543 100644
@@ -185,8 +185,13 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
 		/* no need to allocate new dentry pages to all the indices */
 		dentry_page = find_data_page(dir, bidx);
 		if (IS_ERR(dentry_page)) {
-			room = true;
-			continue;
+			if (PTR_ERR(dentry_page) == -ENOENT) {
+				room = true;
+				continue;
+			} else {
+				*res_page = dentry_page;
+				break;
+			}
 		}
 
 		de = find_in_block(dentry_page, fname, namehash, &max_slots,

@@ -223,19 +228,22 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	struct fscrypt_name fname;
 	int err;
 
-	*res_page = NULL;
-
 	err = fscrypt_setup_filename(dir, child, 1, &fname);
-	if (err)
+	if (err) {
+		*res_page = ERR_PTR(-ENOMEM);
 		return NULL;
+	}
 
 	if (f2fs_has_inline_dentry(dir)) {
+		*res_page = NULL;
 		de = find_in_inline_dir(dir, &fname, res_page);
 		goto out;
 	}
 
-	if (npages == 0)
+	if (npages == 0) {
+		*res_page = NULL;
 		goto out;
+	}
 
 	max_depth = F2FS_I(dir)->i_current_depth;
 	if (unlikely(max_depth > MAX_DIR_HASH_DEPTH)) {

@@ -247,8 +255,9 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	}
 
 	for (level = 0; level < max_depth; level++) {
+		*res_page = NULL;
 		de = find_in_level(dir, level, &fname, res_page);
-		if (de)
+		if (de || IS_ERR(*res_page))
 			break;
 	}
 out:

fs/f2fs/inline.c +3 -1

index 77c9c24..1eb3043 100644
@@ -286,8 +286,10 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode *dir,
 	f2fs_hash_t namehash;
 
 	ipage = get_node_page(sbi, dir->i_ino);
-	if (IS_ERR(ipage))
+	if (IS_ERR(ipage)) {
+		*res_page = ipage;
 		return NULL;
+	}
 
 	namehash = f2fs_dentry_hash(&name);

fs/f2fs/namei.c +5 -0

index 496f4e3..3f6119e 100644
@@ -232,6 +232,9 @@ static int __recover_dot_dentries(struct inode *dir, nid_t pino)
 	if (de) {
 		f2fs_dentry_kunmap(dir, page);
 		f2fs_put_page(page, 0);
+	} else if (IS_ERR(page)) {
+		err = PTR_ERR(page);
+		goto out;
 	} else {
 		err = __f2fs_add_link(dir, &dot, NULL, dir->i_ino, S_IFDIR);
 		if (err)

@@ -242,6 +245,8 @@ static int __recover_dot_dentries(struct inode *dir, nid_t pino)
 	if (de) {
 		f2fs_dentry_kunmap(dir, page);
 		f2fs_put_page(page, 0);
+	} else if (IS_ERR(page)) {
+		err = PTR_ERR(page);
 	} else {
 		err = __f2fs_add_link(dir, &dotdot, NULL, pino, S_IFDIR);
 	}

Signature · 1 line

2.6.3

Jaegeuk Kim

author maintainer

2016-05-27 00:25 UTC #1 lore raw

PATCH 2/4 f2fs: inject to produce some orphan inodes

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/f2fs.h  | 3 +++
 fs/f2fs/inode.c | 5 +++++
 fs/f2fs/super.c | 1 +
 3 files changed, 9 insertions(+)

fs/f2fs/f2fs.h +3 -0

index 5daba19..c4697b7 100644
@@ -45,6 +45,7 @@ enum {
 	FAULT_ORPHAN,
 	FAULT_BLOCK,
 	FAULT_DIR_DEPTH,
+	FAULT_EVICT_INODE,
 	FAULT_MAX,
 };

@@ -74,6 +75,8 @@ static inline bool time_to_inject(int type)
 		return false;
 	else if (type == FAULT_DIR_DEPTH && !IS_FAULT_SET(type))
 		return false;
+	else if (type == FAULT_EVICT_INODE && !IS_FAULT_SET(type))
+		return false;
 
 	atomic_inc(&f2fs_fault.inject_ops);
 	if (atomic_read(&f2fs_fault.inject_ops) >= f2fs_fault.inject_rate) {

fs/f2fs/inode.c +5 -0

index bdd814d..11cb60a 100644
@@ -345,6 +345,11 @@ void f2fs_evict_inode(struct inode *inode)
 	if (inode->i_nlink || is_bad_inode(inode))
 		goto no_delete;
 
+#ifdef CONFIG_F2FS_FAULT_INJECTION
+	if (time_to_inject(FAULT_EVICT_INODE))
+		goto no_delete;
+#endif
+
 	sb_start_intwrite(inode->i_sb);
 	set_inode_flag(inode, FI_NO_ALLOC);
 	i_size_write(inode, 0);

fs/f2fs/super.c +1 -0

index a5b5739..27f76819e 100644
@@ -49,6 +49,7 @@ char *fault_name[FAULT_MAX] = {
 	[FAULT_ORPHAN]		= "orphan",
 	[FAULT_BLOCK]		= "no more block",
 	[FAULT_DIR_DEPTH]	= "too big dir depth",
+	[FAULT_EVICT_INODE]	= "evict_inode fail",
 };
 
 static void f2fs_build_fault_attr(unsigned int rate)

Signature · 1 line

2.6.3

Jaegeuk Kim

author maintainer

2016-05-27 00:25 UTC #2 lore raw

PATCH 3/4 f2fs: do not skip writing data pages

In reply to Jaegeuk Kim

For data pages, let's try to flush as much as possible in background.

On /dev/pmem0,

1. dd if=/dev/zero of=/mnt/test/testfile bs=1M count=2048 conv=fsync Before : 800 MB/s
After : 1.1 GB/s

2. dd if=/dev/zero of=/mnt/test/testfile bs=1M count=2048 Before : 1.3 GB/s
After : 2.2 GB/s

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c    | 11 +++++------
 fs/f2fs/segment.h |  4 +---
 2 files changed, 6 insertions(+), 9 deletions(-)

fs/f2fs/data.c +5 -6

index 7132b02..85ceb2b 100644
@@ -1444,7 +1444,6 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 	struct inode *inode = mapping->host;
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	int ret;
-	long diff;
 
 	/* deal with chardevs and other special file */
 	if (!mapping->a_ops->writepage)

@@ -1469,14 +1468,14 @@ static int f2fs_write_data_pages(struct address_space *mapping,
 
 	trace_f2fs_writepages(mapping->host, wbc, DATA);
 
-	diff = nr_pages_to_write(sbi, DATA, wbc);
-
 	ret = f2fs_write_cache_pages(mapping, wbc, __f2fs_writepage, mapping);
-	f2fs_submit_merged_bio_cond(sbi, inode, NULL, 0, DATA, WRITE);
+	/*
+	 * if some pages were truncated, we cannot guarantee its mapping->host
+	 * to detect pending bios.
+	 */
+	f2fs_submit_merged_bio(sbi, DATA, WRITE);
 
 	remove_dirty_inode(inode);
-
-	wbc->nr_to_write = max((long)0, wbc->nr_to_write - diff);
 	return ret;
 
 skip_write:

fs/f2fs/segment.h +1 -3

index 5d016a1..890bb28d 100644
@@ -728,9 +728,7 @@ static inline long nr_pages_to_write(struct f2fs_sb_info *sbi, int type,
 
 	nr_to_write = wbc->nr_to_write;
 
-	if (type == DATA)
-		desired = 4096;
-	else if (type == NODE)
+	if (type == NODE)
 		desired = 3 * max_hw_blocks(sbi);
 	else
 		desired = MAX_BIO_BLOCKS(sbi);

Signature · 1 line

2.6.3

Jaegeuk Kim

author maintainer

2016-05-27 00:25 UTC #3 lore raw

PATCH 4/4 f2fs: remove two steps to flush dirty data pages

In reply to Jaegeuk Kim

If there is no cold page, we don't need to do a loop to flush dirty data pages.

On /dev/pmem0,

1. dd if=/dev/zero of=/mnt/test/testfile bs=1M count=2048 conv=fsync Before : 1.1 GB/s
After : 1.2 GB/s

2. dd if=/dev/zero of=/mnt/test/testfile bs=1M count=2048 Before : 2.2 GB/s
After : 2.3 GB/s

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/data.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

fs/f2fs/data.c +1 ~~-10~~

index 85ceb2b..5dcd8db 100644
@@ -1326,10 +1326,9 @@ static int f2fs_write_cache_pages(struct address_space *mapping,
 	int cycled;
 	int range_whole = 0;
 	int tag;
-	int step = 0;
 
 	pagevec_init(&pvec, 0);
-next:
+
 	if (wbc->range_cyclic) {
 		writeback_index = mapping->writeback_index; /* prev offset */
 		index = writeback_index;

@@ -1384,9 +1383,6 @@ continue_unlock:
 				goto continue_unlock;
 			}
 
-			if (step == is_cold_data(page))
-				goto continue_unlock;
-
 			if (PageWriteback(page)) {
 				if (wbc->sync_mode != WB_SYNC_NONE)
 					f2fs_wait_on_page_writeback(page,

@@ -1421,11 +1417,6 @@ continue_unlock:
 		cond_resched();
 	}
 
-	if (step < 1) {
-		step++;
-		goto next;
-	}
-
 	if (!cycled && !done) {
 		cycled = 1;
 		index = 0;

Signature · 1 line

2.6.3

He YunLei

2016-05-27 04:48 UTC #4 lore raw

In reply to Jaegeuk Kim

Quoted 59 lines · Jaegeuk Kim

If we get ENOMEM or EIO in f2fs_find_entry, we should stop right away. Otherwise, for example, we can get duplicate directory entry by ->chash and ->clevel.

Signed-off-by Jaegeuk Kim <[email protected]>

---
  fs/f2fs/dir.c    | 23 ++++++++++++++++-------
  fs/f2fs/inline.c |  4 +++-
  fs/f2fs/namei.c  |  5 +++++
  3 files changed, 24 insertions(+), 8 deletions(-)

fs/f2fs/dir.c +14 -6

index 24d1308..ae37543 100644
@@ -185,8 +185,13 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
  		/* no need to allocate new dentry pages to all the indices */
  		dentry_page = find_data_page(dir, bidx);
  		if (IS_ERR(dentry_page)) {
-			room = true;
-			continue;
+			if (PTR_ERR(dentry_page) == -ENOENT) {
+				room = true;
+				continue;
+			} else {
+				*res_page = dentry_page;
+				break;
+			}
  		}
 
  		de = find_in_block(dentry_page, fname, namehash, &max_slots,

@@ -223,19 +228,22 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
  	struct fscrypt_name fname;
  	int err;
 
-	*res_page = NULL;
-
  	err = fscrypt_setup_filename(dir, child, 1, &fname);
-	if (err)
+	if (err) {
+		*res_page = ERR_PTR(-ENOMEM);
  		return NULL;
+	}
 
  	if (f2fs_has_inline_dentry(dir)) {
+		*res_page = NULL;
  		de = find_in_inline_dir(dir, &fname, res_page);
  		goto out;
  	}
 
-	if (npages == 0)
+	if (npages == 0) {
+		*res_page = NULL;
  		goto out;
+	}
 
  	max_depth = F2FS_I(dir)->i_current_depth;
  	if (unlikely(max_depth > MAX_DIR_HASH_DEPTH)) {

fs/f2fs/dir.c struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir, · line 262

Quoted 9 lines

fs/f2fs/dir.c +2 -1

@@ -247,8 +255,9 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
  	}
 
  	for (level = 0; level < max_depth; level++) {
+		*res_page = NULL;
  		de = find_in_level(dir, level, &fname, res_page);
-		if (de)
+		if (de || IS_ERR(*res_page))
  			break;
  	}

Hi, kim
Here, we return NULL for the error of find_data_page, it means
the file looked up is not exist to vfs, but may be the file has already exist behind the block read error. So maybe we 'd better reported the error to vfs.

Thanks.

Quoted 41 lines

fs/f2fs/dir.c +0 -0

  out:

fs/f2fs/inline.c +3 -1

index 77c9c24..1eb3043 100644
@@ -286,8 +286,10 @@ struct f2fs_dir_entry *find_in_inline_dir(struct inode *dir,
  	f2fs_hash_t namehash;
 
  	ipage = get_node_page(sbi, dir->i_ino);
-	if (IS_ERR(ipage))
+	if (IS_ERR(ipage)) {
+		*res_page = ipage;
  		return NULL;
+	}
 
  	namehash = f2fs_dentry_hash(&name);

fs/f2fs/namei.c +5 -0

index 496f4e3..3f6119e 100644
@@ -232,6 +232,9 @@ static int __recover_dot_dentries(struct inode *dir, nid_t pino)
  	if (de) {
  		f2fs_dentry_kunmap(dir, page);
  		f2fs_put_page(page, 0);
+	} else if (IS_ERR(page)) {
+		err = PTR_ERR(page);
+		goto out;
  	} else {
  		err = __f2fs_add_link(dir, &dot, NULL, dir->i_ino, S_IFDIR);
  		if (err)

@@ -242,6 +245,8 @@ static int __recover_dot_dentries(struct inode *dir, nid_t pino)
  	if (de) {
  		f2fs_dentry_kunmap(dir, page);
  		f2fs_put_page(page, 0);
+	} else if (IS_ERR(page)) {
+		err = PTR_ERR(page);
  	} else {
  		err = __f2fs_add_link(dir, &dotdot, NULL, pino, S_IFDIR);
  	}

Jaegeuk Kim

author maintainer

2016-05-30 02:39 UTC #5 lore raw

Quoted 62 lines · He YunLei

Quoted 61 lines · Jaegeuk Kim

If we get ENOMEM or EIO in f2fs_find_entry, we should stop right away. Otherwise, for example, we can get duplicate directory entry by ->chash and ->clevel.

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/dir.c    | 23 ++++++++++++++++-------
 fs/f2fs/inline.c |  4 +++-
 fs/f2fs/namei.c  |  5 +++++
 3 files changed, 24 insertions(+), 8 deletions(-)

fs/f2fs/dir.c +14 -6

index 24d1308..ae37543 100644
@@ -185,8 +185,13 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
 		/* no need to allocate new dentry pages to all the indices */
 		dentry_page = find_data_page(dir, bidx);
 		if (IS_ERR(dentry_page)) {
-			room = true;
-			continue;
+			if (PTR_ERR(dentry_page) == -ENOENT) {
+				room = true;
+				continue;
+			} else {
+				*res_page = dentry_page;
+				break;
+			}
 		}
 
 		de = find_in_block(dentry_page, fname, namehash, &max_slots,

@@ -223,19 +228,22 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	struct fscrypt_name fname;
 	int err;
 
-	*res_page = NULL;
-
 	err = fscrypt_setup_filename(dir, child, 1, &fname);
-	if (err)
+	if (err) {
+		*res_page = ERR_PTR(-ENOMEM);
 		return NULL;
+	}
 
 	if (f2fs_has_inline_dentry(dir)) {
+		*res_page = NULL;
 		de = find_in_inline_dir(dir, &fname, res_page);
 		goto out;
 	}
 
-	if (npages == 0)
+	if (npages == 0) {
+		*res_page = NULL;
 		goto out;
+	}
 
 	max_depth = F2FS_I(dir)->i_current_depth;
 	if (unlikely(max_depth > MAX_DIR_HASH_DEPTH)) {

@@ -247,8 +255,9 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 	}

Quoted 13 lines

fs/f2fs/dir.c struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir, · line 262
Quoted 7 lines
fs/f2fs/dir.c +2 -1
 	for (level = 0; level < max_depth; level++) {
+		*res_page = NULL;
 		de = find_in_level(dir, level, &fname, res_page);
-		if (de)
+		if (de || IS_ERR(*res_page))
 			break;
 	}
Hi, kim
Here, we return NULL for the error of find_data_page, it means
the file looked up is not exist to vfs, but may be the file has already exist behind the block read error. So maybe we 'd better reported the error to vfs.

How about this? :)

Quoted 1 line

From a9098ceb560174debca8b5f06658d7a91cdd7fff Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Fri, 27 May 2016 10:10:41 -0700
Subject: [PATCH] f2fs: return error of f2fs_lookup

Now we can report an error to f2fs_lookup given by f2fs_find_entry.

Suggested-by He YunLei <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/dir.c   | 2 +-
 fs/f2fs/namei.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

fs/f2fs/dir.c +1 -1

index ae37543..6fbb1ed 100644
@@ -230,7 +230,7 @@ struct f2fs_dir_entry *f2fs_find_entry(struct inode *dir,
 
 	err = fscrypt_setup_filename(dir, child, 1, &fname);
 	if (err) {
-		*res_page = ERR_PTR(-ENOMEM);
+		*res_page = ERR_PTR(err);
 		return NULL;
 	}

fs/f2fs/namei.c +4 -1

index 3f6119e..78efe00 100644
@@ -287,8 +287,11 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	de = f2fs_find_entry(dir, &dentry->d_name, &page);
-	if (!de)
+	if (!de) {
+		if (IS_ERR(page))
+			return (struct dentry *)page;
 		return d_splice_alias(inode, dentry);
+	}
 
 	ino = le32_to_cpu(de->ino);
 	f2fs_dentry_kunmap(dir, page);

Signature · 1 line

2.6.3