* Re: [PATCH] f2fs: don't use GFP_ZERO for page caches
2018-04-09 19:00 [PATCH] f2fs: don't use GFP_ZERO for page caches Jaegeuk Kim
@ 2018-04-09 19:03 ` Jaegeuk Kim
2018-04-09 20:42 ` [PATCH v2] " Jaegeuk Kim
2018-04-10 3:32 ` Chao Yu
2 siblings, 0 replies; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-09 19:03 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel; +Cc: Chao Yu, stable
Chao,
I have to test this for a while. Meanwhile, could you take a look at this?
On 04/09, Jaegeuk Kim wrote:
> From: Chao Yu <yuchao0@huawei.com>
>
> 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: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9a99243054ba..6fc3311820ec 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -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);
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index b95e49e4a928..42cd081114ab 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -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);
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 1b23d3febe4c..de24f3247aa5 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -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);
> --
> 2.15.0.531.g2ccb3012c9-goog
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2] f2fs: don't use GFP_ZERO for page caches
2018-04-09 19:00 [PATCH] f2fs: don't use GFP_ZERO for page caches Jaegeuk Kim
2018-04-09 19:03 ` Jaegeuk Kim
@ 2018-04-09 20:42 ` Jaegeuk Kim
2018-04-10 1:35 ` [f2fs-dev] " Jaegeuk Kim
2018-04-10 3:29 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
2018-04-10 3:32 ` Chao Yu
2 siblings, 2 replies; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-09 20:42 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel
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: <stable@vger.kernel.org>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/inode.c | 4 ++--
fs/f2fs/node.c | 3 ++-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 417c9dcd0269..87535bf63421 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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;
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 9a99243054ba..5a4469093e43 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -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);
--
2.15.0.531.g2ccb3012c9-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v2] f2fs: don't use GFP_ZERO for page caches
2018-04-09 20:42 ` [PATCH v2] " Jaegeuk Kim
@ 2018-04-10 1:35 ` Jaegeuk Kim
2018-04-10 1:47 ` Chao Yu
2018-04-10 3:29 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
1 sibling, 1 reply; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-10 1:35 UTC (permalink / raw)
To: linux-kernel, linux-f2fs-devel
On 04/09, Jaegeuk Kim wrote:
> 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.
Okay, it seems we're missing some more places to reset the memory, since
I'm still getting a panic. :(
Thanks,
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/inode.c | 4 ++--
> fs/f2fs/node.c | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9a99243054ba..5a4469093e43 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -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);
> --
> 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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v2] f2fs: don't use GFP_ZERO for page caches
2018-04-10 1:35 ` [f2fs-dev] " Jaegeuk Kim
@ 2018-04-10 1:47 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 1:47 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On 2018/4/10 9:35, Jaegeuk Kim wrote:
> On 04/09, Jaegeuk Kim wrote:
>> 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.
>
> Okay, it seems we're missing some more places to reset the memory, since
> I'm still getting a panic. :(
Alright, let me recheck this patch... ;)
Thanks,
>
> Thanks,
>
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>> fs/f2fs/inode.c | 4 ++--
>> fs/f2fs/node.c | 3 ++-
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index 417c9dcd0269..87535bf63421 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -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;
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 9a99243054ba..5a4469093e43 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -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);
>> --
>> 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
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v2] f2fs: don't use GFP_ZERO for page caches
@ 2018-04-10 1:47 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 1:47 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On 2018/4/10 9:35, Jaegeuk Kim wrote:
> On 04/09, Jaegeuk Kim wrote:
>> 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.
>
> Okay, it seems we're missing some more places to reset the memory, since
> I'm still getting a panic. :(
Alright, let me recheck this patch... ;)
Thanks,
>
> Thanks,
>
>>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>> ---
>> fs/f2fs/inode.c | 4 ++--
>> fs/f2fs/node.c | 3 ++-
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index 417c9dcd0269..87535bf63421 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -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;
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 9a99243054ba..5a4469093e43 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -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);
>> --
>> 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
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
2018-04-09 20:42 ` [PATCH v2] " Jaegeuk Kim
2018-04-10 1:35 ` [f2fs-dev] " Jaegeuk Kim
@ 2018-04-10 3:29 ` Jaegeuk Kim
2018-04-10 6:33 ` Chao Yu
1 sibling, 1 reply; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-10 3:29 UTC (permalink / raw)
To: linux-kernel, 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: <stable@vger.kernel.org>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
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(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index bf779461df13..2e23b953d304 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -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;
}
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 417c9dcd0269..87535bf63421 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -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;
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a4b8e3e24ccb..1f5db557ab96 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
struct f2fs_summary_block *dst;
dst = (struct f2fs_summary_block *)page_address(page);
+ memset(dst, 0, PAGE_SIZE);
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);
/* 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);
written_size = 0;
}
summary = (struct f2fs_summary *)(kaddr + written_size);
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 3325d0769723..492ad0c86fa9 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -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);
for (i = 0; i < end - start; i++) {
rs = &raw_sit->entries[i];
se = get_seg_entry(sbi, start + i);
--
2.15.0.531.g2ccb3012c9-goog
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
2018-04-10 3:29 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
@ 2018-04-10 6:33 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 6:33 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On 2018/4/10 11:29, Jaegeuk Kim wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index bf779461df13..2e23b953d304 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -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;
> }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index a4b8e3e24ccb..1f5db557ab96 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
> struct f2fs_summary_block *dst;
>
> 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?
>
> 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?
>
> /* 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.
> written_size = 0;
> }
> summary = (struct f2fs_summary *)(kaddr + written_size);
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 3325d0769723..492ad0c86fa9 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -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,
> for (i = 0; i < end - start; i++) {
> rs = &raw_sit->entries[i];
> se = get_seg_entry(sbi, start + i);
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
@ 2018-04-10 6:33 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 6:33 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel
On 2018/4/10 11:29, Jaegeuk Kim wrote:
> 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: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index bf779461df13..2e23b953d304 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -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;
> }
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index a4b8e3e24ccb..1f5db557ab96 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
> struct f2fs_summary_block *dst;
>
> 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?
>
> 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?
>
> /* 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.
> written_size = 0;
> }
> summary = (struct f2fs_summary *)(kaddr + written_size);
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 3325d0769723..492ad0c86fa9 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -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,
> for (i = 0; i < end - start; i++) {
> rs = &raw_sit->entries[i];
> se = get_seg_entry(sbi, start + i);
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
2018-04-10 6:33 ` Chao Yu
(?)
@ 2018-04-10 6:40 ` Jaegeuk Kim
2018-04-10 7:27 ` Chao Yu
-1 siblings, 1 reply; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-10 6:40 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 04/10, Chao Yu wrote:
> On 2018/4/10 11:29, Jaegeuk Kim wrote:
> > 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: <stable@vger.kernel.org>
> > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > 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(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index bf779461df13..2e23b953d304 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -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;
> > }
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 417c9dcd0269..87535bf63421 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -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;
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index a4b8e3e24ccb..1f5db557ab96 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
> > struct f2fs_summary_block *dst;
> >
> > 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.
>
> >
> > 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?
>
> >
> > /* 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.
>
> > written_size = 0;
> > }
> > summary = (struct f2fs_summary *)(kaddr + written_size);
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index 3325d0769723..492ad0c86fa9 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -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,
>
> > for (i = 0; i < end - start; i++) {
> > rs = &raw_sit->entries[i];
> > se = get_seg_entry(sbi, start + i);
> >
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
2018-04-10 6:40 ` Jaegeuk Kim
@ 2018-04-10 7:27 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 7:27 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2018/4/10 14:40, Jaegeuk Kim wrote:
> On 04/10, Chao Yu wrote:
>> On 2018/4/10 11:29, Jaegeuk Kim wrote:
>>> 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: <stable@vger.kernel.org>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index bf779461df13..2e23b953d304 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -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;
>>> }
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 417c9dcd0269..87535bf63421 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -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;
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index a4b8e3e24ccb..1f5db557ab96 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
>>> struct f2fs_summary_block *dst;
>>>
>>> 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.
No problem, anyway, the patch is OK to me. :)
Thanks,
>
>>
>>>
>>> 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?
>>
>>>
>>> /* 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.
>>
>>> written_size = 0;
>>> }
>>> summary = (struct f2fs_summary *)(kaddr + written_size);
>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>> index 3325d0769723..492ad0c86fa9 100644
>>> --- a/fs/f2fs/segment.h
>>> +++ b/fs/f2fs/segment.h
>>> @@ -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,
>>
>>> for (i = 0; i < end - start; i++) {
>>> rs = &raw_sit->entries[i];
>>> se = get_seg_entry(sbi, start + i);
>>>
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [f2fs-dev] [PATCH v3] f2fs: don't use GFP_ZERO for page caches
@ 2018-04-10 7:27 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 7:27 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2018/4/10 14:40, Jaegeuk Kim wrote:
> On 04/10, Chao Yu wrote:
>> On 2018/4/10 11:29, Jaegeuk Kim wrote:
>>> 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: <stable@vger.kernel.org>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index bf779461df13..2e23b953d304 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -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;
>>> }
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 417c9dcd0269..87535bf63421 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -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;
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index a4b8e3e24ccb..1f5db557ab96 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -2021,6 +2021,7 @@ static void write_current_sum_page(struct f2fs_sb_info *sbi,
>>> struct f2fs_summary_block *dst;
>>>
>>> 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.
No problem, anyway, the patch is OK to me. :)
Thanks,
>
>>
>>>
>>> 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?
>>
>>>
>>> /* 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.
>>
>>> written_size = 0;
>>> }
>>> summary = (struct f2fs_summary *)(kaddr + written_size);
>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>> index 3325d0769723..492ad0c86fa9 100644
>>> --- a/fs/f2fs/segment.h
>>> +++ b/fs/f2fs/segment.h
>>> @@ -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,
>>
>>> for (i = 0; i < end - start; i++) {
>>> rs = &raw_sit->entries[i];
>>> se = get_seg_entry(sbi, start + i);
>>>
>
> .
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] f2fs: don't use GFP_ZERO for page caches
2018-04-09 19:00 [PATCH] f2fs: don't use GFP_ZERO for page caches Jaegeuk Kim
@ 2018-04-10 3:32 ` Chao Yu
2018-04-09 20:42 ` [PATCH v2] " Jaegeuk Kim
2018-04-10 3:32 ` Chao Yu
2 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 3:32 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel; +Cc: stable
On 2018/4/10 3:00, Jaegeuk Kim wrote:
> From: Chao Yu <yuchao0@huawei.com>
>
> 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: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9a99243054ba..6fc3311820ec 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -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);
At a glance, should be memset(F2FS_NODE(ipage), 0, PAGE_SIZE);
Sorry about that.
Thanks,
> + fill_node_footer(ipage, ino, ino, 0);
> set_cold_node(page, false);
>
> src = F2FS_INODE(page);
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index b95e49e4a928..42cd081114ab 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -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);
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 1b23d3febe4c..de24f3247aa5 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -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);
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] f2fs: don't use GFP_ZERO for page caches
@ 2018-04-10 3:32 ` Chao Yu
0 siblings, 0 replies; 17+ messages in thread
From: Chao Yu @ 2018-04-10 3:32 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel; +Cc: stable
On 2018/4/10 3:00, Jaegeuk Kim wrote:
> From: Chao Yu <yuchao0@huawei.com>
>
> 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: <stable@vger.kernel.org>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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(-)
>
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 417c9dcd0269..87535bf63421 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -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;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 9a99243054ba..6fc3311820ec 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -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);
At a glance, should be memset(F2FS_NODE(ipage), 0, PAGE_SIZE);
Sorry about that.
Thanks,
> + fill_node_footer(ipage, ino, ino, 0);
> set_cold_node(page, false);
>
> src = F2FS_INODE(page);
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index b95e49e4a928..42cd081114ab 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -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);
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 1b23d3febe4c..de24f3247aa5 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -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);
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] f2fs: don't use GFP_ZERO for page caches
2018-04-10 3:32 ` Chao Yu
(?)
@ 2018-04-10 3:50 ` Jaegeuk Kim
2018-04-10 6:36 ` Chao Yu
-1 siblings, 1 reply; 17+ messages in thread
From: Jaegeuk Kim @ 2018-04-10 3:50 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, stable
On 04/10, Chao Yu wrote:
> On 2018/4/10 3:00, Jaegeuk Kim wrote:
> > From: Chao Yu <yuchao0@huawei.com>
> >
> > 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: <stable@vger.kernel.org>
> > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > 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(-)
> >
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 417c9dcd0269..87535bf63421 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -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;
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > index 9a99243054ba..6fc3311820ec 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -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);
>
> At a glance, should be memset(F2FS_NODE(ipage), 0, PAGE_SIZE);
Actually, we don't need to do this, since fill_node_footer(true) will reset the
page.
>
> Sorry about that.
>
> Thanks,
>
> > + fill_node_footer(ipage, ino, ino, 0);
> > set_cold_node(page, false);
> >
> > src = F2FS_INODE(page);
> > diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> > index b95e49e4a928..42cd081114ab 100644
> > --- a/fs/f2fs/node.h
> > +++ b/fs/f2fs/node.h
> > @@ -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);
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index 1b23d3febe4c..de24f3247aa5 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -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);
> >
^ permalink raw reply [flat|nested] 17+ messages in thread