f2fs: check the page status filled from disk

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/3] f2fs: cover more area with nat_tree_lock

There was a subtle bug on nat cache management which incurs wrong nid allocation or wrong block addresses when try_to_free_nats is triggered heavily. This patch enlarges the previous coverage of nat_tree_lock to avoid data race.

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

---
 fs/f2fs/node.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

fs/f2fs/node.c +2 -2

index 669c44e..4dab09f 100644
@@ -262,13 +262,11 @@ static void cache_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid,
 {
 	struct nat_entry *e;
 
-	down_write(&nm_i->nat_tree_lock);
 	e = __lookup_nat_cache(nm_i, nid);
 	if (!e) {
 		e = grab_nat_entry(nm_i, nid);
 		node_info_from_raw_nat(&e->ni, ne);
 	}
-	up_write(&nm_i->nat_tree_lock);
 }
 
 static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,

@@ -380,6 +378,8 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 
 	memset(&ne, 0, sizeof(struct f2fs_nat_entry));
 
+	down_write(&nm_i->nat_tree_lock);
+
 	/* Check current segment summary */
 	mutex_lock(&curseg->curseg_mutex);

fs/f2fs/node.c +1 -0

 	i = lookup_journal_in_cursum(sum, NAT_JOURNAL, nid, 0);

@@ -400,6 +400,7 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 cache:
 	/* cache nat entry */
 	cache_nat_entry(NM_I(sbi), nid, &ne);
+	up_write(&nm_i->nat_tree_lock);
 }
 

/*

fs/f2fs/node.c +9 -16

@@ -1459,13 +1460,10 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
 
 	if (build) {
 		/* do not add allocated nids */
-		down_read(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
-		if (ne &&
-			(!get_nat_flag(ne, IS_CHECKPOINTED) ||
+		if (ne && (!get_nat_flag(ne, IS_CHECKPOINTED) ||
 				nat_get_blkaddr(ne) != NULL_ADDR))
 			allocated = true;
-		up_read(&nm_i->nat_tree_lock);
 		if (allocated)
 			return 0;
 	}

@@ -1551,6 +1549,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES,
 							META_NAT, true);
 
+	down_read(&nm_i->nat_tree_lock);
+
 	while (1) {
 		struct page *page = get_current_nat_page(sbi, nid);
 

@@ -1579,6 +1579,7 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 			remove_free_nid(nm_i, nid);
 	}
 	mutex_unlock(&curseg->curseg_mutex);
+	up_read(&nm_i->nat_tree_lock);
 
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nm_i->next_scan_nid),
 					nm_i->ra_nid_pages, META_NAT, false);

@@ -1861,14 +1862,12 @@ static void remove_nats_in_journal(struct f2fs_sb_info *sbi)
 
 		raw_ne = nat_in_journal(sum, i);
 
-		down_write(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
 		if (!ne) {
 			ne = grab_nat_entry(nm_i, nid);
 			node_info_from_raw_nat(&ne->ni, &raw_ne);
 		}
 		__set_nat_cache_dirty(nm_i, ne);
-		up_write(&nm_i->nat_tree_lock);
 	}
 	update_nats_in_cursum(sum, -i);
 	mutex_unlock(&curseg->curseg_mutex);

@@ -1902,7 +1901,6 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 	struct f2fs_nat_block *nat_blk;
 	struct nat_entry *ne, *cur;
 	struct page *page = NULL;
