* [PATCH RFC] f2fs: fix an error case of missing update inode page
@ 2017-12-05 4:07 Yunlei He
2017-12-08 5:26 ` Jaegeuk Kim
2017-12-14 19:46 ` Jaegeuk Kim
0 siblings, 2 replies; 9+ messages in thread
From: Yunlei He @ 2017-12-05 4:07 UTC (permalink / raw)
To: jaegeuk, yuchao0, linux-f2fs-devel; +Cc: ning.jia, heyunlei
-Thread A Thread B
-write_checkpoint
-block_operations
-f2fs_unlock_all -f2fs_sync_file
-f2fs_write_inode
-f2fs_inode_synced
-f2fs_sync_inode_meta
-sync_node_pages
-set_page_drity
In this case, if sudden power off without next new checkpoint,
the last inode page update will lost. wb_writeback is same with
fsync.
Signed-off-by: Yunlei He <heyunlei@huawei.com>
---
fs/f2fs/f2fs.h | 4 ++--
fs/f2fs/inode.c | 15 +++++++--------
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 82f1dc3..38f9324 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
-int update_inode(struct inode *inode, struct page *node_page);
-int update_inode_page(struct inode *inode);
+void update_inode(struct inode *inode, struct page *node_page);
+void update_inode_page(struct inode *inode);
int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
void f2fs_evict_inode(struct inode *inode);
void handle_failed_inode(struct inode *inode);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index b4c4f2b..10d3c7c 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
return inode;
}
-int update_inode(struct inode *inode, struct page *node_page)
+void update_inode(struct inode *inode, struct page *node_page)
{
struct f2fs_inode *ri;
struct extent_tree *et = F2FS_I(inode)->extent_tree;
- f2fs_inode_synced(inode);
-
f2fs_wait_on_page_writeback(node_page, NODE, true);
+ set_page_dirty(node_page);
+
+ f2fs_inode_synced(inode);
ri = F2FS_INODE(node_page);
@@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
if (inode->i_nlink == 0)
clear_inline_node(node_page);
- return set_page_dirty(node_page);
}
-int update_inode_page(struct inode *inode)
+void update_inode_page(struct inode *inode)
{
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct page *node_page;
@@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
} else if (err != -ENOENT) {
f2fs_stop_checkpoint(sbi, false);
}
- return 0;
+ return;
}
- ret = update_inode(inode, node_page);
+ update_inode(inode, node_page);
f2fs_put_page(node_page, 1);
- return ret;
}
int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
--
1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He
@ 2017-12-08 5:26 ` Jaegeuk Kim
2017-12-08 5:44 ` heyunlei
2017-12-14 19:46 ` Jaegeuk Kim
1 sibling, 1 reply; 9+ messages in thread
From: Jaegeuk Kim @ 2017-12-08 5:26 UTC (permalink / raw)
To: Yunlei He; +Cc: ning.jia, linux-f2fs-devel
On 12/05, Yunlei He wrote:
> -Thread A Thread B
>
> -write_checkpoint
> -block_operations
> -f2fs_unlock_all -f2fs_sync_file
> -f2fs_write_inode
> -f2fs_inode_synced
>
> -f2fs_sync_inode_meta
> -sync_node_pages
> -set_page_drity
>
> In this case, if sudden power off without next new checkpoint,
> the last inode page update will lost. wb_writeback is same with
> fsync.
BTW, what do you mean wb_writeback is same with fsync?
Thanks,
>
> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> ---
> fs/f2fs/f2fs.h | 4 ++--
> fs/f2fs/inode.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 82f1dc3..38f9324 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> -int update_inode(struct inode *inode, struct page *node_page);
> -int update_inode_page(struct inode *inode);
> +void update_inode(struct inode *inode, struct page *node_page);
> +void update_inode_page(struct inode *inode);
> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
> void f2fs_evict_inode(struct inode *inode);
> void handle_failed_inode(struct inode *inode);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index b4c4f2b..10d3c7c 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
> return inode;
> }
>
> -int update_inode(struct inode *inode, struct page *node_page)
> +void update_inode(struct inode *inode, struct page *node_page)
> {
> struct f2fs_inode *ri;
> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>
> - f2fs_inode_synced(inode);
> -
> f2fs_wait_on_page_writeback(node_page, NODE, true);
> + set_page_dirty(node_page);
> +
> + f2fs_inode_synced(inode);
>
> ri = F2FS_INODE(node_page);
>
> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
> if (inode->i_nlink == 0)
> clear_inline_node(node_page);
>
> - return set_page_dirty(node_page);
> }
>
> -int update_inode_page(struct inode *inode)
> +void update_inode_page(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> struct page *node_page;
> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
> } else if (err != -ENOENT) {
> f2fs_stop_checkpoint(sbi, false);
> }
> - return 0;
> + return;
> }
> - ret = update_inode(inode, node_page);
> + update_inode(inode, node_page);
> f2fs_put_page(node_page, 1);
> - return ret;
> }
>
> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> --
> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-08 5:26 ` Jaegeuk Kim
@ 2017-12-08 5:44 ` heyunlei
2017-12-11 13:31 ` Chao Yu
0 siblings, 1 reply; 9+ messages in thread
From: heyunlei @ 2017-12-08 5:44 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
>-----Original Message-----
>From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>Sent: Friday, December 08, 2017 1:27 PM
>To: heyunlei
>Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
>Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
>
>On 12/05, Yunlei He wrote:
>> -Thread A Thread B
>>
>> -write_checkpoint
>> -block_operations
>> -f2fs_unlock_all -f2fs_sync_file
>> -f2fs_write_inode
>> -f2fs_inode_synced
>>
>> -f2fs_sync_inode_meta
>> -sync_node_pages
>> -set_page_drity
>>
>> In this case, if sudden power off without next new checkpoint,
>> the last inode page update will lost. wb_writeback is same with
>> fsync.
>
>BTW, what do you mean wb_writeback is same with fsync?
Background write back will call f2fs_write_inode, which is same with f2fs_sync_file.
Thanks.
>
>Thanks,
>
>>
>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 4 ++--
>> fs/f2fs/inode.c | 15 +++++++--------
>> 2 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 82f1dc3..38f9324 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
>> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
>> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
>> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
>> -int update_inode(struct inode *inode, struct page *node_page);
>> -int update_inode_page(struct inode *inode);
>> +void update_inode(struct inode *inode, struct page *node_page);
>> +void update_inode_page(struct inode *inode);
>> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
>> void f2fs_evict_inode(struct inode *inode);
>> void handle_failed_inode(struct inode *inode);
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index b4c4f2b..10d3c7c 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
>> return inode;
>> }
>>
>> -int update_inode(struct inode *inode, struct page *node_page)
>> +void update_inode(struct inode *inode, struct page *node_page)
>> {
>> struct f2fs_inode *ri;
>> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>>
>> - f2fs_inode_synced(inode);
>> -
>> f2fs_wait_on_page_writeback(node_page, NODE, true);
>> + set_page_dirty(node_page);
>> +
>> + f2fs_inode_synced(inode);
>>
>> ri = F2FS_INODE(node_page);
>>
>> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
>> if (inode->i_nlink == 0)
>> clear_inline_node(node_page);
>>
>> - return set_page_dirty(node_page);
>> }
>>
>> -int update_inode_page(struct inode *inode)
>> +void update_inode_page(struct inode *inode)
>> {
>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> struct page *node_page;
>> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
>> } else if (err != -ENOENT) {
>> f2fs_stop_checkpoint(sbi, false);
>> }
>> - return 0;
>> + return;
>> }
>> - ret = update_inode(inode, node_page);
>> + update_inode(inode, node_page);
>> f2fs_put_page(node_page, 1);
>> - return ret;
>> }
>>
>> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>> --
>> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-08 5:44 ` heyunlei
@ 2017-12-11 13:31 ` Chao Yu
0 siblings, 0 replies; 9+ messages in thread
From: Chao Yu @ 2017-12-11 13:31 UTC (permalink / raw)
To: heyunlei, Jaegeuk Kim
Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
On 2017/12/8 13:44, heyunlei wrote:
>
>
>> -----Original Message-----
>> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>> Sent: Friday, December 08, 2017 1:27 PM
>> To: heyunlei
>> Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
>> Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
>>
>> On 12/05, Yunlei He wrote:
>>> -Thread A Thread B
>>>
>>> -write_checkpoint
>>> -block_operations
>>> -f2fs_unlock_all -f2fs_sync_file
>>> -f2fs_write_inode
>>> -f2fs_inode_synced
>>>
>>> -f2fs_sync_inode_meta
>>> -sync_node_pages
>>> -set_page_drity
>>>
>>> In this case, if sudden power off without next new checkpoint,
>>> the last inode page update will lost. wb_writeback is same with
>>> fsync.
>>
>> BTW, what do you mean wb_writeback is same with fsync?
>
> Background write back will call f2fs_write_inode, which is same with f2fs_sync_file.
Can you add related call stack in commit log? Other part looks good to me.
Reviewed-by: Chao Yu <yuchao0@huawei.com>
Thanks,
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He
2017-12-08 5:26 ` Jaegeuk Kim
@ 2017-12-14 19:46 ` Jaegeuk Kim
2017-12-15 2:24 ` heyunlei
1 sibling, 1 reply; 9+ messages in thread
From: Jaegeuk Kim @ 2017-12-14 19:46 UTC (permalink / raw)
To: Yunlei He; +Cc: ning.jia, linux-f2fs-devel
On 12/05, Yunlei He wrote:
> -Thread A Thread B
>
> -write_checkpoint
> -block_operations
> -f2fs_unlock_all -f2fs_sync_file
> -f2fs_write_inode
> -f2fs_inode_synced
>
> -f2fs_sync_inode_meta
> -sync_node_pages
> -set_page_drity
>
> In this case, if sudden power off without next new checkpoint,
> the last inode page update will lost. wb_writeback is same with
> fsync.
Does this patch fix the problem?
-write_checkpoint
-block_operations
-f2fs_unlock_all -f2fs_sync_file
-f2fs_write_inode
-set_page_drity
-f2fs_inode_synced
-f2fs_sync_inode_meta
-sync_node_pages
I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again.
Thanks,
>
> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> ---
> fs/f2fs/f2fs.h | 4 ++--
> fs/f2fs/inode.c | 15 +++++++--------
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 82f1dc3..38f9324 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> -int update_inode(struct inode *inode, struct page *node_page);
> -int update_inode_page(struct inode *inode);
> +void update_inode(struct inode *inode, struct page *node_page);
> +void update_inode_page(struct inode *inode);
> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
> void f2fs_evict_inode(struct inode *inode);
> void handle_failed_inode(struct inode *inode);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index b4c4f2b..10d3c7c 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
> return inode;
> }
>
> -int update_inode(struct inode *inode, struct page *node_page)
> +void update_inode(struct inode *inode, struct page *node_page)
> {
> struct f2fs_inode *ri;
> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>
> - f2fs_inode_synced(inode);
> -
> f2fs_wait_on_page_writeback(node_page, NODE, true);
> + set_page_dirty(node_page);
> +
> + f2fs_inode_synced(inode);
>
> ri = F2FS_INODE(node_page);
>
> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
> if (inode->i_nlink == 0)
> clear_inline_node(node_page);
>
> - return set_page_dirty(node_page);
> }
>
> -int update_inode_page(struct inode *inode)
> +void update_inode_page(struct inode *inode)
> {
> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> struct page *node_page;
> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
> } else if (err != -ENOENT) {
> f2fs_stop_checkpoint(sbi, false);
> }
> - return 0;
> + return;
> }
> - ret = update_inode(inode, node_page);
> + update_inode(inode, node_page);
> f2fs_put_page(node_page, 1);
> - return ret;
> }
>
> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> --
> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-14 19:46 ` Jaegeuk Kim
@ 2017-12-15 2:24 ` heyunlei
2017-12-15 4:20 ` Jaegeuk Kim
0 siblings, 1 reply; 9+ messages in thread
From: heyunlei @ 2017-12-15 2:24 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
>-----Original Message-----
>From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>Sent: Friday, December 15, 2017 3:46 AM
>To: heyunlei
>Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
>Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
>
>On 12/05, Yunlei He wrote:
>> -Thread A Thread B
>>
>> -write_checkpoint
>> -block_operations
>> -f2fs_unlock_all -f2fs_sync_file
>> -f2fs_write_inode
>> -f2fs_inode_synced
>>
>> -f2fs_sync_inode_meta
>> -sync_node_pages
>> -set_page_drity
>>
>> In this case, if sudden power off without next new checkpoint,
>> the last inode page update will lost. wb_writeback is same with
>> fsync.
>
>Does this patch fix the problem?
>
I modify code as below:
@@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page)
struct extent_tree *et = F2FS_I(inode)->extent_tree;
f2fs_inode_synced(inode);
-
+ msleep(10000);
f2fs_wait_on_page_writeback(node_page, NODE, true);
shell 1: shell2:
dd if=/dev/zero of=./test bs=1M count=10
sync
echo "hello" >> ./test
fsync test // sleep 10s
sync //return quickly
echo c > /proc/sysrq-trigger //before fsync return
fsck will find iblocks mismatch, and with this patch it 's ok.
Besides,I came across another error like this:
f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL.
This case makes sense to this problem also.
> -write_checkpoint
> -block_operations
> -f2fs_unlock_all -f2fs_sync_file
> -f2fs_write_inode
> -set_page_drity
> -f2fs_inode_synced
> -f2fs_sync_inode_meta
> -sync_node_pages
>
>I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again.
It can also work well with f2fs_lock_op(), I am afraid other cases will update inode
like f2fs_write_inode without f2fs_lock_op
Thanks
>
>Thanks,
>
>>
>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>> ---
>> fs/f2fs/f2fs.h | 4 ++--
>> fs/f2fs/inode.c | 15 +++++++--------
>> 2 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 82f1dc3..38f9324 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
>> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
>> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
>> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
>> -int update_inode(struct inode *inode, struct page *node_page);
>> -int update_inode_page(struct inode *inode);
>> +void update_inode(struct inode *inode, struct page *node_page);
>> +void update_inode_page(struct inode *inode);
>> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
>> void f2fs_evict_inode(struct inode *inode);
>> void handle_failed_inode(struct inode *inode);
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index b4c4f2b..10d3c7c 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
>> return inode;
>> }
>>
>> -int update_inode(struct inode *inode, struct page *node_page)
>> +void update_inode(struct inode *inode, struct page *node_page)
>> {
>> struct f2fs_inode *ri;
>> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>>
>> - f2fs_inode_synced(inode);
>> -
>> f2fs_wait_on_page_writeback(node_page, NODE, true);
>> + set_page_dirty(node_page);
>> +
>> + f2fs_inode_synced(inode);
>>
>> ri = F2FS_INODE(node_page);
>>
>> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
>> if (inode->i_nlink == 0)
>> clear_inline_node(node_page);
>>
>> - return set_page_dirty(node_page);
>> }
>>
>> -int update_inode_page(struct inode *inode)
>> +void update_inode_page(struct inode *inode)
>> {
>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> struct page *node_page;
>> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
>> } else if (err != -ENOENT) {
>> f2fs_stop_checkpoint(sbi, false);
>> }
>> - return 0;
>> + return;
>> }
>> - ret = update_inode(inode, node_page);
>> + update_inode(inode, node_page);
>> f2fs_put_page(node_page, 1);
>> - return ret;
>> }
>>
>> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>> --
>> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-15 2:24 ` heyunlei
@ 2017-12-15 4:20 ` Jaegeuk Kim
2017-12-15 6:47 ` heyunlei
0 siblings, 1 reply; 9+ messages in thread
From: Jaegeuk Kim @ 2017-12-15 4:20 UTC (permalink / raw)
To: heyunlei; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
On 12/15, heyunlei wrote:
>
>
> >-----Original Message-----
> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> >Sent: Friday, December 15, 2017 3:46 AM
> >To: heyunlei
> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
> >
> >On 12/05, Yunlei He wrote:
> >> -Thread A Thread B
> >>
> >> -write_checkpoint
> >> -block_operations
> >> -f2fs_unlock_all -f2fs_sync_file
> >> -f2fs_write_inode
> >> -f2fs_inode_synced
> >>
> >> -f2fs_sync_inode_meta
> >> -sync_node_pages
> >> -set_page_drity
> >>
> >> In this case, if sudden power off without next new checkpoint,
> >> the last inode page update will lost. wb_writeback is same with
> >> fsync.
> >
> >Does this patch fix the problem?
> >
> I modify code as below:
> @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page)
> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>
> f2fs_inode_synced(inode);
> -
> + msleep(10000);
> f2fs_wait_on_page_writeback(node_page, NODE, true);
>
> shell 1: shell2:
> dd if=/dev/zero of=./test bs=1M count=10
> sync
> echo "hello" >> ./test
> fsync test // sleep 10s
> sync //return quickly
> echo c > /proc/sysrq-trigger //before fsync return
> fsck will find iblocks mismatch, and with this patch it 's ok.
So, do you mean it was fixed by adding msleep() on top of this patch as well?
Like:
- f2fs_write_inode()
- set_page_drity
- f2fs_inode_synced
- msleep(10000);
>
> Besides,I came across another error like this:
>
> f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL.
>
> This case makes sense to this problem also.
>
> > -write_checkpoint
> > -block_operations
> > -f2fs_unlock_all -f2fs_sync_file
> > -f2fs_write_inode
> > -set_page_drity
> > -f2fs_inode_synced
> > -f2fs_sync_inode_meta
> > -sync_node_pages
> >
> >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again.
>
> It can also work well with f2fs_lock_op(), I am afraid other cases will update inode
> like f2fs_write_inode without f2fs_lock_op
>
> Thanks
> >
> >Thanks,
> >
> >>
> >> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> >> ---
> >> fs/f2fs/f2fs.h | 4 ++--
> >> fs/f2fs/inode.c | 15 +++++++--------
> >> 2 files changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 82f1dc3..38f9324 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
> >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> >> -int update_inode(struct inode *inode, struct page *node_page);
> >> -int update_inode_page(struct inode *inode);
> >> +void update_inode(struct inode *inode, struct page *node_page);
> >> +void update_inode_page(struct inode *inode);
> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
> >> void f2fs_evict_inode(struct inode *inode);
> >> void handle_failed_inode(struct inode *inode);
> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >> index b4c4f2b..10d3c7c 100644
> >> --- a/fs/f2fs/inode.c
> >> +++ b/fs/f2fs/inode.c
> >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
> >> return inode;
> >> }
> >>
> >> -int update_inode(struct inode *inode, struct page *node_page)
> >> +void update_inode(struct inode *inode, struct page *node_page)
> >> {
> >> struct f2fs_inode *ri;
> >> struct extent_tree *et = F2FS_I(inode)->extent_tree;
> >>
> >> - f2fs_inode_synced(inode);
> >> -
> >> f2fs_wait_on_page_writeback(node_page, NODE, true);
> >> + set_page_dirty(node_page);
> >> +
> >> + f2fs_inode_synced(inode);
> >>
> >> ri = F2FS_INODE(node_page);
> >>
> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
> >> if (inode->i_nlink == 0)
> >> clear_inline_node(node_page);
> >>
> >> - return set_page_dirty(node_page);
> >> }
> >>
> >> -int update_inode_page(struct inode *inode)
> >> +void update_inode_page(struct inode *inode)
> >> {
> >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >> struct page *node_page;
> >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
> >> } else if (err != -ENOENT) {
> >> f2fs_stop_checkpoint(sbi, false);
> >> }
> >> - return 0;
> >> + return;
> >> }
> >> - ret = update_inode(inode, node_page);
> >> + update_inode(inode, node_page);
> >> f2fs_put_page(node_page, 1);
> >> - return ret;
> >> }
> >>
> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >> --
> >> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-15 4:20 ` Jaegeuk Kim
@ 2017-12-15 6:47 ` heyunlei
2017-12-19 23:37 ` Jaegeuk Kim
0 siblings, 1 reply; 9+ messages in thread
From: heyunlei @ 2017-12-15 6:47 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
>-----Original Message-----
>From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>Sent: Friday, December 15, 2017 12:21 PM
>To: heyunlei
>Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
>Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
>
>On 12/15, heyunlei wrote:
>>
>>
>> >-----Original Message-----
>> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
>> >Sent: Friday, December 15, 2017 3:46 AM
>> >To: heyunlei
>> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
>> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
>> >
>> >On 12/05, Yunlei He wrote:
>> >> -Thread A Thread B
>> >>
>> >> -write_checkpoint
>> >> -block_operations
>> >> -f2fs_unlock_all -f2fs_sync_file
>> >> -f2fs_write_inode
>> >> -f2fs_inode_synced
>> >>
>> >> -f2fs_sync_inode_meta
>> >> -sync_node_pages
>> >> -set_page_drity
>> >>
>> >> In this case, if sudden power off without next new checkpoint,
>> >> the last inode page update will lost. wb_writeback is same with
>> >> fsync.
>> >
>> >Does this patch fix the problem?
>> >
>> I modify code as below:
>> @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page)
>> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>>
>> f2fs_inode_synced(inode);
>> -
>> + msleep(10000);
>> f2fs_wait_on_page_writeback(node_page, NODE, true);
>>
>> shell 1: shell2:
>> dd if=/dev/zero of=./test bs=1M count=10
>> sync
>> echo "hello" >> ./test
>> fsync test // sleep 10s
>> sync //return quickly
>> echo c > /proc/sysrq-trigger //before fsync return
>> fsck will find iblocks mismatch, and with this patch it 's ok.
>
>So, do you mean it was fixed by adding msleep() on top of this patch as well?
>Like:
> - f2fs_write_inode()
> - set_page_drity
> - f2fs_inode_synced
> - msleep(10000);
>
Yes, write_checkpoint will be blocked in sync dirty node pages, until fsync update inode
page and unlock node page.
Thanks.
>>
>> Besides,I came across another error like this:
>>
>> f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL.
>>
>> This case makes sense to this problem also.
>>
>> > -write_checkpoint
>> > -block_operations
>> > -f2fs_unlock_all -f2fs_sync_file
>> > -f2fs_write_inode
>> > -set_page_drity
>> > -f2fs_inode_synced
>> > -f2fs_sync_inode_meta
>> > -sync_node_pages
>> >
>> >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again.
>>
>> It can also work well with f2fs_lock_op(), I am afraid other cases will update inode
>> like f2fs_write_inode without f2fs_lock_op
>>
>> Thanks
>> >
>> >Thanks,
>> >
>> >>
>> >> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>> >> ---
>> >> fs/f2fs/f2fs.h | 4 ++--
>> >> fs/f2fs/inode.c | 15 +++++++--------
>> >> 2 files changed, 9 insertions(+), 10 deletions(-)
>> >>
>> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> >> index 82f1dc3..38f9324 100644
>> >> --- a/fs/f2fs/f2fs.h
>> >> +++ b/fs/f2fs/f2fs.h
>> >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
>> >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
>> >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
>> >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
>> >> -int update_inode(struct inode *inode, struct page *node_page);
>> >> -int update_inode_page(struct inode *inode);
>> >> +void update_inode(struct inode *inode, struct page *node_page);
>> >> +void update_inode_page(struct inode *inode);
>> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
>> >> void f2fs_evict_inode(struct inode *inode);
>> >> void handle_failed_inode(struct inode *inode);
>> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> >> index b4c4f2b..10d3c7c 100644
>> >> --- a/fs/f2fs/inode.c
>> >> +++ b/fs/f2fs/inode.c
>> >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
>> >> return inode;
>> >> }
>> >>
>> >> -int update_inode(struct inode *inode, struct page *node_page)
>> >> +void update_inode(struct inode *inode, struct page *node_page)
>> >> {
>> >> struct f2fs_inode *ri;
>> >> struct extent_tree *et = F2FS_I(inode)->extent_tree;
>> >>
>> >> - f2fs_inode_synced(inode);
>> >> -
>> >> f2fs_wait_on_page_writeback(node_page, NODE, true);
>> >> + set_page_dirty(node_page);
>> >> +
>> >> + f2fs_inode_synced(inode);
>> >>
>> >> ri = F2FS_INODE(node_page);
>> >>
>> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
>> >> if (inode->i_nlink == 0)
>> >> clear_inline_node(node_page);
>> >>
>> >> - return set_page_dirty(node_page);
>> >> }
>> >>
>> >> -int update_inode_page(struct inode *inode)
>> >> +void update_inode_page(struct inode *inode)
>> >> {
>> >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> >> struct page *node_page;
>> >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
>> >> } else if (err != -ENOENT) {
>> >> f2fs_stop_checkpoint(sbi, false);
>> >> }
>> >> - return 0;
>> >> + return;
>> >> }
>> >> - ret = update_inode(inode, node_page);
>> >> + update_inode(inode, node_page);
>> >> f2fs_put_page(node_page, 1);
>> >> - return ret;
>> >> }
>> >>
>> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
>> >> --
>> >> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC] f2fs: fix an error case of missing update inode page
2017-12-15 6:47 ` heyunlei
@ 2017-12-19 23:37 ` Jaegeuk Kim
0 siblings, 0 replies; 9+ messages in thread
From: Jaegeuk Kim @ 2017-12-19 23:37 UTC (permalink / raw)
To: heyunlei; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net
On 12/15, heyunlei wrote:
>
>
> >-----Original Message-----
> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> >Sent: Friday, December 15, 2017 12:21 PM
> >To: heyunlei
> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
> >
> >On 12/15, heyunlei wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org]
> >> >Sent: Friday, December 15, 2017 3:46 AM
> >> >To: heyunlei
> >> >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; Wangbintian; Jianing (Euler)
> >> >Subject: Re: [f2fs-dev][PATCH RFC] f2fs: fix an error case of missing update inode page
> >> >
> >> >On 12/05, Yunlei He wrote:
> >> >> -Thread A Thread B
> >> >>
> >> >> -write_checkpoint
> >> >> -block_operations
> >> >> -f2fs_unlock_all -f2fs_sync_file
> >> >> -f2fs_write_inode
> >> >> -f2fs_inode_synced
> >> >>
> >> >> -f2fs_sync_inode_meta
> >> >> -sync_node_pages
> >> >> -set_page_drity
> >> >>
> >> >> In this case, if sudden power off without next new checkpoint,
> >> >> the last inode page update will lost. wb_writeback is same with
> >> >> fsync.
> >> >
> >> >Does this patch fix the problem?
> >> >
> >> I modify code as below:
> >> @@ -366,7 +366,7 @@ int update_inode(struct inode *inode, struct page *node_page)
> >> struct extent_tree *et = F2FS_I(inode)->extent_tree;
> >>
> >> f2fs_inode_synced(inode);
> >> -
> >> + msleep(10000);
> >> f2fs_wait_on_page_writeback(node_page, NODE, true);
> >>
> >> shell 1: shell2:
> >> dd if=/dev/zero of=./test bs=1M count=10
> >> sync
> >> echo "hello" >> ./test
> >> fsync test // sleep 10s
> >> sync //return quickly
> >> echo c > /proc/sysrq-trigger //before fsync return
> >> fsck will find iblocks mismatch, and with this patch it 's ok.
> >
> >So, do you mean it was fixed by adding msleep() on top of this patch as well?
> >Like:
> > - f2fs_write_inode()
> > - set_page_drity
> > - f2fs_inode_synced
> > - msleep(10000);
> >
> Yes, write_checkpoint will be blocked in sync dirty node pages, until fsync update inode
> page and unlock node page.
Got it. I've concerned about inversion of PageDirty() and f2fs_inode_synced(),
but for now, I can't find any corner case simply. Let me do stress test with
this.
Thanks,
>
> Thanks.
> >>
> >> Besides,I came across another error like this:
> >>
> >> f2fs_inode-> i_xattr_nid is non-zero, but node blkaddr in raw nat entry is NULL.
> >>
> >> This case makes sense to this problem also.
> >>
> >> > -write_checkpoint
> >> > -block_operations
> >> > -f2fs_unlock_all -f2fs_sync_file
> >> > -f2fs_write_inode
> >> > -set_page_drity
> >> > -f2fs_inode_synced
> >> > -f2fs_sync_inode_meta
> >> > -sync_node_pages
> >> >
> >> >I think, we need to cover f2fs_lock_op() for f2fs_write_inode() again.
> >>
> >> It can also work well with f2fs_lock_op(), I am afraid other cases will update inode
> >> like f2fs_write_inode without f2fs_lock_op
> >>
> >> Thanks
> >> >
> >> >Thanks,
> >> >
> >> >>
> >> >> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> >> >> ---
> >> >> fs/f2fs/f2fs.h | 4 ++--
> >> >> fs/f2fs/inode.c | 15 +++++++--------
> >> >> 2 files changed, 9 insertions(+), 10 deletions(-)
> >> >>
> >> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> >> index 82f1dc3..38f9324 100644
> >> >> --- a/fs/f2fs/f2fs.h
> >> >> +++ b/fs/f2fs/f2fs.h
> >> >> @@ -2513,8 +2513,8 @@ int f2fs_getattr(const struct path *path, struct kstat *stat,
> >> >> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> >> >> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> >> >> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> >> >> -int update_inode(struct inode *inode, struct page *node_page);
> >> >> -int update_inode_page(struct inode *inode);
> >> >> +void update_inode(struct inode *inode, struct page *node_page);
> >> >> +void update_inode_page(struct inode *inode);
> >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc);
> >> >> void f2fs_evict_inode(struct inode *inode);
> >> >> void handle_failed_inode(struct inode *inode);
> >> >> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> >> >> index b4c4f2b..10d3c7c 100644
> >> >> --- a/fs/f2fs/inode.c
> >> >> +++ b/fs/f2fs/inode.c
> >> >> @@ -360,14 +360,15 @@ struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino)
> >> >> return inode;
> >> >> }
> >> >>
> >> >> -int update_inode(struct inode *inode, struct page *node_page)
> >> >> +void update_inode(struct inode *inode, struct page *node_page)
> >> >> {
> >> >> struct f2fs_inode *ri;
> >> >> struct extent_tree *et = F2FS_I(inode)->extent_tree;
> >> >>
> >> >> - f2fs_inode_synced(inode);
> >> >> -
> >> >> f2fs_wait_on_page_writeback(node_page, NODE, true);
> >> >> + set_page_dirty(node_page);
> >> >> +
> >> >> + f2fs_inode_synced(inode);
> >> >>
> >> >> ri = F2FS_INODE(node_page);
> >> >>
> >> >> @@ -426,10 +427,9 @@ int update_inode(struct inode *inode, struct page *node_page)
> >> >> if (inode->i_nlink == 0)
> >> >> clear_inline_node(node_page);
> >> >>
> >> >> - return set_page_dirty(node_page);
> >> >> }
> >> >>
> >> >> -int update_inode_page(struct inode *inode)
> >> >> +void update_inode_page(struct inode *inode)
> >> >> {
> >> >> struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> >> >> struct page *node_page;
> >> >> @@ -444,11 +444,10 @@ int update_inode_page(struct inode *inode)
> >> >> } else if (err != -ENOENT) {
> >> >> f2fs_stop_checkpoint(sbi, false);
> >> >> }
> >> >> - return 0;
> >> >> + return;
> >> >> }
> >> >> - ret = update_inode(inode, node_page);
> >> >> + update_inode(inode, node_page);
> >> >> f2fs_put_page(node_page, 1);
> >> >> - return ret;
> >> >> }
> >> >>
> >> >> int f2fs_write_inode(struct inode *inode, struct writeback_control *wbc)
> >> >> --
> >> >> 1.9.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-19 23:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-05 4:07 [PATCH RFC] f2fs: fix an error case of missing update inode page Yunlei He
2017-12-08 5:26 ` Jaegeuk Kim
2017-12-08 5:44 ` heyunlei
2017-12-11 13:31 ` Chao Yu
2017-12-14 19:46 ` Jaegeuk Kim
2017-12-15 2:24 ` heyunlei
2017-12-15 4:20 ` Jaegeuk Kim
2017-12-15 6:47 ` heyunlei
2017-12-19 23:37 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).