f2fs: don't use GFP_ZERO for page caches

From: Chao Yu <[email protected]>

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/inode.c    | 4 ++--
 fs/f2fs/node.c     | 6 ++++--
 fs/f2fs/node.h     | 7 ++-----
 fs/f2fs/recovery.c | 3 +--
 4 files changed, 9 insertions(+), 11 deletions(-)

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/node.c +4 -2

index 9a99243054ba..6fc3311820ec 100644
@@ -1096,7 +1096,8 @@ struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 
 	f2fs_wait_on_page_writeback(page, NODE, true);
-	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs);
 	set_cold_node(page, S_ISDIR(dn->inode->i_mode));
 	if (!PageUptodate(page))
 		SetPageUptodate(page);

@@ -2311,7 +2312,8 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 
 	if (!PageUptodate(ipage))
 		SetPageUptodate(ipage);
-	fill_node_footer(ipage, ino, ino, 0, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(ipage, ino, ino, 0);
 	set_cold_node(page, false);
 
 	src = F2FS_INODE(page);

fs/f2fs/node.h +2 -5

index b95e49e4a928..42cd081114ab 100644
@@ -263,15 +263,12 @@ static inline block_t next_blkaddr_of_node(struct page *node_page)
 }
 
 static inline void fill_node_footer(struct page *page, nid_t nid,
-				nid_t ino, unsigned int ofs, bool reset)
+				nid_t ino, unsigned int ofs)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
 	unsigned int old_flag = 0;
 
-	if (reset)
-		memset(rn, 0, sizeof(*rn));
-	else
-		old_flag = le32_to_cpu(rn->footer.flag);
+	old_flag = le32_to_cpu(rn->footer.flag);
 
 	rn->footer.nid = cpu_to_le32(nid);
 	rn->footer.ino = cpu_to_le32(ino);

fs/f2fs/recovery.c +1 -2

index 1b23d3febe4c..de24f3247aa5 100644
@@ -540,8 +540,7 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	}
 
 	copy_node_footer(dn.node_page, page);
-	fill_node_footer(dn.node_page, dn.nid, ni.ino,
-					ofs_of_node(page), false);
+	fill_node_footer(dn.node_page, dn.nid, ni.ino, ofs_of_node(page));
 	set_page_dirty(dn.node_page);
 err:
 	f2fs_put_dnode(&dn);

Signature · 1 line

2.15.0.531.g2ccb3012c9-goog

Change log from v1:
- don't memset for recovered page

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/inode.c | 4 ++--
 fs/f2fs/node.c  | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/node.c +2 -1

index 9a99243054ba..5a4469093e43 100644
@@ -1096,7 +1096,8 @@ struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 
 	f2fs_wait_on_page_writeback(page, NODE, true);
-	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, false);
 	set_cold_node(page, S_ISDIR(dn->inode->i_mode));
 	if (!PageUptodate(page))
 		SetPageUptodate(page);

Signature · 10 lines

2.15.0.531.g2ccb3012c9-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Quoted 10 lines · Jaegeuk Kim

Change log from v1:
- don't memset for recovered page

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

Quoted 4 lines

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Okay, it seems we're missing some more places to reset the memory, since I'm still getting a panic. :(

Thanks,

Quoted 51 lines

Cc <[email protected]>

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

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

---
 fs/f2fs/inode.c | 4 ++--
 fs/f2fs/node.c  | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/node.c +2 -1

index 9a99243054ba..5a4469093e43 100644
@@ -1096,7 +1096,8 @@ struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 
 	f2fs_wait_on_page_writeback(page, NODE, true);
-	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, false);
 	set_cold_node(page, S_ISDIR(dn->inode->i_mode));
 	if (!PageUptodate(page))
 		SetPageUptodate(page);

Signature · 10 lines

2.15.0.531.g2ccb3012c9-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

.

Change log from v2:
- consider IO error case when dealing with metapage
- memset by fill_node_footer

Change log from v1:
- don't memset for recovered page

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/checkpoint.c | 4 +++-
 fs/f2fs/inode.c      | 4 ++--
 fs/f2fs/segment.c    | 3 +++
 fs/f2fs/segment.h    | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

fs/f2fs/checkpoint.c +3 -1

index bf779461df13..2e23b953d304 100644
@@ -100,8 +100,10 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
 	 * readonly and make sure do not write checkpoint with non-uptodate
 	 * meta page.
 	 */
-	if (unlikely(!PageUptodate(page)))
+	if (unlikely(!PageUptodate(page))) {
+		memset(page_address(page), 0, PAGE_SIZE);
 		f2fs_stop_checkpoint(sbi, false);
+	}
 out:
 	return page;
 }

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/segment.c +0 -0

index a4b8e3e24ccb..1f5db557ab96 100644

