On 5/2/25 16:15, Dan Carpenter wrote:
> Hello Matthew Wilcox (Oracle),
>
> Commit 5d895f7beae9 ("f2fs: Use folios in do_garbage_collect()") from
> Mar 31, 2025 (linux-next), leads to the following Smatch static
> checker warning:
>
> fs/f2fs/gc.c:1799 do_garbage_collect()
> error: 'sum_folio' dereferencing possible ERR_PTR()
>
> fs/f2fs/gc.c
> 1716 static int do_garbage_collect(struct f2fs_sb_info *sbi,
> 1717 unsigned int start_segno,
> 1718 struct gc_inode_list *gc_list, int
> gc_type,
> 1719 bool force_migrate, bool one_time)
> 1720 {
> 1721 struct blk_plug plug;
> 1722 unsigned int segno = start_segno;
> 1723 unsigned int end_segno = start_segno + SEGS_PER_SEC(sbi);
> 1724 unsigned int sec_end_segno;
> 1725 int seg_freed = 0, migrated = 0;
> 1726 unsigned char type = IS_DATASEG(get_seg_entry(sbi,
> segno)->type) ?
> 1727 SUM_TYPE_DATA :
> SUM_TYPE_NODE;
> 1728 unsigned char data_type = (type == SUM_TYPE_DATA) ? DATA :
> NODE;
> 1729 int submitted = 0;
> 1730
> 1731 if (__is_large_section(sbi)) {
> 1732 sec_end_segno = rounddown(end_segno,
> SEGS_PER_SEC(sbi));
> 1733
> 1734 /*
> 1735 * zone-capacity can be less than zone-size in zoned
> devices,
> 1736 * resulting in less than expected usable segments
> in the zone,
> 1737 * calculate the end segno in the zone which can be
> garbage
> 1738 * collected
> 1739 */
> 1740 if (f2fs_sb_has_blkzoned(sbi))
> 1741 sec_end_segno -= SEGS_PER_SEC(sbi) -
> 1742 f2fs_usable_segs_in_sec(sbi);
> 1743
> 1744 if (gc_type == BG_GC || one_time) {
> 1745 unsigned int window_granularity =
> 1746 sbi->migration_window_granularity;
> 1747
> 1748 if (f2fs_sb_has_blkzoned(sbi) &&
> 1749 !has_enough_free_blocks(sbi,
> 1750
> sbi->gc_thread->boost_zoned_gc_percent))
> 1751 window_granularity *=
> 1752 BOOST_GC_MULTIPLE;
> 1753
> 1754 end_segno = start_segno + window_granularity;
> 1755 }
> 1756
> 1757 if (end_segno > sec_end_segno)
> 1758 end_segno = sec_end_segno;
> 1759 }
> 1760
> 1761 sanity_check_seg_type(sbi, get_seg_entry(sbi, segno)->type);
> 1762
> 1763 /* readahead multi ssa blocks those have contiguous address
> */
> 1764 if (__is_large_section(sbi))
> 1765 f2fs_ra_meta_pages(sbi, GET_SUM_BLOCK(sbi, segno),
> 1766 end_segno - segno, META_SSA,
> true);
> 1767
> 1768 /* reference all summary page */
> 1769 while (segno < end_segno) {
> 1770 struct folio *sum_folio = f2fs_get_sum_folio(sbi,
> segno++);
> 1771 if (IS_ERR(sum_folio)) {
> 1772 int err = PTR_ERR(sum_folio);
> 1773
> 1774 end_segno = segno - 1;
> 1775 for (segno = start_segno; segno < end_segno;
> segno++) {
> 1776 sum_folio =
> filemap_get_folio(META_MAPPING(sbi),
> 1777 GET_SUM_BLOCK(sbi,
> segno));
> 1778 folio_put_refs(sum_folio, 2);
> 1779 }
> 1780 return err;
> 1781 }
> 1782 folio_unlock(sum_folio);
> 1783 }
> 1784
> 1785 blk_start_plug(&plug);
> 1786
> 1787 for (segno = start_segno; segno < end_segno; segno++) {
> 1788 struct f2fs_summary_block *sum;
> 1789
> 1790 /* find segment summary of victim */
> 1791 struct folio *sum_folio =
> filemap_get_folio(META_MAPPING(sbi),
> 1792 GET_SUM_BLOCK(sbi, segno));
>
> Smatch gets a bit confused here and thinks filemap_get_folio() can return
> a lot of different error pointers, but really filemap_get_folio() can only
> return ERR_PTR(-ENOENT). And possibly in this context, it can't even
> return that?
Dan,
It shouldn't fail due to we have grabbed that page in do_garbage_collect() as
below,
and haven't released the reference yet.
> 1768 /* reference all summary page */
> 1769 while (segno < end_segno) {
> 1770 struct folio *sum_folio = f2fs_get_sum_folio(sbi,
> segno++);
>
> One time email warning etc. I could also mark filemap_get_folio() as
> a no fail function to prevent false positives.
So, it doesn't mean filemap_get_folio() never fail, can Smatch detect above
condition to avoid triggering warning?
Thanks,
>
> 1793
> 1794 if (get_valid_blocks(sbi, segno, false) == 0)
> 1795 goto freed;
> 1796 if (gc_type == BG_GC && __is_large_section(sbi) &&
> 1797 migrated >=
> sbi->migration_granularity)
> 1798 goto skip;
> --> 1799 if (!folio_test_uptodate(sum_folio) ||
> ^^^^^^^^^
> Dereferenced here.
>
> 1800 unlikely(f2fs_cp_error(sbi)))
> 1801 goto skip;
> 1802
>
> regards,
> dan carpenter
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
