* [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect()
@ 2025-05-02 8:15 Dan Carpenter
2025-05-07 2:59 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-05-02 8:15 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-f2fs-devel
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?
One time email warning etc. I could also mark filemap_get_folio() as
a no fail function to prevent false positives.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect()
2025-05-02 8:15 [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect() Dan Carpenter
@ 2025-05-07 2:59 ` Chao Yu via Linux-f2fs-devel
2025-05-07 7:31 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2025-05-07 2:59 UTC (permalink / raw)
To: Dan Carpenter, Matthew Wilcox; +Cc: linux-f2fs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect()
2025-05-07 2:59 ` Chao Yu via Linux-f2fs-devel
@ 2025-05-07 7:31 ` Dan Carpenter
2025-05-07 11:48 ` Chao Yu via Linux-f2fs-devel
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2025-05-07 7:31 UTC (permalink / raw)
To: Chao Yu; +Cc: Matthew Wilcox, linux-f2fs-devel
On Wed, May 07, 2025 at 10:59:11AM +0800, Chao Yu wrote:
> On 5/2/25 16:15, Dan Carpenter wrote:
> > Hello Matthew Wilcox (Oracle),
> >
> > 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 for looking at this!
I tend to not worry about false positives a lot. Only warnings in new
code should be considered as real, everything old is something that we
have reviewed and ignored. If people have questions they can look it up
on lore.
regards,
dan carpenter
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect()
2025-05-07 7:31 ` Dan Carpenter
@ 2025-05-07 11:48 ` Chao Yu via Linux-f2fs-devel
0 siblings, 0 replies; 4+ messages in thread
From: Chao Yu via Linux-f2fs-devel @ 2025-05-07 11:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Matthew Wilcox, linux-f2fs-devel
On 5/7/25 15:31, Dan Carpenter wrote:
> On Wed, May 07, 2025 at 10:59:11AM +0800, Chao Yu wrote:
>> On 5/2/25 16:15, Dan Carpenter wrote:
>>> Hello Matthew Wilcox (Oracle),
>>>
>>> 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 for looking at this!
>
> I tend to not worry about false positives a lot. Only warnings in new
> code should be considered as real, everything old is something that we
> have reviewed and ignored. If people have questions they can look it up
> on lore.
Ah, above implementation (not checking return valud of f2fs_get_sum_page) exists
for a long time, for such old code, Smatch didn't complain.
It matches what you explained. ;-)
Thanks,
>
> regards,
> dan carpenter
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-05-07 11:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-02 8:15 [f2fs-dev] [bug report] f2fs: Use folios in do_garbage_collect() Dan Carpenter
2025-05-07 2:59 ` Chao Yu via Linux-f2fs-devel
2025-05-07 7:31 ` Dan Carpenter
2025-05-07 11:48 ` Chao Yu via Linux-f2fs-devel
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).