fs/f2fs/segment.h +0 -0

 	for (i = 0; i < end - start; i++) {
 		rs = &raw_sit->entries[i];
 		se = get_seg_entry(sbi, start + i);

Quoted 69 lines · Jaegeuk Kim

Change log from v2:
- consider IO error case when dealing with metapage
- memset by fill_node_footer

Change log from v1:
- don't memset for recovered page

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/checkpoint.c | 4 +++-
 fs/f2fs/inode.c      | 4 ++--
 fs/f2fs/segment.c    | 3 +++
 fs/f2fs/segment.h    | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

fs/f2fs/checkpoint.c +3 -1

index bf779461df13..2e23b953d304 100644
@@ -100,8 +100,10 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
 	 * readonly and make sure do not write checkpoint with non-uptodate
 	 * meta page.
 	 */
-	if (unlikely(!PageUptodate(page)))
+	if (unlikely(!PageUptodate(page))) {
+		memset(page_address(page), 0, PAGE_SIZE);
 		f2fs_stop_checkpoint(sbi, false);
+	}
 out:
 	return page;
 }

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/segment.c +0 -0

index a4b8e3e24ccb..1f5db557ab96 100644
@@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
 	struct f2fs_summary_block *dst;
 

fs/f2fs/segment.c static void write_current_sum_page(struct f2fs_sb_info *sbi, · line 2024

Quoted 2 lines

fs/f2fs/segment.c +1 -0

 	dst = (struct f2fs_summary_block *)page_address(page);
+	memset(dst, 0, PAGE_SIZE);

We will fill all summary page later, so we don't need initialize it?

fs/f2fs/segment.c static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr) · line 3121

Quoted 9 lines

fs/f2fs/segment.c +1 -0

 	mutex_lock(&curseg->curseg_mutex);
 

@@ -3117,6 +3118,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
 
 	page = grab_meta_page(sbi, blkaddr++);
 	kaddr = (unsigned char *)page_address(page);
+	memset(kaddr, 0, PAGE_SIZE);

I checked here, we will not use uninitialized space due to:

read_compacted_summaries()
{
...

	if (offset + SUMMARY_SIZE <= PAGE_SIZE - SUM_FOOTER_SIZE)
		continue;

...
}

Are we safe here?

fs/f2fs/segment.c static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr) · line 3146

Quoted 8 lines

fs/f2fs/segment.c +1 -0

 	/* Step 1: write nat cache */
 	seg_i = CURSEG_I(sbi, CURSEG_HOT_DATA);