-	struct f2fs_nm_info *nm_i = NM_I(sbi);
 
 	/*
 	 * there are two steps to flush nat entries:

@@ -1939,12 +1937,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 			raw_ne = &nat_blk->entries[nid - start_nid];
 		}
 		raw_nat_from_node_info(raw_ne, &ne->ni);
-
-		down_write(&NM_I(sbi)->nat_tree_lock);
 		nat_reset_flag(ne);
 		__clear_nat_cache_dirty(NM_I(sbi), ne);
-		up_write(&NM_I(sbi)->nat_tree_lock);
-
 		if (nat_get_blkaddr(ne) == NULL_ADDR)
 			add_free_nid(sbi, nid, false);
 	}

@@ -1956,9 +1950,7 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 
 	f2fs_bug_on(sbi, set->entry_cnt);
 
-	down_write(&nm_i->nat_tree_lock);
 	radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
-	up_write(&nm_i->nat_tree_lock);
 	kmem_cache_free(nat_entry_set_slab, set);
 }
 

@@ -1978,6 +1970,9 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 
 	if (!nm_i->dirty_nat_cnt)
 		return;
+
+	down_write(&nm_i->nat_tree_lock);
+
 	/*
 	 * if there are no enough space in journal to store dirty nat
 	 * entries, remove all entries from journal and merge them

@@ -1986,7 +1981,6 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 	if (!__has_cursum_space(sum, nm_i->dirty_nat_cnt, NAT_JOURNAL))
 		remove_nats_in_journal(sbi);
 
-	down_write(&nm_i->nat_tree_lock);
 	while ((found = __gang_lookup_nat_set(nm_i,
 					set_idx, SETVEC_SIZE, setvec))) {
 		unsigned idx;

@@ -1995,12 +1989,13 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 			__adjust_nat_entry_set(setvec[idx], &sets,
 							MAX_NAT_JENTRIES(sum));
 	}
-	up_write(&nm_i->nat_tree_lock);
 
 	/* flush dirty nats in nat entry set */
 	list_for_each_entry_safe(set, tmp, &sets, set_list)
 		__flush_nat_entry_set(sbi, set);
 
+	up_write(&nm_i->nat_tree_lock);
+
 	f2fs_bug_on(sbi, nm_i->dirty_nat_cnt);
 }
 
--

2.6.3

------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Hi Jaegeuk,

Quoted 8 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/3] f2fs: cover more area with nat_tree_lock

Quoted 3 lines

There was a subtle bug on nat cache management which incurs wrong nid allocation or wrong block addresses when try_to_free_nats is triggered heavily. This patch enlarges the previous coverage of nat_tree_lock to avoid data race.

Have you figured out how this happen? I'm curious about this issue, since still I can't reproduce it and find any clue by reviewing code so far.

Thanks,

Quoted 163 lines

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

---
 fs/f2fs/node.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

fs/f2fs/node.c +2 -2

index 669c44e..4dab09f 100644
@@ -262,13 +262,11 @@ static void cache_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid,
 {
 	struct nat_entry *e;
 
-	down_write(&nm_i->nat_tree_lock);
 	e = __lookup_nat_cache(nm_i, nid);
 	if (!e) {
 		e = grab_nat_entry(nm_i, nid);
 		node_info_from_raw_nat(&e->ni, ne);
 	}
-	up_write(&nm_i->nat_tree_lock);
 }
 
 static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,

@@ -380,6 +378,8 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 
 	memset(&ne, 0, sizeof(struct f2fs_nat_entry));
 
+	down_write(&nm_i->nat_tree_lock);
+
 	/* Check current segment summary */
 	mutex_lock(&curseg->curseg_mutex);

fs/f2fs/node.c +1 -0

 	i = lookup_journal_in_cursum(sum, NAT_JOURNAL, nid, 0);

@@ -400,6 +400,7 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 cache:
 	/* cache nat entry */
 	cache_nat_entry(NM_I(sbi), nid, &ne);
+	up_write(&nm_i->nat_tree_lock);
 }
 

/*

fs/f2fs/node.c +9 -16

@@ -1459,13 +1460,10 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
 
 	if (build) {
 		/* do not add allocated nids */
