2022-04-12 23:09:56

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 1/2] f2fs: fix wrong condition check when failing metapage read

This patch fixes wrong initialization.

Fixes: 50c63009f6ab ("f2fs: avoid an infinite loop in f2fs_sync_dirty_inodes")
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/checkpoint.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index aba1b8a1ce66..ed61ac99a3cf 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -98,9 +98,9 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
}

if (unlikely(!PageUptodate(page))) {
- if (page->index == sbi->metapage_eio_ofs &&
- sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO) {
- set_ckpt_flags(sbi, CP_ERROR_FLAG);
+ if (page->index == sbi->metapage_eio_ofs) {
+ if (sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO)
+ set_ckpt_flags(sbi, CP_ERROR_FLAG);
} else {
sbi->metapage_eio_ofs = page->index;
sbi->metapage_eio_cnt = 0;
--
2.35.1.1178.g4f1659d476-goog


2022-04-13 00:09:02

by Jaegeuk Kim

[permalink] [raw]
Subject: [PATCH 2/2] f2fs: avoid infinite loop to flush node pages

xfstests/generic/475 can give EIO all the time which give an infinite loop
to flush node page like below. Let's avoid it.

[16418.518551] Call Trace:
[16418.518553] ? dm_submit_bio+0x48/0x400
[16418.518574] ? submit_bio_checks+0x1ac/0x5a0
[16418.525207] __submit_bio+0x1a9/0x230
[16418.525210] ? kmem_cache_alloc+0x29e/0x3c0
[16418.525223] submit_bio_noacct+0xa8/0x2b0
[16418.525226] submit_bio+0x4d/0x130
[16418.525238] __submit_bio+0x49/0x310 [f2fs]
[16418.525339] ? bio_add_page+0x6a/0x90
[16418.525344] f2fs_submit_page_bio+0x134/0x1f0 [f2fs]
[16418.525365] read_node_page+0x125/0x1b0 [f2fs]
[16418.525388] __get_node_page.part.0+0x58/0x3f0 [f2fs]
[16418.525409] __get_node_page+0x2f/0x60 [f2fs]
[16418.525431] f2fs_get_dnode_of_data+0x423/0x860 [f2fs]
[16418.525452] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525458] ? __mod_memcg_state.part.0+0x2a/0x30
[16418.525465] ? __mod_memcg_lruvec_state+0x27/0x40
[16418.525467] ? __xa_set_mark+0x57/0x70
[16418.525472] f2fs_do_write_data_page+0x10e/0x7b0 [f2fs]
[16418.525493] f2fs_write_single_data_page+0x555/0x830 [f2fs]
[16418.525514] ? sysvec_apic_timer_interrupt+0x4e/0x90
[16418.525518] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525523] f2fs_write_cache_pages+0x303/0x880 [f2fs]
[16418.525545] ? blk_flush_plug_list+0x47/0x100
[16418.525548] f2fs_write_data_pages+0xfd/0x320 [f2fs]
[16418.525569] do_writepages+0xd5/0x210
[16418.525648] filemap_fdatawrite_wbc+0x7d/0xc0
[16418.525655] filemap_fdatawrite+0x50/0x70
[16418.525658] f2fs_sync_dirty_inodes+0xa4/0x230 [f2fs]
[16418.525679] f2fs_write_checkpoint+0x16d/0x1720 [f2fs]
[16418.525699] ? ttwu_do_wakeup+0x1c/0x160
[16418.525709] ? ttwu_do_activate+0x6d/0xd0
[16418.525711] ? __wait_for_common+0x11d/0x150
[16418.525715] kill_f2fs_super+0xca/0x100 [f2fs]
[16418.525733] deactivate_locked_super+0x3b/0xb0
[16418.525739] deactivate_super+0x40/0x50
[16418.525741] cleanup_mnt+0x139/0x190
[16418.525747] __cleanup_mnt+0x12/0x20
[16418.525749] task_work_run+0x6d/0xa0
[16418.525765] exit_to_user_mode_prepare+0x1ad/0x1b0
[16418.525771] syscall_exit_to_user_mode+0x27/0x50
[16418.525774] do_syscall_64+0x48/0xc0
[16418.525776] entry_SYSCALL_64_after_hwframe+0x44/0xae

Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/checkpoint.c | 8 +-------
fs/f2fs/f2fs.h | 20 ++++++++++++++++----
fs/f2fs/node.c | 1 +
3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index ed61ac99a3cf..a1aa6b8f2ed2 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -98,13 +98,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
}

