* [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() @ 2018-07-10 15:00 Chao Yu 2018-07-15 1:57 ` Jaegeuk Kim 0 siblings, 1 reply; 4+ messages in thread From: Chao Yu @ 2018-07-10 15:00 UTC (permalink / raw) To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu From: Chao Yu <yuchao0@huawei.com> If caller of __get_meta_page() can handle error, let's propagate error from __get_meta_page(). Signed-off-by: Chao Yu <yuchao0@huawei.com> --- v2: - handle error path in __get_meta_page() correctly. fs/f2fs/checkpoint.c | 21 +++++++++++++++++---- fs/f2fs/f2fs.h | 2 +- fs/f2fs/node.c | 27 +++++++++++++++++++-------- fs/f2fs/recovery.c | 8 ++++++++ fs/f2fs/segment.c | 33 +++++++++++++++++++++++++++------ 5 files changed, 72 insertions(+), 19 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 41e6e5769a2c..b672fc1cdb05 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -71,6 +71,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, .encrypted_page = NULL, .is_meta = is_meta, }; + int err; if (unlikely(!is_meta)) fio.op_flags &= ~REQ_META; @@ -85,11 +86,12 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, fio.page = page; - if (f2fs_submit_page_bio(&fio)) { + err = f2fs_submit_page_bio(&fio); + if (err) { memset(page_address(page), 0, PAGE_SIZE); + f2fs_put_page(page, 1); f2fs_stop_checkpoint(sbi, false); - f2fs_bug_on(sbi, 1); - return page; + return ERR_PTR(err); } lock_page(page); @@ -658,9 +660,15 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi) f2fs_ra_meta_pages(sbi, start_blk, orphan_blocks, META_CP, true); for (i = 0; i < orphan_blocks; i++) { - struct page *page = f2fs_get_meta_page(sbi, start_blk + i); + struct page *page; struct f2fs_orphan_block *orphan_blk; + page = f2fs_get_meta_page(sbi, start_blk + i); + if (IS_ERR(page)) { + err = PTR_ERR(page); + goto out; + } + orphan_blk = (struct f2fs_orphan_block *)page_address(page); for (j = 0; j < le32_to_cpu(orphan_blk->entry_count); j++) { nid_t ino = le32_to_cpu(orphan_blk->ino[j]); @@ -751,6 +759,9 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr, __u32 crc = 0; *cp_page = f2fs_get_meta_page(sbi, cp_addr); + if (IS_ERR(*cp_page)) + return PTR_ERR(*cp_page); + *cp_block = (struct f2fs_checkpoint *)page_address(*cp_page); crc_offset = le32_to_cpu((*cp_block)->checksum_offset); @@ -876,6 +887,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi) unsigned char *ckpt = (unsigned char *)sbi->ckpt; cur_page = f2fs_get_meta_page(sbi, cp_blk_no + i); + if (IS_ERR(cur_page)) + goto free_fail_no_cp; sit_bitmap_ptr = page_address(cur_page); memcpy(ckpt + i * blk_size, sit_bitmap_ptr, blk_size); f2fs_put_page(cur_page, 1); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 359526d88d3f..76b51a0de3cd 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -2864,7 +2864,7 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink); void f2fs_recover_inline_xattr(struct inode *inode, struct page *page); int f2fs_recover_xattr_data(struct inode *inode, struct page *page); int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page); -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, unsigned int segno, struct f2fs_summary_block *sum); void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc); int f2fs_build_node_manager(struct f2fs_sb_info *sbi); diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index c48e2a2e5e82..248d401bf40a 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -113,25 +113,26 @@ static void clear_node_page_dirty(struct page *page) static struct page *get_current_nat_page(struct f2fs_sb_info *sbi, nid_t nid) { - pgoff_t index = current_nat_addr(sbi, nid); - return f2fs_get_meta_page(sbi, index); + struct page *page; + + page = f2fs_get_meta_page(sbi, current_nat_addr(sbi, nid)); + BUG_ON(IS_ERR(page)); + return page; } static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid) { struct page *src_page; struct page *dst_page; - pgoff_t src_off; pgoff_t dst_off; void *src_addr; void *dst_addr; struct f2fs_nm_info *nm_i = NM_I(sbi); - src_off = current_nat_addr(sbi, nid); - dst_off = next_nat_addr(sbi, src_off); + dst_off = next_nat_addr(sbi, current_nat_addr(sbi, nid)); /* get current nat block page with lock */ - src_page = f2fs_get_meta_page(sbi, src_off); + src_page = get_current_nat_page(sbi, nid); dst_page = f2fs_grab_meta_page(sbi, dst_off); f2fs_bug_on(sbi, PageDirty(src_page)); @@ -2502,7 +2503,7 @@ int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page) return 0; } -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, unsigned int segno, struct f2fs_summary_block *sum) { struct f2fs_node *rn; @@ -2524,6 +2525,9 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, for (idx = addr; idx < addr + nrpages; idx++) { struct page *page = f2fs_get_tmp_page(sbi, idx); + if (IS_ERR(page)) + return PTR_ERR(page); + rn = F2FS_NODE(page); sum_entry->nid = rn->footer.nid; sum_entry->version = 0; @@ -2535,6 +2539,7 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, invalidate_mapping_pages(META_MAPPING(sbi), addr, addr + nrpages); } + return 0; } static void remove_nats_in_journal(struct f2fs_sb_info *sbi) @@ -2771,7 +2776,13 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi) nat_bits_addr = __start_cp_addr(sbi) + sbi->blocks_per_seg - nm_i->nat_bits_blocks; for (i = 0; i < nm_i->nat_bits_blocks; i++) { - struct page *page = f2fs_get_meta_page(sbi, nat_bits_addr++); + struct page *page; + + page = f2fs_get_meta_page(sbi, nat_bits_addr++); + if (IS_ERR(page)) { + disable_nat_bits(sbi, true); + return PTR_ERR(page); + } memcpy(nm_i->nat_bits + (i << F2FS_BLKSIZE_BITS), page_address(page), F2FS_BLKSIZE); diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c index 0d927ae26c48..ac70d5547a97 100644 --- a/fs/f2fs/recovery.c +++ b/fs/f2fs/recovery.c @@ -256,6 +256,10 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head, return 0; page = f2fs_get_tmp_page(sbi, blkaddr); + if (IS_ERR(page)) { + err = PTR_ERR(page); + break; + } if (!is_recoverable_dnode(page)) break; @@ -574,6 +578,10 @@ static int recover_data(struct f2fs_sb_info *sbi, struct list_head *inode_list, f2fs_ra_meta_pages_cond(sbi, blkaddr); page = f2fs_get_tmp_page(sbi, blkaddr); + if (IS_ERR(page)) { + err = PTR_ERR(page); + break; + } if (!is_recoverable_dnode(page)) { f2fs_put_page(page, 1); diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index e55188975fcc..22600002c690 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -2096,7 +2096,11 @@ int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra) */ struct page *f2fs_get_sum_page(struct f2fs_sb_info *sbi, unsigned int segno) { - return f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); + struct page *page; + + page = f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); + BUG_ON(IS_ERR(page)); + return page; } void f2fs_update_meta_page(struct f2fs_sb_info *sbi, @@ -3122,7 +3126,7 @@ void f2fs_wait_on_block_writeback(struct f2fs_sb_info *sbi, block_t blkaddr) } } -static void read_compacted_summaries(struct f2fs_sb_info *sbi) +static int read_compacted_summaries(struct f2fs_sb_info *sbi) { struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); struct curseg_info *seg_i; @@ -3134,6 +3138,8 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) start = start_sum_block(sbi); page = f2fs_get_meta_page(sbi, start++); + if (IS_ERR(page)) + return PTR_ERR(page); kaddr = (unsigned char *)page_address(page); /* Step 1: restore nat cache */ @@ -3174,11 +3180,14 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) page = NULL; page = f2fs_get_meta_page(sbi, start++); + if (IS_ERR(page)) + return PTR_ERR(page); kaddr = (unsigned char *)page_address(page); offset = 0; } } f2fs_put_page(page, 1); + return 0; } static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) @@ -3190,6 +3199,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) unsigned short blk_off; unsigned int segno = 0; block_t blk_addr = 0; + int err = 0; /* get segment number and block addr */ if (IS_DATASEG(type)) { @@ -3213,6 +3223,8 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) } new = f2fs_get_meta_page(sbi, blk_addr); + if (IS_ERR(new)) + return PTR_ERR(new); sum = (struct f2fs_summary_block *)page_address(new); if (IS_NODESEG(type)) { @@ -3224,7 +3236,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) ns->ofs_in_node = 0; } } else { - f2fs_restore_node_summary(sbi, segno, sum); + err = f2fs_restore_node_summary(sbi, segno, sum); + if (err) + goto out; } } @@ -3244,8 +3258,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) curseg->alloc_type = ckpt->alloc_type[type]; curseg->next_blkoff = blk_off; mutex_unlock(&curseg->curseg_mutex); +out: f2fs_put_page(new, 1); - return 0; + return err; } static int restore_curseg_summaries(struct f2fs_sb_info *sbi) @@ -3263,7 +3278,9 @@ static int restore_curseg_summaries(struct f2fs_sb_info *sbi) META_CP, true); /* restore for compacted data summary */ - read_compacted_summaries(sbi); + err = read_compacted_summaries(sbi); + if (err) + return err; type = CURSEG_HOT_NODE; } @@ -3394,7 +3411,11 @@ int f2fs_lookup_journal_in_cursum(struct f2fs_journal *journal, int type, static struct page *get_current_sit_page(struct f2fs_sb_info *sbi, unsigned int segno) { - return f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); + struct page *page; + + page = f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); + BUG_ON(IS_ERR(page)); + return page; } static struct page *get_next_sit_page(struct f2fs_sb_info *sbi, -- 2.16.2.17.g38e79b1fd ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() 2018-07-10 15:00 [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() Chao Yu @ 2018-07-15 1:57 ` Jaegeuk Kim 2018-07-15 2:21 ` Chao Yu 0 siblings, 1 reply; 4+ messages in thread From: Jaegeuk Kim @ 2018-07-15 1:57 UTC (permalink / raw) To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel, Chao Yu On 07/10, Chao Yu wrote: > From: Chao Yu <yuchao0@huawei.com> > > If caller of __get_meta_page() can handle error, let's propagate error > from __get_meta_page(). > > Signed-off-by: Chao Yu <yuchao0@huawei.com> > --- > v2: > - handle error path in __get_meta_page() correctly. > fs/f2fs/checkpoint.c | 21 +++++++++++++++++---- > fs/f2fs/f2fs.h | 2 +- > fs/f2fs/node.c | 27 +++++++++++++++++++-------- > fs/f2fs/recovery.c | 8 ++++++++ > fs/f2fs/segment.c | 33 +++++++++++++++++++++++++++------ > 5 files changed, 72 insertions(+), 19 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 41e6e5769a2c..b672fc1cdb05 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -71,6 +71,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, > .encrypted_page = NULL, > .is_meta = is_meta, > }; > + int err; > > if (unlikely(!is_meta)) > fio.op_flags &= ~REQ_META; > @@ -85,11 +86,12 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, > > fio.page = page; > > - if (f2fs_submit_page_bio(&fio)) { > + err = f2fs_submit_page_bio(&fio); > + if (err) { > memset(page_address(page), 0, PAGE_SIZE); No reason to do memset(), if we handle an error? > + f2fs_put_page(page, 1); > f2fs_stop_checkpoint(sbi, false); > - f2fs_bug_on(sbi, 1); Keep to f2fs_bug_on(1)? > - return page; > + return ERR_PTR(err); > } > > lock_page(page); Need to return an error if read_end_io is failed below in the path. > @@ -658,9 +660,15 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi) > f2fs_ra_meta_pages(sbi, start_blk, orphan_blocks, META_CP, true); > > for (i = 0; i < orphan_blocks; i++) { > - struct page *page = f2fs_get_meta_page(sbi, start_blk + i); > + struct page *page; > struct f2fs_orphan_block *orphan_blk; > > + page = f2fs_get_meta_page(sbi, start_blk + i); > + if (IS_ERR(page)) { > + err = PTR_ERR(page); > + goto out; > + } > + > orphan_blk = (struct f2fs_orphan_block *)page_address(page); > for (j = 0; j < le32_to_cpu(orphan_blk->entry_count); j++) { > nid_t ino = le32_to_cpu(orphan_blk->ino[j]); > @@ -751,6 +759,9 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr, > __u32 crc = 0; > > *cp_page = f2fs_get_meta_page(sbi, cp_addr); > + if (IS_ERR(*cp_page)) > + return PTR_ERR(*cp_page); > + > *cp_block = (struct f2fs_checkpoint *)page_address(*cp_page); > > crc_offset = le32_to_cpu((*cp_block)->checksum_offset); > @@ -876,6 +887,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi) > unsigned char *ckpt = (unsigned char *)sbi->ckpt; > > cur_page = f2fs_get_meta_page(sbi, cp_blk_no + i); > + if (IS_ERR(cur_page)) > + goto free_fail_no_cp; > sit_bitmap_ptr = page_address(cur_page); > memcpy(ckpt + i * blk_size, sit_bitmap_ptr, blk_size); > f2fs_put_page(cur_page, 1); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 359526d88d3f..76b51a0de3cd 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -2864,7 +2864,7 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink); > void f2fs_recover_inline_xattr(struct inode *inode, struct page *page); > int f2fs_recover_xattr_data(struct inode *inode, struct page *page); > int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page); > -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > unsigned int segno, struct f2fs_summary_block *sum); > void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc); > int f2fs_build_node_manager(struct f2fs_sb_info *sbi); > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index c48e2a2e5e82..248d401bf40a 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -113,25 +113,26 @@ static void clear_node_page_dirty(struct page *page) > > static struct page *get_current_nat_page(struct f2fs_sb_info *sbi, nid_t nid) > { > - pgoff_t index = current_nat_addr(sbi, nid); > - return f2fs_get_meta_page(sbi, index); > + struct page *page; > + > + page = f2fs_get_meta_page(sbi, current_nat_addr(sbi, nid)); > + BUG_ON(IS_ERR(page)); Do we need this at this moment? > + return page; > } > > static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid) > { > struct page *src_page; > struct page *dst_page; > - pgoff_t src_off; > pgoff_t dst_off; > void *src_addr; > void *dst_addr; > struct f2fs_nm_info *nm_i = NM_I(sbi); > > - src_off = current_nat_addr(sbi, nid); > - dst_off = next_nat_addr(sbi, src_off); > + dst_off = next_nat_addr(sbi, current_nat_addr(sbi, nid)); > > /* get current nat block page with lock */ > - src_page = f2fs_get_meta_page(sbi, src_off); > + src_page = get_current_nat_page(sbi, nid); This is a different clean-up change. > dst_page = f2fs_grab_meta_page(sbi, dst_off); > f2fs_bug_on(sbi, PageDirty(src_page)); > > @@ -2502,7 +2503,7 @@ int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page) > return 0; > } > > -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > unsigned int segno, struct f2fs_summary_block *sum) > { > struct f2fs_node *rn; > @@ -2524,6 +2525,9 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > for (idx = addr; idx < addr + nrpages; idx++) { > struct page *page = f2fs_get_tmp_page(sbi, idx); > > + if (IS_ERR(page)) > + return PTR_ERR(page); > + > rn = F2FS_NODE(page); > sum_entry->nid = rn->footer.nid; > sum_entry->version = 0; > @@ -2535,6 +2539,7 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > invalidate_mapping_pages(META_MAPPING(sbi), addr, > addr + nrpages); > } > + return 0; > } > > static void remove_nats_in_journal(struct f2fs_sb_info *sbi) > @@ -2771,7 +2776,13 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi) > nat_bits_addr = __start_cp_addr(sbi) + sbi->blocks_per_seg - > nm_i->nat_bits_blocks; > for (i = 0; i < nm_i->nat_bits_blocks; i++) { > - struct page *page = f2fs_get_meta_page(sbi, nat_bits_addr++); > + struct page *page; > + > + page = f2fs_get_meta_page(sbi, nat_bits_addr++); > + if (IS_ERR(page)) { > + disable_nat_bits(sbi, true); > + return PTR_ERR(page); > + } > > memcpy(nm_i->nat_bits + (i << F2FS_BLKSIZE_BITS), > page_address(page), F2FS_BLKSIZE); > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c > index 0d927ae26c48..ac70d5547a97 100644 > --- a/fs/f2fs/recovery.c > +++ b/fs/f2fs/recovery.c > @@ -256,6 +256,10 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head, > return 0; > > page = f2fs_get_tmp_page(sbi, blkaddr); > + if (IS_ERR(page)) { > + err = PTR_ERR(page); > + break; > + } > > if (!is_recoverable_dnode(page)) > break; > @@ -574,6 +578,10 @@ static int recover_data(struct f2fs_sb_info *sbi, struct list_head *inode_list, > f2fs_ra_meta_pages_cond(sbi, blkaddr); > > page = f2fs_get_tmp_page(sbi, blkaddr); > + if (IS_ERR(page)) { > + err = PTR_ERR(page); > + break; > + } > > if (!is_recoverable_dnode(page)) { > f2fs_put_page(page, 1); > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index e55188975fcc..22600002c690 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -2096,7 +2096,11 @@ int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra) > */ > struct page *f2fs_get_sum_page(struct f2fs_sb_info *sbi, unsigned int segno) > { > - return f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); > + struct page *page; > + > + page = f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); > + BUG_ON(IS_ERR(page)); Ditto. > + return page; > } > > void f2fs_update_meta_page(struct f2fs_sb_info *sbi, > @@ -3122,7 +3126,7 @@ void f2fs_wait_on_block_writeback(struct f2fs_sb_info *sbi, block_t blkaddr) > } > } > > -static void read_compacted_summaries(struct f2fs_sb_info *sbi) > +static int read_compacted_summaries(struct f2fs_sb_info *sbi) > { > struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); > struct curseg_info *seg_i; > @@ -3134,6 +3138,8 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) > start = start_sum_block(sbi); > > page = f2fs_get_meta_page(sbi, start++); > + if (IS_ERR(page)) > + return PTR_ERR(page); > kaddr = (unsigned char *)page_address(page); > > /* Step 1: restore nat cache */ > @@ -3174,11 +3180,14 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) > page = NULL; > > page = f2fs_get_meta_page(sbi, start++); > + if (IS_ERR(page)) > + return PTR_ERR(page); > kaddr = (unsigned char *)page_address(page); > offset = 0; > } > } > f2fs_put_page(page, 1); > + return 0; > } > > static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > @@ -3190,6 +3199,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > unsigned short blk_off; > unsigned int segno = 0; > block_t blk_addr = 0; > + int err = 0; > > /* get segment number and block addr */ > if (IS_DATASEG(type)) { > @@ -3213,6 +3223,8 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > } > > new = f2fs_get_meta_page(sbi, blk_addr); > + if (IS_ERR(new)) > + return PTR_ERR(new); > sum = (struct f2fs_summary_block *)page_address(new); > > if (IS_NODESEG(type)) { > @@ -3224,7 +3236,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > ns->ofs_in_node = 0; > } > } else { > - f2fs_restore_node_summary(sbi, segno, sum); > + err = f2fs_restore_node_summary(sbi, segno, sum); > + if (err) > + goto out; > } > } > > @@ -3244,8 +3258,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > curseg->alloc_type = ckpt->alloc_type[type]; > curseg->next_blkoff = blk_off; > mutex_unlock(&curseg->curseg_mutex); > +out: > f2fs_put_page(new, 1); > - return 0; > + return err; > } > > static int restore_curseg_summaries(struct f2fs_sb_info *sbi) > @@ -3263,7 +3278,9 @@ static int restore_curseg_summaries(struct f2fs_sb_info *sbi) > META_CP, true); > > /* restore for compacted data summary */ > - read_compacted_summaries(sbi); > + err = read_compacted_summaries(sbi); > + if (err) > + return err; > type = CURSEG_HOT_NODE; > } > > @@ -3394,7 +3411,11 @@ int f2fs_lookup_journal_in_cursum(struct f2fs_journal *journal, int type, > static struct page *get_current_sit_page(struct f2fs_sb_info *sbi, > unsigned int segno) > { > - return f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); > + struct page *page; > + > + page = f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); > + BUG_ON(IS_ERR(page)); Ditoo. > + return page; > } > > static struct page *get_next_sit_page(struct f2fs_sb_info *sbi, > -- > 2.16.2.17.g38e79b1fd ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() 2018-07-15 1:57 ` Jaegeuk Kim @ 2018-07-15 2:21 ` Chao Yu 2018-07-15 3:04 ` Jaegeuk Kim 0 siblings, 1 reply; 4+ messages in thread From: Chao Yu @ 2018-07-15 2:21 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel On 2018/7/15 9:57, Jaegeuk Kim wrote: > On 07/10, Chao Yu wrote: >> From: Chao Yu <yuchao0@huawei.com> >> >> If caller of __get_meta_page() can handle error, let's propagate error >> from __get_meta_page(). >> >> Signed-off-by: Chao Yu <yuchao0@huawei.com> >> --- >> v2: >> - handle error path in __get_meta_page() correctly. >> fs/f2fs/checkpoint.c | 21 +++++++++++++++++---- >> fs/f2fs/f2fs.h | 2 +- >> fs/f2fs/node.c | 27 +++++++++++++++++++-------- >> fs/f2fs/recovery.c | 8 ++++++++ >> fs/f2fs/segment.c | 33 +++++++++++++++++++++++++++------ >> 5 files changed, 72 insertions(+), 19 deletions(-) >> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >> index 41e6e5769a2c..b672fc1cdb05 100644 >> --- a/fs/f2fs/checkpoint.c >> +++ b/fs/f2fs/checkpoint.c >> @@ -71,6 +71,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, >> .encrypted_page = NULL, >> .is_meta = is_meta, >> }; >> + int err; >> >> if (unlikely(!is_meta)) >> fio.op_flags &= ~REQ_META; >> @@ -85,11 +86,12 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, >> >> fio.page = page; >> >> - if (f2fs_submit_page_bio(&fio)) { >> + err = f2fs_submit_page_bio(&fio); >> + if (err) { >> memset(page_address(page), 0, PAGE_SIZE); > > No reason to do memset(), if we handle an error? Agreed. > >> + f2fs_put_page(page, 1); >> f2fs_stop_checkpoint(sbi, false); >> - f2fs_bug_on(sbi, 1); > > Keep to f2fs_bug_on(1)? If we want to report error return value to caller, we should not let system panic here? > >> - return page; >> + return ERR_PTR(err); >> } >> >> lock_page(page); > > Need to return an error if read_end_io is failed below in the path. Agreed, previously, I found that if we propagate the error to get_node_info(), it will be a little hard to handle, anyway, let me try. > >> @@ -658,9 +660,15 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi) >> f2fs_ra_meta_pages(sbi, start_blk, orphan_blocks, META_CP, true); >> >> for (i = 0; i < orphan_blocks; i++) { >> - struct page *page = f2fs_get_meta_page(sbi, start_blk + i); >> + struct page *page; >> struct f2fs_orphan_block *orphan_blk; >> >> + page = f2fs_get_meta_page(sbi, start_blk + i); >> + if (IS_ERR(page)) { >> + err = PTR_ERR(page); >> + goto out; >> + } >> + >> orphan_blk = (struct f2fs_orphan_block *)page_address(page); >> for (j = 0; j < le32_to_cpu(orphan_blk->entry_count); j++) { >> nid_t ino = le32_to_cpu(orphan_blk->ino[j]); >> @@ -751,6 +759,9 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr, >> __u32 crc = 0; >> >> *cp_page = f2fs_get_meta_page(sbi, cp_addr); >> + if (IS_ERR(*cp_page)) >> + return PTR_ERR(*cp_page); >> + >> *cp_block = (struct f2fs_checkpoint *)page_address(*cp_page); >> >> crc_offset = le32_to_cpu((*cp_block)->checksum_offset); >> @@ -876,6 +887,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi) >> unsigned char *ckpt = (unsigned char *)sbi->ckpt; >> >> cur_page = f2fs_get_meta_page(sbi, cp_blk_no + i); >> + if (IS_ERR(cur_page)) >> + goto free_fail_no_cp; >> sit_bitmap_ptr = page_address(cur_page); >> memcpy(ckpt + i * blk_size, sit_bitmap_ptr, blk_size); >> f2fs_put_page(cur_page, 1); >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >> index 359526d88d3f..76b51a0de3cd 100644 >> --- a/fs/f2fs/f2fs.h >> +++ b/fs/f2fs/f2fs.h >> @@ -2864,7 +2864,7 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink); >> void f2fs_recover_inline_xattr(struct inode *inode, struct page *page); >> int f2fs_recover_xattr_data(struct inode *inode, struct page *page); >> int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page); >> -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> unsigned int segno, struct f2fs_summary_block *sum); >> void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc); >> int f2fs_build_node_manager(struct f2fs_sb_info *sbi); >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c >> index c48e2a2e5e82..248d401bf40a 100644 >> --- a/fs/f2fs/node.c >> +++ b/fs/f2fs/node.c >> @@ -113,25 +113,26 @@ static void clear_node_page_dirty(struct page *page) >> >> static struct page *get_current_nat_page(struct f2fs_sb_info *sbi, nid_t nid) >> { >> - pgoff_t index = current_nat_addr(sbi, nid); >> - return f2fs_get_meta_page(sbi, index); >> + struct page *page; >> + >> + page = f2fs_get_meta_page(sbi, current_nat_addr(sbi, nid)); >> + BUG_ON(IS_ERR(page)); > > Do we need this at this moment? How can we handle this condition if there occurs read failure in __get_meta_page()? just retrying f2fs_get_meta_page()? > >> + return page; >> } >> >> static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid) >> { >> struct page *src_page; >> struct page *dst_page; >> - pgoff_t src_off; >> pgoff_t dst_off; >> void *src_addr; >> void *dst_addr; >> struct f2fs_nm_info *nm_i = NM_I(sbi); >> >> - src_off = current_nat_addr(sbi, nid); >> - dst_off = next_nat_addr(sbi, src_off); >> + dst_off = next_nat_addr(sbi, current_nat_addr(sbi, nid)); >> >> /* get current nat block page with lock */ >> - src_page = f2fs_get_meta_page(sbi, src_off); >> + src_page = get_current_nat_page(sbi, nid); > > This is a different clean-up change. Yes, let me split this into another cleanup patch. Thanks, > >> dst_page = f2fs_grab_meta_page(sbi, dst_off); >> f2fs_bug_on(sbi, PageDirty(src_page)); >> >> @@ -2502,7 +2503,7 @@ int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page) >> return 0; >> } >> >> -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> unsigned int segno, struct f2fs_summary_block *sum) >> { >> struct f2fs_node *rn; >> @@ -2524,6 +2525,9 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> for (idx = addr; idx < addr + nrpages; idx++) { >> struct page *page = f2fs_get_tmp_page(sbi, idx); >> >> + if (IS_ERR(page)) >> + return PTR_ERR(page); >> + >> rn = F2FS_NODE(page); >> sum_entry->nid = rn->footer.nid; >> sum_entry->version = 0; >> @@ -2535,6 +2539,7 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, >> invalidate_mapping_pages(META_MAPPING(sbi), addr, >> addr + nrpages); >> } >> + return 0; >> } >> >> static void remove_nats_in_journal(struct f2fs_sb_info *sbi) >> @@ -2771,7 +2776,13 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi) >> nat_bits_addr = __start_cp_addr(sbi) + sbi->blocks_per_seg - >> nm_i->nat_bits_blocks; >> for (i = 0; i < nm_i->nat_bits_blocks; i++) { >> - struct page *page = f2fs_get_meta_page(sbi, nat_bits_addr++); >> + struct page *page; >> + >> + page = f2fs_get_meta_page(sbi, nat_bits_addr++); >> + if (IS_ERR(page)) { >> + disable_nat_bits(sbi, true); >> + return PTR_ERR(page); >> + } >> >> memcpy(nm_i->nat_bits + (i << F2FS_BLKSIZE_BITS), >> page_address(page), F2FS_BLKSIZE); >> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c >> index 0d927ae26c48..ac70d5547a97 100644 >> --- a/fs/f2fs/recovery.c >> +++ b/fs/f2fs/recovery.c >> @@ -256,6 +256,10 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head, >> return 0; >> >> page = f2fs_get_tmp_page(sbi, blkaddr); >> + if (IS_ERR(page)) { >> + err = PTR_ERR(page); >> + break; >> + } >> >> if (!is_recoverable_dnode(page)) >> break; >> @@ -574,6 +578,10 @@ static int recover_data(struct f2fs_sb_info *sbi, struct list_head *inode_list, >> f2fs_ra_meta_pages_cond(sbi, blkaddr); >> >> page = f2fs_get_tmp_page(sbi, blkaddr); >> + if (IS_ERR(page)) { >> + err = PTR_ERR(page); >> + break; >> + } >> >> if (!is_recoverable_dnode(page)) { >> f2fs_put_page(page, 1); >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >> index e55188975fcc..22600002c690 100644 >> --- a/fs/f2fs/segment.c >> +++ b/fs/f2fs/segment.c >> @@ -2096,7 +2096,11 @@ int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra) >> */ >> struct page *f2fs_get_sum_page(struct f2fs_sb_info *sbi, unsigned int segno) >> { >> - return f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); >> + struct page *page; >> + >> + page = f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); >> + BUG_ON(IS_ERR(page)); > > Ditto. > >> + return page; >> } >> >> void f2fs_update_meta_page(struct f2fs_sb_info *sbi, >> @@ -3122,7 +3126,7 @@ void f2fs_wait_on_block_writeback(struct f2fs_sb_info *sbi, block_t blkaddr) >> } >> } >> >> -static void read_compacted_summaries(struct f2fs_sb_info *sbi) >> +static int read_compacted_summaries(struct f2fs_sb_info *sbi) >> { >> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); >> struct curseg_info *seg_i; >> @@ -3134,6 +3138,8 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) >> start = start_sum_block(sbi); >> >> page = f2fs_get_meta_page(sbi, start++); >> + if (IS_ERR(page)) >> + return PTR_ERR(page); >> kaddr = (unsigned char *)page_address(page); >> >> /* Step 1: restore nat cache */ >> @@ -3174,11 +3180,14 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) >> page = NULL; >> >> page = f2fs_get_meta_page(sbi, start++); >> + if (IS_ERR(page)) >> + return PTR_ERR(page); >> kaddr = (unsigned char *)page_address(page); >> offset = 0; >> } >> } >> f2fs_put_page(page, 1); >> + return 0; >> } >> >> static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) >> @@ -3190,6 +3199,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) >> unsigned short blk_off; >> unsigned int segno = 0; >> block_t blk_addr = 0; >> + int err = 0; >> >> /* get segment number and block addr */ >> if (IS_DATASEG(type)) { >> @@ -3213,6 +3223,8 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) >> } >> >> new = f2fs_get_meta_page(sbi, blk_addr); >> + if (IS_ERR(new)) >> + return PTR_ERR(new); >> sum = (struct f2fs_summary_block *)page_address(new); >> >> if (IS_NODESEG(type)) { >> @@ -3224,7 +3236,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) >> ns->ofs_in_node = 0; >> } >> } else { >> - f2fs_restore_node_summary(sbi, segno, sum); >> + err = f2fs_restore_node_summary(sbi, segno, sum); >> + if (err) >> + goto out; >> } >> } >> >> @@ -3244,8 +3258,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) >> curseg->alloc_type = ckpt->alloc_type[type]; >> curseg->next_blkoff = blk_off; >> mutex_unlock(&curseg->curseg_mutex); >> +out: >> f2fs_put_page(new, 1); >> - return 0; >> + return err; >> } >> >> static int restore_curseg_summaries(struct f2fs_sb_info *sbi) >> @@ -3263,7 +3278,9 @@ static int restore_curseg_summaries(struct f2fs_sb_info *sbi) >> META_CP, true); >> >> /* restore for compacted data summary */ >> - read_compacted_summaries(sbi); >> + err = read_compacted_summaries(sbi); >> + if (err) >> + return err; >> type = CURSEG_HOT_NODE; >> } >> >> @@ -3394,7 +3411,11 @@ int f2fs_lookup_journal_in_cursum(struct f2fs_journal *journal, int type, >> static struct page *get_current_sit_page(struct f2fs_sb_info *sbi, >> unsigned int segno) >> { >> - return f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); >> + struct page *page; >> + >> + page = f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); >> + BUG_ON(IS_ERR(page)); > > Ditoo. > >> + return page; >> } >> >> static struct page *get_next_sit_page(struct f2fs_sb_info *sbi, >> -- >> 2.16.2.17.g38e79b1fd ------------------------------------------------------------------------------ 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] 4+ messages in thread
* Re: [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() 2018-07-15 2:21 ` Chao Yu @ 2018-07-15 3:04 ` Jaegeuk Kim 0 siblings, 0 replies; 4+ messages in thread From: Jaegeuk Kim @ 2018-07-15 3:04 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel On 07/15, Chao Yu wrote: > On 2018/7/15 9:57, Jaegeuk Kim wrote: > > On 07/10, Chao Yu wrote: > >> From: Chao Yu <yuchao0@huawei.com> > >> > >> If caller of __get_meta_page() can handle error, let's propagate error > >> from __get_meta_page(). > >> > >> Signed-off-by: Chao Yu <yuchao0@huawei.com> > >> --- > >> v2: > >> - handle error path in __get_meta_page() correctly. > >> fs/f2fs/checkpoint.c | 21 +++++++++++++++++---- > >> fs/f2fs/f2fs.h | 2 +- > >> fs/f2fs/node.c | 27 +++++++++++++++++++-------- > >> fs/f2fs/recovery.c | 8 ++++++++ > >> fs/f2fs/segment.c | 33 +++++++++++++++++++++++++++------ > >> 5 files changed, 72 insertions(+), 19 deletions(-) > >> > >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > >> index 41e6e5769a2c..b672fc1cdb05 100644 > >> --- a/fs/f2fs/checkpoint.c > >> +++ b/fs/f2fs/checkpoint.c > >> @@ -71,6 +71,7 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, > >> .encrypted_page = NULL, > >> .is_meta = is_meta, > >> }; > >> + int err; > >> > >> if (unlikely(!is_meta)) > >> fio.op_flags &= ~REQ_META; > >> @@ -85,11 +86,12 @@ static struct page *__get_meta_page(struct f2fs_sb_info *sbi, pgoff_t index, > >> > >> fio.page = page; > >> > >> - if (f2fs_submit_page_bio(&fio)) { > >> + err = f2fs_submit_page_bio(&fio); > >> + if (err) { > >> memset(page_address(page), 0, PAGE_SIZE); > > > > No reason to do memset(), if we handle an error? > > Agreed. > > > > >> + f2fs_put_page(page, 1); > >> f2fs_stop_checkpoint(sbi, false); > >> - f2fs_bug_on(sbi, 1); > > > > Keep to f2fs_bug_on(1)? > > If we want to report error return value to caller, we should not let system > panic here? > > > > >> - return page; > >> + return ERR_PTR(err); > >> } > >> > >> lock_page(page); > > > > Need to return an error if read_end_io is failed below in the path. > > Agreed, previously, I found that if we propagate the error to get_node_info(), > it will be a little hard to handle, anyway, let me try. > > > > >> @@ -658,9 +660,15 @@ int f2fs_recover_orphan_inodes(struct f2fs_sb_info *sbi) > >> f2fs_ra_meta_pages(sbi, start_blk, orphan_blocks, META_CP, true); > >> > >> for (i = 0; i < orphan_blocks; i++) { > >> - struct page *page = f2fs_get_meta_page(sbi, start_blk + i); > >> + struct page *page; > >> struct f2fs_orphan_block *orphan_blk; > >> > >> + page = f2fs_get_meta_page(sbi, start_blk + i); > >> + if (IS_ERR(page)) { > >> + err = PTR_ERR(page); > >> + goto out; > >> + } > >> + > >> orphan_blk = (struct f2fs_orphan_block *)page_address(page); > >> for (j = 0; j < le32_to_cpu(orphan_blk->entry_count); j++) { > >> nid_t ino = le32_to_cpu(orphan_blk->ino[j]); > >> @@ -751,6 +759,9 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr, > >> __u32 crc = 0; > >> > >> *cp_page = f2fs_get_meta_page(sbi, cp_addr); > >> + if (IS_ERR(*cp_page)) > >> + return PTR_ERR(*cp_page); > >> + > >> *cp_block = (struct f2fs_checkpoint *)page_address(*cp_page); > >> > >> crc_offset = le32_to_cpu((*cp_block)->checksum_offset); > >> @@ -876,6 +887,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi) > >> unsigned char *ckpt = (unsigned char *)sbi->ckpt; > >> > >> cur_page = f2fs_get_meta_page(sbi, cp_blk_no + i); > >> + if (IS_ERR(cur_page)) > >> + goto free_fail_no_cp; > >> sit_bitmap_ptr = page_address(cur_page); > >> memcpy(ckpt + i * blk_size, sit_bitmap_ptr, blk_size); > >> f2fs_put_page(cur_page, 1); > >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >> index 359526d88d3f..76b51a0de3cd 100644 > >> --- a/fs/f2fs/f2fs.h > >> +++ b/fs/f2fs/f2fs.h > >> @@ -2864,7 +2864,7 @@ int f2fs_try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink); > >> void f2fs_recover_inline_xattr(struct inode *inode, struct page *page); > >> int f2fs_recover_xattr_data(struct inode *inode, struct page *page); > >> int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page); > >> -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> unsigned int segno, struct f2fs_summary_block *sum); > >> void f2fs_flush_nat_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc); > >> int f2fs_build_node_manager(struct f2fs_sb_info *sbi); > >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > >> index c48e2a2e5e82..248d401bf40a 100644 > >> --- a/fs/f2fs/node.c > >> +++ b/fs/f2fs/node.c > >> @@ -113,25 +113,26 @@ static void clear_node_page_dirty(struct page *page) > >> > >> static struct page *get_current_nat_page(struct f2fs_sb_info *sbi, nid_t nid) > >> { > >> - pgoff_t index = current_nat_addr(sbi, nid); > >> - return f2fs_get_meta_page(sbi, index); > >> + struct page *page; > >> + > >> + page = f2fs_get_meta_page(sbi, current_nat_addr(sbi, nid)); > >> + BUG_ON(IS_ERR(page)); > > > > Do we need this at this moment? > > How can we handle this condition if there occurs read failure in > __get_meta_page()? just retrying f2fs_get_meta_page()? IMO, retrying Would be better, since I've concerned about fault injection test. Thanks, > > > > >> + return page; > >> } > >> > >> static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid) > >> { > >> struct page *src_page; > >> struct page *dst_page; > >> - pgoff_t src_off; > >> pgoff_t dst_off; > >> void *src_addr; > >> void *dst_addr; > >> struct f2fs_nm_info *nm_i = NM_I(sbi); > >> > >> - src_off = current_nat_addr(sbi, nid); > >> - dst_off = next_nat_addr(sbi, src_off); > >> + dst_off = next_nat_addr(sbi, current_nat_addr(sbi, nid)); > >> > >> /* get current nat block page with lock */ > >> - src_page = f2fs_get_meta_page(sbi, src_off); > >> + src_page = get_current_nat_page(sbi, nid); > > > > This is a different clean-up change. > > Yes, let me split this into another cleanup patch. > > Thanks, > > > > >> dst_page = f2fs_grab_meta_page(sbi, dst_off); > >> f2fs_bug_on(sbi, PageDirty(src_page)); > >> > >> @@ -2502,7 +2503,7 @@ int f2fs_recover_inode_page(struct f2fs_sb_info *sbi, struct page *page) > >> return 0; > >> } > >> > >> -void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> +int f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> unsigned int segno, struct f2fs_summary_block *sum) > >> { > >> struct f2fs_node *rn; > >> @@ -2524,6 +2525,9 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> for (idx = addr; idx < addr + nrpages; idx++) { > >> struct page *page = f2fs_get_tmp_page(sbi, idx); > >> > >> + if (IS_ERR(page)) > >> + return PTR_ERR(page); > >> + > >> rn = F2FS_NODE(page); > >> sum_entry->nid = rn->footer.nid; > >> sum_entry->version = 0; > >> @@ -2535,6 +2539,7 @@ void f2fs_restore_node_summary(struct f2fs_sb_info *sbi, > >> invalidate_mapping_pages(META_MAPPING(sbi), addr, > >> addr + nrpages); > >> } > >> + return 0; > >> } > >> > >> static void remove_nats_in_journal(struct f2fs_sb_info *sbi) > >> @@ -2771,7 +2776,13 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi) > >> nat_bits_addr = __start_cp_addr(sbi) + sbi->blocks_per_seg - > >> nm_i->nat_bits_blocks; > >> for (i = 0; i < nm_i->nat_bits_blocks; i++) { > >> - struct page *page = f2fs_get_meta_page(sbi, nat_bits_addr++); > >> + struct page *page; > >> + > >> + page = f2fs_get_meta_page(sbi, nat_bits_addr++); > >> + if (IS_ERR(page)) { > >> + disable_nat_bits(sbi, true); > >> + return PTR_ERR(page); > >> + } > >> > >> memcpy(nm_i->nat_bits + (i << F2FS_BLKSIZE_BITS), > >> page_address(page), F2FS_BLKSIZE); > >> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c > >> index 0d927ae26c48..ac70d5547a97 100644 > >> --- a/fs/f2fs/recovery.c > >> +++ b/fs/f2fs/recovery.c > >> @@ -256,6 +256,10 @@ static int find_fsync_dnodes(struct f2fs_sb_info *sbi, struct list_head *head, > >> return 0; > >> > >> page = f2fs_get_tmp_page(sbi, blkaddr); > >> + if (IS_ERR(page)) { > >> + err = PTR_ERR(page); > >> + break; > >> + } > >> > >> if (!is_recoverable_dnode(page)) > >> break; > >> @@ -574,6 +578,10 @@ static int recover_data(struct f2fs_sb_info *sbi, struct list_head *inode_list, > >> f2fs_ra_meta_pages_cond(sbi, blkaddr); > >> > >> page = f2fs_get_tmp_page(sbi, blkaddr); > >> + if (IS_ERR(page)) { > >> + err = PTR_ERR(page); > >> + break; > >> + } > >> > >> if (!is_recoverable_dnode(page)) { > >> f2fs_put_page(page, 1); > >> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >> index e55188975fcc..22600002c690 100644 > >> --- a/fs/f2fs/segment.c > >> +++ b/fs/f2fs/segment.c > >> @@ -2096,7 +2096,11 @@ int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra) > >> */ > >> struct page *f2fs_get_sum_page(struct f2fs_sb_info *sbi, unsigned int segno) > >> { > >> - return f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); > >> + struct page *page; > >> + > >> + page = f2fs_get_meta_page(sbi, GET_SUM_BLOCK(sbi, segno)); > >> + BUG_ON(IS_ERR(page)); > > > > Ditto. > > > >> + return page; > >> } > >> > >> void f2fs_update_meta_page(struct f2fs_sb_info *sbi, > >> @@ -3122,7 +3126,7 @@ void f2fs_wait_on_block_writeback(struct f2fs_sb_info *sbi, block_t blkaddr) > >> } > >> } > >> > >> -static void read_compacted_summaries(struct f2fs_sb_info *sbi) > >> +static int read_compacted_summaries(struct f2fs_sb_info *sbi) > >> { > >> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi); > >> struct curseg_info *seg_i; > >> @@ -3134,6 +3138,8 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) > >> start = start_sum_block(sbi); > >> > >> page = f2fs_get_meta_page(sbi, start++); > >> + if (IS_ERR(page)) > >> + return PTR_ERR(page); > >> kaddr = (unsigned char *)page_address(page); > >> > >> /* Step 1: restore nat cache */ > >> @@ -3174,11 +3180,14 @@ static void read_compacted_summaries(struct f2fs_sb_info *sbi) > >> page = NULL; > >> > >> page = f2fs_get_meta_page(sbi, start++); > >> + if (IS_ERR(page)) > >> + return PTR_ERR(page); > >> kaddr = (unsigned char *)page_address(page); > >> offset = 0; > >> } > >> } > >> f2fs_put_page(page, 1); > >> + return 0; > >> } > >> > >> static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > >> @@ -3190,6 +3199,7 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > >> unsigned short blk_off; > >> unsigned int segno = 0; > >> block_t blk_addr = 0; > >> + int err = 0; > >> > >> /* get segment number and block addr */ > >> if (IS_DATASEG(type)) { > >> @@ -3213,6 +3223,8 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > >> } > >> > >> new = f2fs_get_meta_page(sbi, blk_addr); > >> + if (IS_ERR(new)) > >> + return PTR_ERR(new); > >> sum = (struct f2fs_summary_block *)page_address(new); > >> > >> if (IS_NODESEG(type)) { > >> @@ -3224,7 +3236,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > >> ns->ofs_in_node = 0; > >> } > >> } else { > >> - f2fs_restore_node_summary(sbi, segno, sum); > >> + err = f2fs_restore_node_summary(sbi, segno, sum); > >> + if (err) > >> + goto out; > >> } > >> } > >> > >> @@ -3244,8 +3258,9 @@ static int read_normal_summaries(struct f2fs_sb_info *sbi, int type) > >> curseg->alloc_type = ckpt->alloc_type[type]; > >> curseg->next_blkoff = blk_off; > >> mutex_unlock(&curseg->curseg_mutex); > >> +out: > >> f2fs_put_page(new, 1); > >> - return 0; > >> + return err; > >> } > >> > >> static int restore_curseg_summaries(struct f2fs_sb_info *sbi) > >> @@ -3263,7 +3278,9 @@ static int restore_curseg_summaries(struct f2fs_sb_info *sbi) > >> META_CP, true); > >> > >> /* restore for compacted data summary */ > >> - read_compacted_summaries(sbi); > >> + err = read_compacted_summaries(sbi); > >> + if (err) > >> + return err; > >> type = CURSEG_HOT_NODE; > >> } > >> > >> @@ -3394,7 +3411,11 @@ int f2fs_lookup_journal_in_cursum(struct f2fs_journal *journal, int type, > >> static struct page *get_current_sit_page(struct f2fs_sb_info *sbi, > >> unsigned int segno) > >> { > >> - return f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); > >> + struct page *page; > >> + > >> + page = f2fs_get_meta_page(sbi, current_sit_addr(sbi, segno)); > >> + BUG_ON(IS_ERR(page)); > > > > Ditoo. > > > >> + return page; > >> } > >> > >> static struct page *get_next_sit_page(struct f2fs_sb_info *sbi, > >> -- > >> 2.16.2.17.g38e79b1fd ------------------------------------------------------------------------------ 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] 4+ messages in thread
end of thread, other threads:[~2018-07-15 3:04 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-10 15:00 [PATCH v2 5/5] f2fs: fix to propagate error from __get_meta_page() Chao Yu 2018-07-15 1:57 ` Jaegeuk Kim 2018-07-15 2:21 ` Chao Yu 2018-07-15 3:04 ` 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).