-		down_read(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
-		if (ne &&
-			(!get_nat_flag(ne, IS_CHECKPOINTED) ||
+		if (ne && (!get_nat_flag(ne, IS_CHECKPOINTED) ||
 				nat_get_blkaddr(ne) != NULL_ADDR))
 			allocated = true;
-		up_read(&nm_i->nat_tree_lock);
 		if (allocated)
 			return 0;
 	}

@@ -1551,6 +1549,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES,
 							META_NAT, true);
 
+	down_read(&nm_i->nat_tree_lock);
+
 	while (1) {
 		struct page *page = get_current_nat_page(sbi, nid);
 

@@ -1579,6 +1579,7 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 			remove_free_nid(nm_i, nid);
 	}
 	mutex_unlock(&curseg->curseg_mutex);
+	up_read(&nm_i->nat_tree_lock);
 
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nm_i->next_scan_nid),
 					nm_i->ra_nid_pages, META_NAT, false);

@@ -1861,14 +1862,12 @@ static void remove_nats_in_journal(struct f2fs_sb_info *sbi)
 
 		raw_ne = nat_in_journal(sum, i);
 
-		down_write(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
 		if (!ne) {
 			ne = grab_nat_entry(nm_i, nid);
 			node_info_from_raw_nat(&ne->ni, &raw_ne);
 		}
 		__set_nat_cache_dirty(nm_i, ne);
-		up_write(&nm_i->nat_tree_lock);
 	}
 	update_nats_in_cursum(sum, -i);
 	mutex_unlock(&curseg->curseg_mutex);

@@ -1902,7 +1901,6 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 	struct f2fs_nat_block *nat_blk;
 	struct nat_entry *ne, *cur;
 	struct page *page = NULL;
-	struct f2fs_nm_info *nm_i = NM_I(sbi);
 
 	/*
 	 * there are two steps to flush nat entries:

@@ -1939,12 +1937,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 			raw_ne = &nat_blk->entries[nid - start_nid];
 		}
 		raw_nat_from_node_info(raw_ne, &ne->ni);
-
-		down_write(&NM_I(sbi)->nat_tree_lock);
 		nat_reset_flag(ne);
 		__clear_nat_cache_dirty(NM_I(sbi), ne);
-		up_write(&NM_I(sbi)->nat_tree_lock);
-
 		if (nat_get_blkaddr(ne) == NULL_ADDR)
 			add_free_nid(sbi, nid, false);
 	}

@@ -1956,9 +1950,7 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 
 	f2fs_bug_on(sbi, set->entry_cnt);
 
-	down_write(&nm_i->nat_tree_lock);
 	radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
-	up_write(&nm_i->nat_tree_lock);
 	kmem_cache_free(nat_entry_set_slab, set);
 }
 

@@ -1978,6 +1970,9 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 
 	if (!nm_i->dirty_nat_cnt)
 		return;
+
+	down_write(&nm_i->nat_tree_lock);
+
 	/*
 	 * if there are no enough space in journal to store dirty nat
 	 * entries, remove all entries from journal and merge them

@@ -1986,7 +1981,6 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 	if (!__has_cursum_space(sum, nm_i->dirty_nat_cnt, NAT_JOURNAL))
 		remove_nats_in_journal(sbi);
 
-	down_write(&nm_i->nat_tree_lock);
 	while ((found = __gang_lookup_nat_set(nm_i,
 					set_idx, SETVEC_SIZE, setvec))) {
 		unsigned idx;

@@ -1995,12 +1989,13 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 			__adjust_nat_entry_set(setvec[idx], &sets,
 							MAX_NAT_JENTRIES(sum));
 	}
-	up_write(&nm_i->nat_tree_lock);
 
 	/* flush dirty nats in nat entry set */
 	list_for_each_entry_safe(set, tmp, &sets, set_list)
 		__flush_nat_entry_set(sbi, set);
 
+	up_write(&nm_i->nat_tree_lock);
+
 	f2fs_bug_on(sbi, nm_i->dirty_nat_cnt);
 }
 
--

2.6.3

------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 1:58 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 2/3] f2fs: cover more area with nat_tree_lock

Hi Chao,

Quoted 10 lines · Chao Yu

Hi Jaegeuk,

Quoted 8 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 2/3] f2fs: cover more area with nat_tree_lock