if (unlikely(!PageUptodate(page))) {
- if (page->index == sbi->metapage_eio_ofs) {
- if (sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO)
- set_ckpt_flags(sbi, CP_ERROR_FLAG);
- } else {
- sbi->metapage_eio_ofs = page->index;
- sbi->metapage_eio_cnt = 0;
- }
+ f2fs_handle_page_eio(sbi, page->index, META);
f2fs_put_page(page, 1);
return ERR_PTR(-EIO);
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 9c72a91912ae..2d10f1c8ed12 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -579,8 +579,8 @@ enum {
/* maximum retry quota flush count */
#define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8

-/* maximum retry of EIO'ed meta page */
-#define MAX_RETRY_META_PAGE_EIO 100
+/* maximum retry of EIO'ed page */
+#define MAX_RETRY_PAGE_EIO 100

#define F2FS_LINK_MAX 0xffffffff /* maximum link count per file */

@@ -1621,8 +1621,8 @@ struct f2fs_sb_info {
/* keep migration IO order for LFS mode */
struct f2fs_rwsem io_order_lock;
mempool_t *write_io_dummy; /* Dummy pages */
- pgoff_t metapage_eio_ofs; /* EIO page offset */
- int metapage_eio_cnt; /* EIO count */
+ pgoff_t page_eio_ofs[NR_PAGE_TYPE]; /* EIO page offset */
+ int page_eio_cnt[NR_PAGE_TYPE]; /* EIO count */

/* for checkpoint */
struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */
@@ -4538,6 +4538,18 @@ static inline bool f2fs_block_unit_discard(struct f2fs_sb_info *sbi)
return F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_BLOCK;
}

+static inline void f2fs_handle_page_eio(struct f2fs_sb_info *sbi, pgoff_t ofs,
+ enum page_type type)
+{
+ if (ofs == sbi->page_eio_ofs[type]) {
+ if (sbi->page_eio_cnt[type]++ == MAX_RETRY_PAGE_EIO)
+ set_ckpt_flags(sbi, CP_ERROR_FLAG);
+ } else {
+ sbi->page_eio_ofs[type] = ofs;
+ sbi->page_eio_cnt[type] = 0;
+ }
+}
+
#define EFSBADCRC EBADMSG /* Bad CRC detected */
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index fe8c38c9ac77..87f1a8364f21 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1416,6 +1416,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,

err = read_node_page(page, 0);
if (err < 0) {
+ f2fs_handle_page_eio(sbi, page->index, NODE);
f2fs_put_page(page, 1);
return ERR_PTR(err);
} else if (err == LOCKED_PAGE) {
--
2.35.1.1178.g4f1659d476-goog

2022-04-16 01:50:56

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [PATCH 2/2 v2] f2fs: avoid infinite loop to flush node pages

xfstests/generic/475 can give EIO all the time which give an infinite loop
to flush node page like below. Let's avoid it.

[16418.518551] Call Trace:
[16418.518553] ? dm_submit_bio+0x48/0x400
[16418.518574] ? submit_bio_checks+0x1ac/0x5a0
[16418.525207] __submit_bio+0x1a9/0x230
[16418.525210] ? kmem_cache_alloc+0x29e/0x3c0
[16418.525223] submit_bio_noacct+0xa8/0x2b0
[16418.525226] submit_bio+0x4d/0x130
[16418.525238] __submit_bio+0x49/0x310 [f2fs]
[16418.525339] ? bio_add_page+0x6a/0x90
[16418.525344] f2fs_submit_page_bio+0x134/0x1f0 [f2fs]
[16418.525365] read_node_page+0x125/0x1b0 [f2fs]
[16418.525388] __get_node_page.part.0+0x58/0x3f0 [f2fs]
[16418.525409] __get_node_page+0x2f/0x60 [f2fs]
[16418.525431] f2fs_get_dnode_of_data+0x423/0x860 [f2fs]
[16418.525452] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525458] ? __mod_memcg_state.part.0+0x2a/0x30
[16418.525465] ? __mod_memcg_lruvec_state+0x27/0x40
[16418.525467] ? __xa_set_mark+0x57/0x70
[16418.525472] f2fs_do_write_data_page+0x10e/0x7b0 [f2fs]
[16418.525493] f2fs_write_single_data_page+0x555/0x830 [f2fs]
[16418.525514] ? sysvec_apic_timer_interrupt+0x4e/0x90
[16418.525518] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525523] f2fs_write_cache_pages+0x303/0x880 [f2fs]
[16418.525545] ? blk_flush_plug_list+0x47/0x100
[16418.525548] f2fs_write_data_pages+0xfd/0x320 [f2fs]
[16418.525569] do_writepages+0xd5/0x210
[16418.525648] filemap_fdatawrite_wbc+0x7d/0xc0
[16418.525655] filemap_fdatawrite+0x50/0x70
[16418.525658] f2fs_sync_dirty_inodes+0xa4/0x230 [f2fs]
[16418.525679] f2fs_write_checkpoint+0x16d/0x1720 [f2fs]
[16418.525699] ? ttwu_do_wakeup+0x1c/0x160
[16418.525709] ? ttwu_do_activate+0x6d/0xd0
[16418.525711] ? __wait_for_common+0x11d/0x150
[16418.525715] kill_f2fs_super+0xca/0x100 [f2fs]
[16418.525733] deactivate_locked_super+0x3b/0xb0
[16418.525739] deactivate_super+0x40/0x50
[16418.525741] cleanup_mnt+0x139/0x190
[16418.525747] __cleanup_mnt+0x12/0x20
[16418.525749] task_work_run+0x6d/0xa0
[16418.525765] exit_to_user_mode_prepare+0x1ad/0x1b0
[16418.525771] syscall_exit_to_user_mode+0x27/0x50
[16418.525774] do_syscall_64+0x48/0xc0
[16418.525776] entry_SYSCALL_64_after_hwframe+0x44/0xae

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

Change log from v1:
- cover one more error case

fs/f2fs/checkpoint.c | 8 +-------
fs/f2fs/f2fs.h | 20 ++++++++++++++++----
fs/f2fs/node.c | 2 ++
3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 909085a78f9c..319903c2b34f 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -98,13 +98,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
}

