On 05/02/2024 09:51, Barry Song wrote:
> +Chris, Suren and Chuanhua
>
> Hi Ryan,
>
>> + /*
>> + * __scan_swap_map_try_ssd_cluster() may drop si->lock during discard,
>> + * so indicate that we are scanning to synchronise with swapoff.
>> + */
>> + si->flags += SWP_SCANNING;
>> + ret = __scan_swap_map_try_ssd_cluster(si, &offset, &scan_base, order);
>> + si->flags -= SWP_SCANNING;
>
> nobody is using this scan_base afterwards. it seems a bit weird to
> pass a pointer.
>
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -1212,11 +1212,13 @@ static unsigned int shrink_folio_list(struct list_head *folio_list,
>> if (!can_split_folio(folio, NULL))
>> goto activate_locked;
>> /*
>> - * Split folios without a PMD map right
>> - * away. Chances are some or all of the
>> - * tail pages can be freed without IO.
>> + * Split PMD-mappable folios without a
>> + * PMD map right away. Chances are some
>> + * or all of the tail pages can be freed
>> + * without IO.
>> */
>> - if (!folio_entire_mapcount(folio) &&
>> + if (folio_test_pmd_mappable(folio) &&
>> + !folio_entire_mapcount(folio) &&
>> split_folio_to_list(folio,
>> folio_list))
>> goto activate_locked;
>> --
>
> Chuanhua and I ran this patchset for a couple of days and found a race
> between reclamation and split_folio. this might cause applications get
> wrong data 0 while swapping-in.
>
> in case one thread(T1) is reclaiming a large folio by some means, still
> another thread is calling madvise MADV_PGOUT(T2). and at the same time,
> we have two threads T3 and T4 to swap-in in parallel. T1 doesn't split
> and T2 does split as below,
>
> static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> unsigned long addr, unsigned long end,
> struct mm_walk *walk)
> {
>
> /*
> * Creating a THP page is expensive so split it only if we
> * are sure it's worth. Split it if we are only owner.
> */
> if (folio_test_large(folio)) {
> int err;
>
> if (folio_estimated_sharers(folio) != 1)
> break;
> if (pageout_anon_only_filter && !folio_test_anon(folio))
> break;
> if (!folio_trylock(folio))
> break;
> folio_get(folio);
> arch_leave_lazy_mmu_mode();
> pte_unmap_unlock(start_pte, ptl);
> start_pte = NULL;
> err = split_folio(folio);
> folio_unlock(folio);
> folio_put(folio);
> if (err)
> break;
> start_pte = pte =
> pte_offset_map_lock(mm, pmd, addr, &ptl);
> if (!start_pte)
> break;
> arch_enter_lazy_mmu_mode();
> pte--;
> addr -= PAGE_SIZE;
> continue;
> }
>
> return 0;
> }
>
>
>
> if T3 and T4 swap-in same page, and they both do swap_read_folio(). the
> first one of T3 and T4 who gets PTL will set pte, and the 2nd one will
> check pte_same() and find pte has been changed by another thread, thus
> goto out_nomap in do_swap_page.
> vm_fault_t do_swap_page(struct vm_fault *vmf)
> {
> if (!folio) {
> if (data_race(si->flags & SWP_SYNCHRONOUS_IO) &&
> __swap_count(entry) == 1) {
> /* skip swapcache */
> folio = vma_alloc_folio(GFP_HIGHUSER_MOVABLE, 0,
> vma, vmf->address, false);
> page = &folio->page;
> if (folio) {
> __folio_set_locked(folio);
> __folio_set_swapbacked(folio);
>
> /* To provide entry to swap_read_folio() */
> folio->swap = entry;
> swap_read_folio(folio, true, NULL);
> folio->private = NULL;
> }
> } else {
> }
>
>
> /*
> * Back out if somebody else already faulted in this pte.
> */
> vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, vmf->address,
> &vmf->ptl);
> if (unlikely(!vmf->pte || !pte_same(ptep_get(vmf->pte), vmf->orig_pte)))
> goto out_nomap;
>
> swap_free(entry);
> pte = mk_pte(page, vma->vm_page_prot);
>
> set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
> return ret;
> }
>
>
> while T1 and T2 is working in parallel, T2 will split folio. this can
> run into race with T1's reclamation without splitting. T2 will split
> a large folio into a couple of normal pages and reclaim them.
>
> If T3 finishes swap_read_folio and gets PTL earlier than T4, it calls
> set_pte and swap_free. this will cause zRAM to free the slot. then
> t4 will get zero data in swap_read_folio() as the below zRAM code
> will fill zero for freed slots,
>
> static int zram_read_from_zspool(struct zram *zram, struct page *page,
> u32 index)
> {
> ...
>
> handle = zram_get_handle(zram, index);
> if (!handle || zram_test_flag(zram, index, ZRAM_SAME)) {
> unsigned long value;
> void *mem;
>
> value = handle ? zram_get_element(zram, index) : 0;
> mem = kmap_local_page(page);
> zram_fill_page(mem, PAGE_SIZE, value);
> kunmap_local(mem);
> return 0;
> }
> }
>
> usually, after t3 frees swap and does set_pte, t4's pte_same becomes
> false, it won't set pte again. So filling zero data into freed slot
> by zRAM driver is not a problem at all. but the race is that T1 and
> T2 might do set swap to ptes twice as t1 doesn't split but t2 splits
> (splitted normal folios are also added into reclaim_list), thus, the
> corrupted zero data will get a chance to be set into PTE by t4 as t4
> reads the new PTE which is set secondly and has the same swap entry
> as its orig_pte after T3 has swapped-in and free the swap entry.
>
> we have worked around this problem by preventing T4 from splitting
> large folios and letting it goto skip the large folios entirely in
> MADV PAGEOUT once we detect a concurrent reclamation for this large
> folio.
>
> so my understanding is changing vmscan isn't sufficient to support
> large folio swap-out without splitting. we have to adjust madvise
> as well. we will have a fix for this problem in
> [PATCH RFC 6/6] mm: madvise: don't split mTHP for MADV_PAGEOUT
> https://lore.kernel.org/linux-mm/20240118111036.72641-7-21cnbao@xxxxxxxxx/
>
> But i feel this patch should be a part of your swap-out patchset rather
> than the swap-in series of Chuanhua and me :-)
Hi Barry, Chuanhua,
Thanks for the very detailed bug report! I'm going to have to take some time to
get my head around the details. But yes, I agree the fix needs to be part of the
swap-out series.
Sorry I haven't progressed this series as I had hoped. I've been concentrating
on getting the contpte series upstream. I'm hoping I will find some time to move
this series along by the tail end of Feb (hoping to get it in shape for v6.10).
Hopefully that doesn't cause you any big problems?
Thanks,
Ryan
>
> Thanks
> Barry