Quoted 7 lines

Quoted 3 lines

There was a subtle bug on nat cache management which incurs wrong nid allocation or wrong block addresses when try_to_free_nats is triggered heavily. This patch enlarges the previous coverage of nat_tree_lock to avoid data race.

Have you figured out how this happen? I'm curious about this issue, since still I can't reproduce it and find any clue by reviewing code so far.

It's very very subtle bug. I got one panic after 2-days fsstress execution with flushing caches very frequently.
The possible scenario was to mix a lot of directory operations, f2fs's shrinking path, and fsyncing files.

And, the suspicious functions are
- try_to_free_nats,
- f2fs_find_entry->get_node_page->get_node_info->cache_nat_entry
- fsync->checkpoint->flush_nat_entries
- build_free_nids

I guess there is somewhat data race to grab and release nat cache entries when flush_nat_entries is doing, since f2fs_find_entry is not covered by f2fs_lock_op.

A good thing is that, with this patch, I couldn't get any bug again for 4 days; still running tho.

Nevertheless, I couldn't describe this precisely, since I couldn't specify the real root-cause.

Thanks,

Quoted 45 lines

Thanks,

Quoted 42 lines

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

---
 fs/f2fs/node.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

fs/f2fs/node.c +2 -2

index 669c44e..4dab09f 100644
@@ -262,13 +262,11 @@ static void cache_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid,
 {
 	struct nat_entry *e;
 
-	down_write(&nm_i->nat_tree_lock);
 	e = __lookup_nat_cache(nm_i, nid);
 	if (!e) {
 		e = grab_nat_entry(nm_i, nid);
 		node_info_from_raw_nat(&e->ni, ne);
 	}
-	up_write(&nm_i->nat_tree_lock);
 }
 
 static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,

@@ -380,6 +378,8 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 
 	memset(&ne, 0, sizeof(struct f2fs_nat_entry));
 
+	down_write(&nm_i->nat_tree_lock);
+
 	/* Check current segment summary */
 	mutex_lock(&curseg->curseg_mutex);

fs/f2fs/node.c +1 -0

 	i = lookup_journal_in_cursum(sum, NAT_JOURNAL, nid, 0);

@@ -400,6 +400,7 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid, struct node_info
 *ni)
 cache:
 	/* cache nat entry */
 	cache_nat_entry(NM_I(sbi), nid, &ne);
+	up_write(&nm_i->nat_tree_lock);
 }
 

Quoted 2 lines

fs/f2fs/node.c static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool

Quoted 2 lines

/*

fs/f2fs/node.c +0 -0

@@ -1459,13 +1460,10 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool

build)

Quoted 119 lines

Quoted 119 lines

fs/f2fs/node.c +9 -16

 	if (build) {
 		/* do not add allocated nids */
-		down_read(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
-		if (ne &&
-			(!get_nat_flag(ne, IS_CHECKPOINTED) ||
+		if (ne && (!get_nat_flag(ne, IS_CHECKPOINTED) ||
 				nat_get_blkaddr(ne) != NULL_ADDR))
 			allocated = true;
-		up_read(&nm_i->nat_tree_lock);
 		if (allocated)
 			return 0;
 	}

@@ -1551,6 +1549,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES,
 							META_NAT, true);
 
+	down_read(&nm_i->nat_tree_lock);
+
 	while (1) {
 		struct page *page = get_current_nat_page(sbi, nid);
 

@@ -1579,6 +1579,7 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 			remove_free_nid(nm_i, nid);
 	}
 	mutex_unlock(&curseg->curseg_mutex);
+	up_read(&nm_i->nat_tree_lock);
 
 	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nm_i->next_scan_nid),
 					nm_i->ra_nid_pages, META_NAT, false);

@@ -1861,14 +1862,12 @@ static void remove_nats_in_journal(struct f2fs_sb_info *sbi)
 
 		raw_ne = nat_in_journal(sum, i);
 