if (unlikely(!PageUptodate(page))) {
- if (page->index == sbi->metapage_eio_ofs) {
- if (sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO)
- set_ckpt_flags(sbi, CP_ERROR_FLAG);
- } else {
- sbi->metapage_eio_ofs = page->index;
- sbi->metapage_eio_cnt = 0;
- }
+ f2fs_handle_page_eio(sbi, page->index, META);
f2fs_put_page(page, 1);
return ERR_PTR(-EIO);
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index cd1e65bcf0b0..977826a22568 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -579,8 +579,8 @@ enum {
/* maximum retry quota flush count */
#define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8

-/* maximum retry of EIO'ed meta page */
-#define MAX_RETRY_META_PAGE_EIO 100
+/* maximum retry of EIO'ed page */
+#define MAX_RETRY_PAGE_EIO 100

#define F2FS_LINK_MAX 0xffffffff /* maximum link count per file */

@@ -1621,8 +1621,8 @@ struct f2fs_sb_info {
/* keep migration IO order for LFS mode */
struct f2fs_rwsem io_order_lock;
mempool_t *write_io_dummy; /* Dummy pages */
- pgoff_t metapage_eio_ofs; /* EIO page offset */
- int metapage_eio_cnt; /* EIO count */
+ pgoff_t page_eio_ofs[NR_PAGE_TYPE]; /* EIO page offset */
+ int page_eio_cnt[NR_PAGE_TYPE]; /* EIO count */

/* for checkpoint */
struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */
@@ -4543,6 +4543,18 @@ static inline void f2fs_io_schedule_timeout(long timeout)
io_schedule_timeout(timeout);
}

+static inline void f2fs_handle_page_eio(struct f2fs_sb_info *sbi, pgoff_t ofs,
+ enum page_type type)
+{
+ if (ofs == sbi->page_eio_ofs[type]) {
+ if (sbi->page_eio_cnt[type]++ == MAX_RETRY_PAGE_EIO)
+ set_ckpt_flags(sbi, CP_ERROR_FLAG);
+ } else {
+ sbi->page_eio_ofs[type] = ofs;
+ sbi->page_eio_cnt[type] = 0;
+ }
+}
+
#define EFSBADCRC EBADMSG /* Bad CRC detected */
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index c45d341dcf6e..c280f482c741 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1416,6 +1416,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,

err = read_node_page(page, 0);
if (err < 0) {
+ f2fs_handle_page_eio(sbi, page->index, NODE);
f2fs_put_page(page, 1);
return ERR_PTR(err);
} else if (err == LOCKED_PAGE) {
@@ -1452,6 +1453,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
err = -EINVAL;
out_err:
ClearPageUptodate(page);
+ f2fs_handle_page_eio(sbi, page->index, NODE);
f2fs_put_page(page, 1);
return ERR_PTR(err);
}
--
2.36.0.rc0.470.gd361397f0d-goog

2022-04-18 12:38:39

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid infinite loop to flush node pages

On 2022/4/15 6:27, Jaegeuk Kim wrote:
> xfstests/generic/475 can give EIO all the time which give an infinite loop
> to flush node page like below. Let's avoid it.
>
> [16418.518551] Call Trace:
> [16418.518553] ? dm_submit_bio+0x48/0x400
> [16418.518574] ? submit_bio_checks+0x1ac/0x5a0
> [16418.525207] __submit_bio+0x1a9/0x230
> [16418.525210] ? kmem_cache_alloc+0x29e/0x3c0
> [16418.525223] submit_bio_noacct+0xa8/0x2b0
> [16418.525226] submit_bio+0x4d/0x130
> [16418.525238] __submit_bio+0x49/0x310 [f2fs]
> [16418.525339] ? bio_add_page+0x6a/0x90
> [16418.525344] f2fs_submit_page_bio+0x134/0x1f0 [f2fs]
> [16418.525365] read_node_page+0x125/0x1b0 [f2fs]
> [16418.525388] __get_node_page.part.0+0x58/0x3f0 [f2fs]
> [16418.525409] __get_node_page+0x2f/0x60 [f2fs]
> [16418.525431] f2fs_get_dnode_of_data+0x423/0x860 [f2fs]
> [16418.525452] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
> [16418.525458] ? __mod_memcg_state.part.0+0x2a/0x30
> [16418.525465] ? __mod_memcg_lruvec_state+0x27/0x40
> [16418.525467] ? __xa_set_mark+0x57/0x70
> [16418.525472] f2fs_do_write_data_page+0x10e/0x7b0 [f2fs]
> [16418.525493] f2fs_write_single_data_page+0x555/0x830 [f2fs]
> [16418.525514] ? sysvec_apic_timer_interrupt+0x4e/0x90
> [16418.525518] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
> [16418.525523] f2fs_write_cache_pages+0x303/0x880 [f2fs]
> [16418.525545] ? blk_flush_plug_list+0x47/0x100
> [16418.525548] f2fs_write_data_pages+0xfd/0x320 [f2fs]
> [16418.525569] do_writepages+0xd5/0x210
> [16418.525648] filemap_fdatawrite_wbc+0x7d/0xc0
> [16418.525655] filemap_fdatawrite+0x50/0x70
> [16418.525658] f2fs_sync_dirty_inodes+0xa4/0x230 [f2fs]
> [16418.525679] f2fs_write_checkpoint+0x16d/0x1720 [f2fs]
> [16418.525699] ? ttwu_do_wakeup+0x1c/0x160
> [16418.525709] ? ttwu_do_activate+0x6d/0xd0
> [16418.525711] ? __wait_for_common+0x11d/0x150
> [16418.525715] kill_f2fs_super+0xca/0x100 [f2fs]
> [16418.525733] deactivate_locked_super+0x3b/0xb0
> [16418.525739] deactivate_super+0x40/0x50
> [16418.525741] cleanup_mnt+0x139/0x190
> [16418.525747] __cleanup_mnt+0x12/0x20
> [16418.525749] task_work_run+0x6d/0xa0
> [16418.525765] exit_to_user_mode_prepare+0x1ad/0x1b0
> [16418.525771] syscall_exit_to_user_mode+0x27/0x50
> [16418.525774] do_syscall_64+0x48/0xc0
> [16418.525776] entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
>
> Change log from v1:
> - cover one more error case
>
> fs/f2fs/checkpoint.c | 8 +-------
> fs/f2fs/f2fs.h | 20 ++++++++++++++++----
> fs/f2fs/node.c | 2 ++
> 3 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 909085a78f9c..319903c2b34f 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -98,13 +98,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
> }
>
> if (unlikely(!PageUptodate(page))) {
> - if (page->index == sbi->metapage_eio_ofs) {
> - if (sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO)
> - set_ckpt_flags(sbi, CP_ERROR_FLAG);
> - } else {
> - sbi->metapage_eio_ofs = page->index;
> - sbi->metapage_eio_cnt = 0;
> - }
> + f2fs_handle_page_eio(sbi, page->index, META);
> f2fs_put_page(page, 1);
> return ERR_PTR(-EIO);
> }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index cd1e65bcf0b0..977826a22568 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -579,8 +579,8 @@ enum {
> /* maximum retry quota flush count */
> #define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8
>
> -/* maximum retry of EIO'ed meta page */
> -#define MAX_RETRY_META_PAGE_EIO 100
> +/* maximum retry of EIO'ed page */
> +#define MAX_RETRY_PAGE_EIO 100
>
> #define F2FS_LINK_MAX 0xffffffff /* maximum link count per file */
>
> @@ -1621,8 +1621,8 @@ struct f2fs_sb_info {
> /* keep migration IO order for LFS mode */
> struct f2fs_rwsem io_order_lock;
> mempool_t *write_io_dummy; /* Dummy pages */
> - pgoff_t metapage_eio_ofs; /* EIO page offset */
> - int metapage_eio_cnt; /* EIO count */
> + pgoff_t page_eio_ofs[NR_PAGE_TYPE]; /* EIO page offset */
> + int page_eio_cnt[NR_PAGE_TYPE]; /* EIO count */
>
> /* for checkpoint */
> struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */
> @@ -4543,6 +4543,18 @@ static inline void f2fs_io_schedule_timeout(long timeout)
> io_schedule_timeout(timeout);
> }
>
> +static inline void f2fs_handle_page_eio(struct f2fs_sb_info *sbi, pgoff_t ofs,
> + enum page_type type)
> +{

If we have already set cp_error, it doesn't need to process below logic
redundantly?

if (f2fs_cp_error())
return;

> + if (ofs == sbi->page_eio_ofs[type]) {
> + if (sbi->page_eio_cnt[type]++ == MAX_RETRY_PAGE_EIO)
> + set_ckpt_flags(sbi, CP_ERROR_FLAG);
> + } else {
> + sbi->page_eio_ofs[type] = ofs;
> + sbi->page_eio_cnt[type] = 0;
> + }
> +}
> +
> #define EFSBADCRC EBADMSG /* Bad CRC detected */
> #define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index c45d341dcf6e..c280f482c741 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1416,6 +1416,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
>
> err = read_node_page(page, 0);
> if (err < 0) {
> + f2fs_handle_page_eio(sbi, page->index, NODE);

How about:

goto out_put;

> f2fs_put_page(page, 1);
> return ERR_PTR(err);
> } else if (err == LOCKED_PAGE) {
> @@ -1452,6 +1453,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
> err = -EINVAL;
> out_err:
> ClearPageUptodate(page);

out_put:

Thanks,

> + f2fs_handle_page_eio(sbi, page->index, NODE);
> f2fs_put_page(page, 1);
> return ERR_PTR(err);
> }

2022-04-18 12:47:48

by Chao Yu

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: fix wrong condition check when failing metapage read

On 2022/4/12 5:21, Jaegeuk Kim wrote:
> This patch fixes wrong initialization.
>
> Fixes: 50c63009f6ab ("f2fs: avoid an infinite loop in f2fs_sync_dirty_inodes")
> Signed-off-by: Jaegeuk Kim <[email protected]>

Reviewed-by: Chao Yu <[email protected]>

Thanks,

2022-04-21 12:54:40

by Jaegeuk Kim

[permalink] [raw]
Subject: Re: [f2fs-dev] [PATCH 2/2 v3] f2fs: avoid infinite loop to flush node pages

xfstests/generic/475 can give EIO all the time which give an infinite loop
to flush node page like below. Let's avoid it.

[16418.518551] Call Trace:
[16418.518553] ? dm_submit_bio+0x48/0x400
[16418.518574] ? submit_bio_checks+0x1ac/0x5a0
[16418.525207] __submit_bio+0x1a9/0x230
[16418.525210] ? kmem_cache_alloc+0x29e/0x3c0
[16418.525223] submit_bio_noacct+0xa8/0x2b0
[16418.525226] submit_bio+0x4d/0x130
[16418.525238] __submit_bio+0x49/0x310 [f2fs]
[16418.525339] ? bio_add_page+0x6a/0x90
[16418.525344] f2fs_submit_page_bio+0x134/0x1f0 [f2fs]
[16418.525365] read_node_page+0x125/0x1b0 [f2fs]
[16418.525388] __get_node_page.part.0+0x58/0x3f0 [f2fs]
[16418.525409] __get_node_page+0x2f/0x60 [f2fs]
[16418.525431] f2fs_get_dnode_of_data+0x423/0x860 [f2fs]
[16418.525452] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525458] ? __mod_memcg_state.part.0+0x2a/0x30
[16418.525465] ? __mod_memcg_lruvec_state+0x27/0x40
[16418.525467] ? __xa_set_mark+0x57/0x70
[16418.525472] f2fs_do_write_data_page+0x10e/0x7b0 [f2fs]
[16418.525493] f2fs_write_single_data_page+0x555/0x830 [f2fs]
[16418.525514] ? sysvec_apic_timer_interrupt+0x4e/0x90
[16418.525518] ? asm_sysvec_apic_timer_interrupt+0x12/0x20
[16418.525523] f2fs_write_cache_pages+0x303/0x880 [f2fs]
[16418.525545] ? blk_flush_plug_list+0x47/0x100
[16418.525548] f2fs_write_data_pages+0xfd/0x320 [f2fs]
[16418.525569] do_writepages+0xd5/0x210
[16418.525648] filemap_fdatawrite_wbc+0x7d/0xc0
[16418.525655] filemap_fdatawrite+0x50/0x70
[16418.525658] f2fs_sync_dirty_inodes+0xa4/0x230 [f2fs]
[16418.525679] f2fs_write_checkpoint+0x16d/0x1720 [f2fs]
[16418.525699] ? ttwu_do_wakeup+0x1c/0x160
[16418.525709] ? ttwu_do_activate+0x6d/0xd0
[16418.525711] ? __wait_for_common+0x11d/0x150
[16418.525715] kill_f2fs_super+0xca/0x100 [f2fs]
[16418.525733] deactivate_locked_super+0x3b/0xb0
[16418.525739] deactivate_super+0x40/0x50
[16418.525741] cleanup_mnt+0x139/0x190
[16418.525747] __cleanup_mnt+0x12/0x20
[16418.525749] task_work_run+0x6d/0xa0
[16418.525765] exit_to_user_mode_prepare+0x1ad/0x1b0
[16418.525771] syscall_exit_to_user_mode+0x27/0x50
[16418.525774] do_syscall_64+0x48/0xc0
[16418.525776] entry_SYSCALL_64_after_hwframe+0x44/0xae

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

Change log from v2:
- minor code refactoring

fs/f2fs/checkpoint.c | 8 +-------
fs/f2fs/f2fs.h | 23 +++++++++++++++++++----
fs/f2fs/node.c | 23 ++++++++++++-----------
3 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 71b1e93cbe0c..beceac9885c3 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -98,13 +98,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index,
}