@@ -3141,6 +3143,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
 			if (!page) {
 				page = grab_meta_page(sbi, blkaddr++);
 				kaddr = (unsigned char *)page_address(page);
+				memset(kaddr, 0, PAGE_SIZE);

Ditto.

fs/f2fs/segment.h static inline void seg_info_to_sit_page(struct f2fs_sb_info *sbi, · line 378

Quoted 12 lines

fs/f2fs/segment.c +0 -0

 				written_size = 0;
 			}
 			summary = (struct f2fs_summary *)(kaddr + written_size);

fs/f2fs/segment.h +1 -0

index 3325d0769723..492ad0c86fa9 100644
@@ -375,6 +375,7 @@ static inline void seg_info_to_sit_page(struct f2fs_sb_info *sbi,
 	int i;
 
 	raw_sit = (struct f2fs_sit_block *)page_address(page);
+	memset(raw_sit, 0, PAGE_SIZE);

I suspect that root cause of panic is that we don't memset here?

Thanks,

Quoted 4 lines

fs/f2fs/segment.h +0 -0

 	for (i = 0; i < end - start; i++) {
 		rs = &raw_sit->entries[i];
 		se = get_seg_entry(sbi, start + i);

Quoted 70 lines · Chao Yu

Quoted 69 lines · Jaegeuk Kim

Change log from v2:
- consider IO error case when dealing with metapage
- memset by fill_node_footer

Change log from v1:
- don't memset for recovered page

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/checkpoint.c | 4 +++-
 fs/f2fs/inode.c      | 4 ++--
 fs/f2fs/segment.c    | 3 +++
 fs/f2fs/segment.h    | 1 +
 4 files changed, 9 insertions(+), 3 deletions(-)

fs/f2fs/checkpoint.c +3 -1

index bf779461df13..2e23b953d304 100644
@@ -100,8 +100,10 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
 	 * readonly and make sure do not write checkpoint with non-uptodate
 	 * meta page.
 	 */
-	if (unlikely(!PageUptodate(page)))
+	if (unlikely(!PageUptodate(page))) {
+		memset(page_address(page), 0, PAGE_SIZE);
 		f2fs_stop_checkpoint(sbi, false);
+	}
 out:
 	return page;
 }

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/segment.c +0 -0

index a4b8e3e24ccb..1f5db557ab96 100644
@@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
 	struct f2fs_summary_block *dst;
 

Quoted 4 lines

fs/f2fs/segment.c static void write_current_sum_page(struct f2fs_sb_info *sbi, · line 2024

Quoted 2 lines

fs/f2fs/segment.c +1 -0

 	dst = (struct f2fs_summary_block *)page_address(page);
+	memset(dst, 0, PAGE_SIZE);

We will fill all summary page later, so we don't need initialize it?

I'd like to reserve the uninitialzed space as zeros for future reference.

Quoted 44 lines

fs/f2fs/segment.c static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr) · line 3121

Quoted 10 lines

fs/f2fs/segment.c +1 -0

 	mutex_lock(&curseg->curseg_mutex);
 

@@ -3117,6 +3118,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
 
 	page = grab_meta_page(sbi, blkaddr++);
 	kaddr = (unsigned char *)page_address(page);
+	memset(kaddr, 0, PAGE_SIZE);

I checked here, we will not use uninitialized space due to:

read_compacted_summaries()
{
...

	if (offset + SUMMARY_SIZE <= PAGE_SIZE - SUM_FOOTER_SIZE)
		continue;

...
}

Are we safe here?

fs/f2fs/segment.c static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr) · line 3146

Quoted 8 lines

fs/f2fs/segment.c +1 -0

 	/* Step 1: write nat cache */
 	seg_i = CURSEG_I(sbi, CURSEG_HOT_DATA);

@@ -3141,6 +3143,7 @@ static void write_compacted_summaries(struct f2fs_sb_info *sbi, block_t blkaddr)
 			if (!page) {
 				page = grab_meta_page(sbi, blkaddr++);
 				kaddr = (unsigned char *)page_address(page);
+				memset(kaddr, 0, PAGE_SIZE);

Ditto.

Quoted 10 lines

fs/f2fs/segment.c +0 -0

 				written_size = 0;
 			}
 			summary = (struct f2fs_summary *)(kaddr + written_size);

fs/f2fs/segment.h +0 -0

index 3325d0769723..492ad0c86fa9 100644
@@ -375,6 +375,7 @@ static inline void seg_info_to_sit_page(struct f2fs_sb_info *sbi,
 	int i;
 

Quoted 11 lines

fs/f2fs/segment.h static inline void seg_info_to_sit_page(struct f2fs_sb_info *sbi, · line 378

Quoted 2 lines

fs/f2fs/segment.h +1 -0

 	raw_sit = (struct f2fs_sit_block *)page_address(page);
+	memset(raw_sit, 0, PAGE_SIZE);

I suspect that root cause of panic is that we don't memset here?

Thanks,

Quoted 4 lines

fs/f2fs/segment.h +0 -0

 	for (i = 0; i < end - start; i++) {
 		rs = &raw_sit->entries[i];
 		se = get_seg_entry(sbi, start + i);

.

From: Chao Yu <[email protected]>

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/inode.c    | 4 ++--
 fs/f2fs/node.c     | 6 ++++--
 fs/f2fs/node.h     | 7 ++-----
 fs/f2fs/recovery.c | 3 +--
 4 files changed, 9 insertions(+), 11 deletions(-)

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/node.c +2 -1

index 9a99243054ba..6fc3311820ec 100644
@@ -1096,7 +1096,8 @@ struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 
 	f2fs_wait_on_page_writeback(page, NODE, true);
-	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs);
 	set_cold_node(page, S_ISDIR(dn->inode->i_mode));
 	if (!PageUptodate(page))
 		SetPageUptodate(page);

fs/f2fs/node.c +1 -0

+	fill_node_footer(ipage, ino, ino, 0);
 	set_cold_node(page, false);
 
 	src = F2FS_INODE(page);

fs/f2fs/node.h +2 -5

index b95e49e4a928..42cd081114ab 100644
@@ -263,15 +263,12 @@ static inline block_t next_blkaddr_of_node(struct page *node_page)
 }
 
 static inline void fill_node_footer(struct page *page, nid_t nid,
-				nid_t ino, unsigned int ofs, bool reset)
+				nid_t ino, unsigned int ofs)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
 	unsigned int old_flag = 0;
 
-	if (reset)
-		memset(rn, 0, sizeof(*rn));
-	else
-		old_flag = le32_to_cpu(rn->footer.flag);
+	old_flag = le32_to_cpu(rn->footer.flag);
 
 	rn->footer.nid = cpu_to_le32(nid);
 	rn->footer.ino = cpu_to_le32(ino);

fs/f2fs/recovery.c +1 -2

index 1b23d3febe4c..de24f3247aa5 100644
@@ -540,8 +540,7 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	}
 
 	copy_node_footer(dn.node_page, page);