-		down_write(&nm_i->nat_tree_lock);
 		ne = __lookup_nat_cache(nm_i, nid);
 		if (!ne) {
 			ne = grab_nat_entry(nm_i, nid);
 			node_info_from_raw_nat(&ne->ni, &raw_ne);
 		}
 		__set_nat_cache_dirty(nm_i, ne);
-		up_write(&nm_i->nat_tree_lock);
 	}
 	update_nats_in_cursum(sum, -i);
 	mutex_unlock(&curseg->curseg_mutex);

@@ -1902,7 +1901,6 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 	struct f2fs_nat_block *nat_blk;
 	struct nat_entry *ne, *cur;
 	struct page *page = NULL;
-	struct f2fs_nm_info *nm_i = NM_I(sbi);
 
 	/*
 	 * there are two steps to flush nat entries:

@@ -1939,12 +1937,8 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 			raw_ne = &nat_blk->entries[nid - start_nid];
 		}
 		raw_nat_from_node_info(raw_ne, &ne->ni);
-
-		down_write(&NM_I(sbi)->nat_tree_lock);
 		nat_reset_flag(ne);
 		__clear_nat_cache_dirty(NM_I(sbi), ne);
-		up_write(&NM_I(sbi)->nat_tree_lock);
-
 		if (nat_get_blkaddr(ne) == NULL_ADDR)
 			add_free_nid(sbi, nid, false);
 	}

@@ -1956,9 +1950,7 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 
 	f2fs_bug_on(sbi, set->entry_cnt);
 
-	down_write(&nm_i->nat_tree_lock);
 	radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
-	up_write(&nm_i->nat_tree_lock);
 	kmem_cache_free(nat_entry_set_slab, set);
 }
 

@@ -1978,6 +1970,9 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 
 	if (!nm_i->dirty_nat_cnt)
 		return;
+
+	down_write(&nm_i->nat_tree_lock);
+
 	/*
 	 * if there are no enough space in journal to store dirty nat
 	 * entries, remove all entries from journal and merge them

@@ -1986,7 +1981,6 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 	if (!__has_cursum_space(sum, nm_i->dirty_nat_cnt, NAT_JOURNAL))
 		remove_nats_in_journal(sbi);
 
-	down_write(&nm_i->nat_tree_lock);
 	while ((found = __gang_lookup_nat_set(nm_i,
 					set_idx, SETVEC_SIZE, setvec))) {
 		unsigned idx;

@@ -1995,12 +1989,13 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 			__adjust_nat_entry_set(setvec[idx], &sets,
 							MAX_NAT_JENTRIES(sum));
 	}
-	up_write(&nm_i->nat_tree_lock);
 
 	/* flush dirty nats in nat entry set */
 	list_for_each_entry_safe(set, tmp, &sets, set_list)
 		__flush_nat_entry_set(sbi, set);
 
+	up_write(&nm_i->nat_tree_lock);
+
 	f2fs_bug_on(sbi, nm_i->dirty_nat_cnt);
 }
 
--

2.6.3

------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +0 -0

index 89a978c..11b2111 100644

From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001