if (unlikely(!PageUptodate(page))) {
- if (page->index == sbi->metapage_eio_ofs) {
- if (sbi->metapage_eio_cnt++ == MAX_RETRY_META_PAGE_EIO)
- set_ckpt_flags(sbi, CP_ERROR_FLAG);
- } else {
- sbi->metapage_eio_ofs = page->index;
- sbi->metapage_eio_cnt = 0;
- }
+ f2fs_handle_page_eio(sbi, page->index, META);
f2fs_put_page(page, 1);
return ERR_PTR(-EIO);
}
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 8c570de21ed5..f3eda4f13646 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -578,8 +578,8 @@ enum {
/* maximum retry quota flush count */
#define DEFAULT_RETRY_QUOTA_FLUSH_COUNT 8

-/* maximum retry of EIO'ed meta page */
-#define MAX_RETRY_META_PAGE_EIO 100
+/* maximum retry of EIO'ed page */
+#define MAX_RETRY_PAGE_EIO 100

#define F2FS_LINK_MAX 0xffffffff /* maximum link count per file */

@@ -1614,8 +1614,8 @@ struct f2fs_sb_info {
/* keep migration IO order for LFS mode */
struct f2fs_rwsem io_order_lock;
mempool_t *write_io_dummy; /* Dummy pages */
- pgoff_t metapage_eio_ofs; /* EIO page offset */
- int metapage_eio_cnt; /* EIO count */
+ pgoff_t page_eio_ofs[NR_PAGE_TYPE]; /* EIO page offset */
+ int page_eio_cnt[NR_PAGE_TYPE]; /* EIO count */

/* for checkpoint */
struct f2fs_checkpoint *ckpt; /* raw checkpoint pointer */
@@ -4534,6 +4534,21 @@ static inline void f2fs_io_schedule_timeout(long timeout)
io_schedule_timeout(timeout);
}

+static inline void f2fs_handle_page_eio(struct f2fs_sb_info *sbi, pgoff_t ofs,
+ enum page_type type)
+{
+ if (unlikely(f2fs_cp_error(sbi)))
+ return;
+
+ if (ofs == sbi->page_eio_ofs[type]) {
+ if (sbi->page_eio_cnt[type]++ == MAX_RETRY_PAGE_EIO)
+ set_ckpt_flags(sbi, CP_ERROR_FLAG);
+ } else {
+ sbi->page_eio_ofs[type] = ofs;
+ sbi->page_eio_cnt[type] = 0;
+ }
+}
+
#define EFSBADCRC EBADMSG /* Bad CRC detected */
#define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 144f9f966690..51230cba841b 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1416,8 +1416,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,

err = read_node_page(page, 0);
if (err < 0) {
- f2fs_put_page(page, 1);
- return ERR_PTR(err);
+ goto out_put_err;
} else if (err == LOCKED_PAGE) {
err = 0;
goto page_hit;
@@ -1443,19 +1442,21 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
goto out_err;
}
page_hit:
- if (unlikely(nid != nid_of_node(page))) {
- f2fs_warn(sbi, "inconsistent node block, nid:%lu, node_footer[nid:%u,ino:%u,ofs:%u,cpver:%llu,blkaddr:%u]",
+ if (likely(nid == nid_of_node(page)))
+ return page;
+
+ f2fs_warn(sbi, "inconsistent node block, nid:%lu, node_footer[nid:%u,ino:%u,ofs:%u,cpver:%llu,blkaddr:%u]",
nid, nid_of_node(page), ino_of_node(page),
ofs_of_node(page), cpver_of_node(page),
next_blkaddr_of_node(page));
- set_sbi_flag(sbi, SBI_NEED_FSCK);
- err = -EINVAL;
+ set_sbi_flag(sbi, SBI_NEED_FSCK);
+ err = -EINVAL;
out_err:
- ClearPageUptodate(page);
- f2fs_put_page(page, 1);
- return ERR_PTR(err);
- }
- return page;
+ ClearPageUptodate(page);
+out_put_err:
+ f2fs_handle_page_eio(sbi, page->index, NODE);
+ f2fs_put_page(page, 1);
+ return ERR_PTR(err);
}

struct page *f2fs_get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid)
--
2.36.0.rc0.470.gd361397f0d-goog