-	fill_node_footer(dn.node_page, dn.nid, ni.ino,
-					ofs_of_node(page), false);
+	fill_node_footer(dn.node_page, dn.nid, ni.ino, ofs_of_node(page));
 	set_page_dirty(dn.node_page);
 err:
 	f2fs_put_dnode(&dn);

Quoted 57 lines · Jaegeuk Kim

From: Chao Yu <[email protected]>

Related to https://lkml.org/lkml/2018/4/8/661

Sometimes, we need to write meta data to new allocated block address, then we will allocate a zeroed page in inner inode's address space, and fill partial data in it, and leave other place with zero value which means some fields are initial status.

There are two inner inodes (meta inode and node inode) setting __GFP_ZERO, I have just checked them, for both of them, we can avoid using __GFP_ZERO, and do initialization by ourselves to avoid unneeded/redundant zeroing from mm.

Cc <[email protected]>

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

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

---
 fs/f2fs/inode.c    | 4 ++--
 fs/f2fs/node.c     | 6 ++++--
 fs/f2fs/node.h     | 7 ++-----
 fs/f2fs/recovery.c | 3 +--
 4 files changed, 9 insertions(+), 11 deletions(-)

fs/f2fs/inode.c +2 -2

index 417c9dcd0269..87535bf63421 100644
@@ -320,10 +320,10 @@ struct inode *f2fs_iget(struct super_block *sb, unsigned long ino)
 make_now:
 	if (ino == F2FS_NODE_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_node_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (ino == F2FS_META_INO(sbi)) {
 		inode->i_mapping->a_ops = &f2fs_meta_aops;
-		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_ZERO);
+		mapping_set_gfp_mask(inode->i_mapping, GFP_NOFS);
 	} else if (S_ISREG(inode->i_mode)) {
 		inode->i_op = &f2fs_file_inode_operations;
 		inode->i_fop = &f2fs_file_operations;

fs/f2fs/node.c +2 -1

index 9a99243054ba..6fc3311820ec 100644
@@ -1096,7 +1096,8 @@ struct page *new_node_page(struct dnode_of_data *dn, unsigned int ofs)
 	set_node_addr(sbi, &new_ni, NEW_ADDR, false);
 
 	f2fs_wait_on_page_writeback(page, NODE, true);
-	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);
+	fill_node_footer(page, dn->nid, dn->inode->i_ino, ofs);
 	set_cold_node(page, S_ISDIR(dn->inode->i_mode));
 	if (!PageUptodate(page))
 		SetPageUptodate(page);

@@ -2311,7 +2312,8 @@ int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page)
 

fs/f2fs/node.c int recover_inode_page(struct f2fs_sb_info *sbi, struct page *page) · line 2315

Quoted 4 lines

fs/f2fs/node.c +1 -1

 	if (!PageUptodate(ipage))
 		SetPageUptodate(ipage);
-	fill_node_footer(ipage, ino, ino, 0, true);
+	memset(F2FS_NODE(page), 0, PAGE_SIZE);

At a glance, should be memset(F2FS_NODE(ipage), 0, PAGE_SIZE);

Sorry about that.

Thanks,

Quoted 41 lines

fs/f2fs/node.c +1 -0

+	fill_node_footer(ipage, ino, ino, 0);
 	set_cold_node(page, false);
 
 	src = F2FS_INODE(page);

fs/f2fs/node.h +2 -5

index b95e49e4a928..42cd081114ab 100644
@@ -263,15 +263,12 @@ static inline block_t next_blkaddr_of_node(struct page *node_page)
 }
 
 static inline void fill_node_footer(struct page *page, nid_t nid,
-				nid_t ino, unsigned int ofs, bool reset)
+				nid_t ino, unsigned int ofs)
 {
 	struct f2fs_node *rn = F2FS_NODE(page);
 	unsigned int old_flag = 0;
 
-	if (reset)
-		memset(rn, 0, sizeof(*rn));
-	else
-		old_flag = le32_to_cpu(rn->footer.flag);
+	old_flag = le32_to_cpu(rn->footer.flag);
 
 	rn->footer.nid = cpu_to_le32(nid);
 	rn->footer.ino = cpu_to_le32(ino);

fs/f2fs/recovery.c +1 -2

index 1b23d3febe4c..de24f3247aa5 100644
@@ -540,8 +540,7 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 	}
 
 	copy_node_footer(dn.node_page, page);
-	fill_node_footer(dn.node_page, dn.nid, ni.ino,
-					ofs_of_node(page), false);
+	fill_node_footer(dn.node_page, dn.nid, ni.ino, ofs_of_node(page));
 	set_page_dirty(dn.node_page);
 err:
 	f2fs_put_dnode(&dn);

Actually, we don't need to do this, since fill_node_footer(true) will reset the page.