fs/f2fs/data.c +0 -1

 	}
 got_it:
 	if (new_i_size && i_size_read(inode) <
--

2.6.3

------------------------------------------------------------------------------
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Hi Jaegeuk,

Quoted 21 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +0 -0

index 89a978c..11b2111 100644
@@ -448,6 +448,14 @@ repeat:
 

fs/f2fs/data.c repeat: · line 453

Quoted 5 lines

fs/f2fs/data.c +3 -0

 		/* wait for read completion */
 		lock_page(page);
+		if (unlikely(!PageUptodate(page))) {
+			f2fs_put_page(page, 1);
+			return ERR_PTR(-EIO);

There is a convention in get_new_data_page, anyway we should release ipage if there is any error occurs, but I think it will be ok to return directly since it seems impossible the new dentry page has its real block address.

To avoid any bug here or wrong usage, how about add bug_on as following patch?

Quoted 1 line

From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Tue, 5 Jan 2016 15:28:56 +0800
Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page

In get_new_data_page, locked inode page should not be hold before get_read_data_page, this patch adds f2fs_bug_on to detect this condition.

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

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

fs/f2fs/data.c +2 -0

index 48f0bd3..2c5e3f6 100644
@@ -440,6 +440,8 @@ repeat:
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 		SetPageUptodate(page);
 	} else {
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
+
 		f2fs_put_page(page, 1);
 
 		page = get_read_data_page(inode, index, READ_SYNC, true);

Signature · 1 line

2.6.3

fs/f2fs/data.c

Quoted 5 lines

fs/f2fs/data.c +5 -0

+		}
+		if (unlikely(page->mapping != mapping)) {
+			f2fs_put_page(page, 1);
+			goto repeat;
+		}

How about use get_lock_data_page to avoid duplicated code?

From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 1:49 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

Hi Chao,

Quoted 23 lines · Chao Yu

Hi Jaegeuk,

Quoted 21 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +0 -0

index 89a978c..11b2111 100644
@@ -448,6 +448,14 @@ repeat:
 

Quoted 9 lines

fs/f2fs/data.c repeat: · line 453

Quoted 5 lines

fs/f2fs/data.c +3 -0

 		/* wait for read completion */
 		lock_page(page);
+		if (unlikely(!PageUptodate(page))) {
+			f2fs_put_page(page, 1);
+			return ERR_PTR(-EIO);

There is a convention in get_new_data_page, anyway we should release ipage if there is any error occurs, but I think it will be ok to return directly since it seems impossible the new dentry page has its real block address.

Makes sense, but definitely ipage should be put. :)

Quoted 35 lines

To avoid any bug here or wrong usage, how about add bug_on as following patch?

Quoted 1 line

From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Tue, 5 Jan 2016 15:28:56 +0800
Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page

In get_new_data_page, locked inode page should not be hold before get_read_data_page, this patch adds f2fs_bug_on to detect this condition.

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

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

fs/f2fs/data.c +2 -1

index 48f0bd3..2c5e3f6 100644
@@ -440,6 +440,8 @@ repeat:
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 		SetPageUptodate(page);
 	} else {
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
+
 		f2fs_put_page(page, 1);
 
 		page = get_read_data_page(inode, index, READ_SYNC, true);
--

2.6.3

Quoted 7 lines

fs/f2fs/data.c repeat: · line 454

Quoted 5 lines

fs/f2fs/data.c +5 -0

+		}
+		if (unlikely(page->mapping != mapping)) {
+			f2fs_put_page(page, 1);
+			goto repeat;
+		}

How about use get_lock_data_page to avoid duplicated code?

Agreed.

How about this?

From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 1 Jan 2016 22:03:47 -0800
Subject: [PATCH] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +1 -3

index 89a978c..89d633a 100644
@@ -442,12 +442,16 @@ repeat:
 	} else {
 		f2fs_put_page(page, 1);
 
-		page = get_read_data_page(inode, index, READ_SYNC, true);
-		if (IS_ERR(page))
-			goto repeat;
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);

fs/f2fs/data.c +1 -1

+		}
 	}
 got_it:
 	if (new_i_size && i_size_read(inode) <
--

2.6.3

Hi Jaegeuk,

Quoted 44 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 1:49 AM
To: Chao Yu
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

Hi Chao,

Quoted 23 lines · Chao Yu

Hi Jaegeuk,

Quoted 21 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Sunday, January 03, 2016 9:26 AM
To: [email protected]; [email protected]; [email protected]
Cc: Jaegeuk Kim
Subject: [f2fs-dev] [PATCH 1/3] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +0 -0

index 89a978c..11b2111 100644
@@ -448,6 +448,14 @@ repeat:
 

Quoted 9 lines

fs/f2fs/data.c repeat: · line 453

Quoted 5 lines

fs/f2fs/data.c +3 -0

 		/* wait for read completion */
 		lock_page(page);
+		if (unlikely(!PageUptodate(page))) {
+			f2fs_put_page(page, 1);
+			return ERR_PTR(-EIO);

There is a convention in get_new_data_page, anyway we should release ipage if there is any error occurs, but I think it will be ok to return directly since it seems impossible the new dentry page has its real block address.

Quoted 1 line

Makes sense, but definitely ipage should be put. :)

Alright. :)

