On Thu, Mar 28, 2024 at 10:12 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Thu, Mar 28, 2024 at 12:03:02PM +0800, Zhaoyang Huang wrote: > > On Thu, Mar 28, 2024 at 11:18 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > > > On Thu, Mar 28, 2024 at 09:27:31AM +0800, Zhaoyang Huang wrote: > > > > ok, I missed the refcnt from alloc_pages. However, I still think it is > > > > a bug to call readahead_folio in read_pages as the refcnt obtained by > > > > alloc_pages should be its final guard which is paired to the one which > > > > checked in shrink_folio_list->__remove_mapping->folio_ref_freeze(2)(this > > > > 2 represent alloc_pages & page cache). If we removed this one without > > > > > > __remove_mapping() requires that the caller holds the folio locked. > > > Since the readahead code unlocks the folio, __remove_mapping() cannot > > > be run because the caller of __remove_mapping() will wait for the folio > > > lock. > > repost the whole timing sequence to make it more clear and fix > > incorrect description of previous feedback > > I can't understand what you think the problem is here. Please try > again. > > > Follow the refcount through. > > > > In page_cache_ra_unbounded(): > > > > folio = filemap_alloc_folio(gfp_mask, 0); > > (folio has refcount 1) > > ret = filemap_add_folio(mapping, folio, index + i, gfp_mask); > > (folio has refcount 2, PG_lru) > > > > Then we call read_pages() > > First we call ->readahead() which for some reason stops early. > > Then we call readahead_folio() which calls folio_put() > > (folio has refcount 1) > > Then we call folio_get() > > (folio has refcount 2) > > Then we call filemap_remove_folio() > > (folio has refcount 1) > > Then we call folio_unlock() > > Then we call folio_put() > > > > Amending steps for previous timing sequence below where [1] races with > > [2] that has nothing to do with __remove_mapping(). IMO, no file_folio > > should be freed by folio_put as the refcnt obtained by alloc_pages > > keep it always imbalanced until shrink_folio_list->__remove_mapping, > > where the folio_ref_freeze(2) implies the refcnt of alloc_pages and > > isolation should be the last two. release_pages is a special scenario > > that the refcnt of alloc_pages is freed implicitly in > > delete_from_page_cache_batch->filemap_free_folio. > > > > folio_put() > > { > > if(folio_put_test_zero()) > > *** we should NOT be here as the refcnt of alloc_pages should NOT be dropped *** > > if (folio_test_lru()) > > *** preempted here with refcnt == 0 and pass PG_lru check *** > > [1] > > lruvec_del_folio() > > Then thread_isolate call folio_isolate_lru() > > folio_isolate_lru() > > { > > folio_test_clear_lru() > > folio_get() > > [2] > > lruvec_del_folio() > > } > > -------------------------------------------------------------------------------------------- > > shrink_folio_list() > > { > > __remove_mapping() > > { > > refcount = 1 + folio_nr_pages; > > *** the refcount = 1 + 1 implies there should be only the refcnt of > > alloc_pages and previous isolation for a no-busy folio as all PTE has > > gone*** > > if (!folio_ref_freeze(refcount)) > > goto keeplock; > > } > > } key steps in brief: Thread_truncate get folio to its local fbatch by find_get_entry in step 2 The refcnt is deducted to 1 which is not as expect as from alloc_pages but from thread_truncate's local fbatch in step 7 Thread_reclaim succeed to isolate the folio by the wrong refcnt(not the value but meaning) in step 8 Thread_truncate hit the VM_BUG_ON in step 9 all steps: Thread_readahead: 0. folio = filemap_alloc_folio(gfp_mask, 0); (folio has refcount 1) 1. ret = filemap_add_folio(mapping, folio, index + i, gfp_mask); (folio has refcount 2) 2. thread_truncate hold one refcnt and add this folio to fbatch_truncate (folio has refcount 3(alloc, page cache, fbatch_truncate), PG_lru) 3. Then we call read_pages() First we call ->readahead() which for some reason stops early. 4. Then we call readahead_folio() which calls folio_put() (folio has refcount 2) 5. Then we call folio_get() (folio has refcount 3) 6. Then we call filemap_remove_folio() (folio has refcount 2) 7. Then we call folio_unlock() Then we call folio_put() (folio has refcount 1(fbatch_truncate)) 8. thread_reclaim call shrink_inactive_list->isolate_lru_folios shrink_inactive_list isolate_lru_folios if (!folio_test_lru(folio)) if (!folio_try_get(folio)) if (!folio_test_clear_lru(folio)) list_move(folio, dst) (folio has refcount 2) 8.1. thread_reclaim call shrink_folio_list->__remove_mapping shrink_folio_list() __remove_mapping() (refcount = 2) if (!folio_ref_freeze(2)) //true list_add(folio, free_folios); (folio has refcount 0) 9. thread_truncate will hit the refcnt VM_BUG_ON(refcnt == 0) in folio_put_testzero