Quoted 72 lines

Quoted 35 lines

To avoid any bug here or wrong usage, how about add bug_on as following patch?

Quoted 1 line

From d92f0f34493b27ef28da67c446d552ce721b5d6f Mon Sep 17 00:00:00 2001

From: Chao Yu <[email protected]>
Date: Tue, 5 Jan 2016 15:28:56 +0800
Subject: [PATCH] f2fs: add f2fs_bug_on in get_new_data_page

In get_new_data_page, locked inode page should not be hold before get_read_data_page, this patch adds f2fs_bug_on to detect this condition.

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

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

fs/f2fs/data.c +2 -1

index 48f0bd3..2c5e3f6 100644
@@ -440,6 +440,8 @@ repeat:
 		zero_user_segment(page, 0, PAGE_CACHE_SIZE);
 		SetPageUptodate(page);
 	} else {
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
+
 		f2fs_put_page(page, 1);
 
 		page = get_read_data_page(inode, index, READ_SYNC, true);
--

2.6.3

Quoted 7 lines

fs/f2fs/data.c repeat: · line 454

Quoted 5 lines

fs/f2fs/data.c +5 -0

+		}
+		if (unlikely(page->mapping != mapping)) {
+			f2fs_put_page(page, 1);
+			goto repeat;
+		}

How about use get_lock_data_page to avoid duplicated code?

Agreed.

How about this?

From fef77fb244a706491e8e4c46cb245e99e22003c3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Fri, 1 Jan 2016 22:03:47 -0800
Subject: [PATCH] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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

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

fs/f2fs/data.c +1 -3

index 89a978c..89d633a 100644
@@ -442,12 +442,16 @@ repeat:
 	} else {
 		f2fs_put_page(page, 1);
 
-		page = get_read_data_page(inode, index, READ_SYNC, true);
-		if (IS_ERR(page))
-			goto repeat;
+		f2fs_bug_on(F2FS_I_SB(inode), ipage);
 

fs/f2fs/data.c repeat: · line 453

Quoted 9 lines

fs/f2fs/data.c +7 -2

-		/* wait for read completion */
-		lock_page(page);
+		page = get_lock_data_page(inode, index, true);
+		if (IS_ERR(page)) {
+			if (PTR_ERR(page) == -EIO) {
+				f2fs_put_page(ipage, 1);
+				return page;
+			}
+			goto repeat;

Seems if get_lock_data_page always return -EFAULT, we may run into an infinite loop. IMO, it's not a bad thing to tolerate other error more than EIO returned from get_lock_data_page. How about return directly when error is returned? And add a bug_on for ENOENT which seems not impossible here?

Thanks,

Quoted 6 lines

fs/f2fs/data.c +1 -1

+		}
 	}
 got_it:
 	if (new_i_size && i_size_read(inode) <
--

2.6.3

From 4882f227e71b482c53e55a5c3ad559d33f620a20 Mon Sep 17 00:00:00 2001

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Wednesday, January 06, 2016 12:11 PM
To: [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH 1/3 v2] f2fs: check the page status filled from disk

Change log from v1:
- use get_lock_data_page and return error, suggested by Chao

Quoted 1 line

From 4882f227e71b482c53e55a5c3ad559d33f620a20 Mon Sep 17 00:00:00 2001

From: Jaegeuk Kim <[email protected]>
Date: Fri, 1 Jan 2016 22:03:47 -0800
Subject: [PATCH] f2fs: check the page status filled from disk

After reading a page, we need to check whether there is any error.

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