Re: [PATCH v6 07/12] mm, swap: reclaim physical slots backing cache-only vswap entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mm, swap: try_to_reclaim_vswap_backing() overshoots physical run when i > 0
- [Medium] mm, swap: folio_realloc_swap() misses SWP_RMAP_CACHE_ONLY when swap_count is 0
- [Medium] mm, swap: missing vswap cache-only reclaim in cluster_scan_range()
--

commit e03ce92e52f674a68e33585bcd17c229fae53bf0
Author: Nhat Pham <nphamcs@gmail.com>

mm, swap: reclaim physical slots backing cache-only vswap entries

This commit introduces a SWP_RMAP_CACHE_ONLY flag on physical swap table rmap
entries when a vswap slot's swap count drops to zero while its folio remains
in the swap cache. It teaches swap_reclaim_full_clusters() to reclaim those
physical backing slots via try_to_reclaim_vswap_backing().

> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index be66ac79313ce..640e38a55497e 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
[ ... ]
> @@ -877,6 +882,55 @@ static int swap_cluster_setup_bad_slot(struct swap_info_struct *si,
>  	return ret;
>  }
>  
> +/*
> + * Try to reclaim a Vswap-tagged physical slot backing a vswap entry.
> + * The physical cluster lock must NOT be held. Returns the backing folio's
> + * page count, negated if the slots could not be reclaimed, or 0 if the
> + * folio could not be shown to own @offset (i.e. there is a race).
> + */
> +static int try_to_reclaim_vswap_backing(struct swap_info_struct *si,
> +					unsigned long offset,
> +					swp_entry_t vswap_entry)
> +{
> +	swp_entry_t phys_base;
> +	struct folio *folio;
> +	unsigned int i;
> +	int ret;
> +
> +	folio = swap_cache_get_folio(vswap_entry);
> +	if (!folio)
> +		return 0;
> +
> +	if (!folio_trylock(folio)) {
> +		/* Unvalidated: the folio may not even own @offset. */
> +		folio_put(folio);
> +		return 0;
> +	}
> +
> +	if (!folio_matches_swap_entry(folio, vswap_entry)) {
> +		folio_unlock(folio);
> +		folio_put(folio);
> +		return 0;
> +	}
> +
> +	i = vswap_entry.val - folio->swap.val;
> +	phys_base = vswap_to_phys(folio->swap);
> +	if (!phys_base.val || swp_type(phys_base) != si->type ||
> +	    swp_offset(phys_base) + i != offset) {
> +		folio_unlock(folio);
> +		folio_put(folio);
> +		return 0;
> +	}
> +
> +	/* The run is ours: skip it all, whether or not the free succeeds. */
> +	ret = folio_nr_pages(folio);
> +	if (!folio_free_swap(folio))
> +		ret = -ret;
> +	folio_unlock(folio);
> +	folio_put(folio);
> +	return ret;
> +}

[Severity: Medium]
When try_to_reclaim_vswap_backing() in mm/swapfile.c is called for a large
folio at a non-zero subpage offset (where i = vswap_entry.val -
folio->swap.val > 0 and offset == swp_offset(phys_base) + i), doesn't
returning folio_nr_pages(folio) cause swap_reclaim_full_clusters() to
overshoot the end of the folio's physical slot run by i slots?

This can happen when scanning a cluster in swap_reclaim_full_clusters() if:
- subpages 0..i-1 still have swap_count > 0 (so SWP_RMAP_CACHE_ONLY is not
  set on them) while subpage i had its swap_count drop to 0 via single-PTE
  swap-in or partial unmap,
- folio_trylock(folio) fails on subpages 0..i-1 and then succeeds on
  subpage i, or
- a concurrent swap_put_entries_cluster() marks the folio's subpages
  SWP_RMAP_CACHE_ONLY after the scanner has already advanced past subpages
  0..i-1.

Because swap_reclaim_full_clusters() advances offset by:

    offset += abs(nr_reclaim);

returning folio_nr_pages(folio) instead of folio_nr_pages(folio) - i advances
offset to swp_offset(phys_base) + i + folio_nr_pages(folio), skipping the
first i physical slots of the following folio(s) in the cluster. Should ret
be initialized to folio_nr_pages(folio) - i instead?

> @@ -1182,6 +1237,19 @@ static void swap_reclaim_full_clusters(struct swap_info_struct *si, bool force)
>  					offset += abs(nr_reclaim);
>  					continue;
>  				}
> +			} else if (swp_tb_is_vswap(swp_tb) &&
> +				   (swp_tb & SWP_RMAP_CACHE_ONLY)) {
> +				vswap_entry = swp_tb_vswap_to_swp_entry(swp_tb);
> +				spin_unlock(&ci->lock);
> +				nr_reclaim = try_to_reclaim_vswap_backing(si, offset,
> +									  vswap_entry);
> +				ci = swap_cluster_lock(si, offset);
> +				if (!ci)
> +					goto next;
> +				if (nr_reclaim) {
> +					offset += abs(nr_reclaim);
> +					continue;
> +				}
>  			}
>  			offset++;
>  		}

[Severity: Medium]
While swap_reclaim_full_clusters() in mm/swapfile.c is updated here to
reclaim SWP_RMAP_CACHE_ONLY entries from full clusters, what about non-full
or fragmented clusters scanned during allocation via
folio_alloc_phys_swap() -> alloc_swap_scan_cluster()?

In mm/swapfile.c, cluster_scan_range() currently only checks for direct
folio entries with swap_count == 0:

    if (swp_tb_is_folio(swp_tb) && !__swp_tb_get_count(swp_tb)) {
        if (swap_is_vswap(si) || !vm_swap_full())
            return false;
        *need_reclaim = true;
        continue;
    }
    /* Slot with zero count can only be NULL or folio */
    VM_WARN_ON(!swp_tb_get_count(swp_tb));
    return false;

And cluster_reclaim_range() stops on any entry where swp_tb_get_count(swp_tb)
is non-zero (which returns -EINVAL for Vswap-tagged entries):

    do {
        swp_tb = swap_table_get(ci, offset % SWAPFILE_CLUSTER);
        if (swp_tb_get_count(swp_tb))
            break;
        if (swp_tb_is_folio(swp_tb))
            if (__try_to_reclaim_swap(si, offset, TTRS_ANYWAY) < 0)
                break;
    } while (++offset < end);

If a physical cluster has some free slots (ci->count < SWAPFILE_CLUSTER, so
it sits on percpu_swap_cluster, global_cluster, si->nonfull_clusters[order],
or si->frag_clusters[order] rather than si->full_clusters), won't
SWP_RMAP_CACHE_ONLY slots in that cluster prevent cluster_scan_range() and
cluster_reclaim_range() from reclaiming them when vm_swap_full() is true,
causing high-order physical swap allocations to fail prematurely?

[ ... ]
> @@ -1752,6 +1820,8 @@ static void swap_put_entries_cluster(struct swap_info_struct *si,
>  			}
>  			/* count will be 0 after put, slot can be reclaimed */
>  			need_reclaim = true;
> +			if (swap_is_vswap(si))
> +				vswap_mark_cache_only(ci, ci_off);
>  		}
>  		/*
>  		 * A count != 1 or cached slot can't be freed. Put its swap
[ ... ]
> @@ -2008,6 +2080,51 @@ int folio_alloc_swap(struct folio *folio)
>  	return order ? -E2BIG : -ENOMEM;
>  }
>  
> +static void vswap_mark_cache_only(struct swap_cluster_info *ci,
> +				  unsigned int ci_off)
> +{
> +	struct swap_cluster_info_dynamic *ci_dyn;
> +	struct swap_cluster_info *pci;
> +	swp_entry_t phys;
> +	unsigned long vt;
> +
> +	ci_dyn = container_of(ci, struct swap_cluster_info_dynamic, ci);
> +	vt = __vtable_get(ci_dyn, ci_off);
> +
> +	if (vtable_type(vt) == VSWAP_SWAPFILE) {
> +		phys = vtable_to_phys(vt);
> +		pci = __swap_entry_to_cluster(phys);
> +		swap_rmap_mark_cache_only(pci, swp_cluster_offset(phys));
> +	}
> +}

[Severity: Medium]
Can physical swap slots remain unmarked with SWP_RMAP_CACHE_ONLY if a vswap
slot already has swap_count == 0 when folio_realloc_swap() in mm/swapfile.c
installs its physical backing?

In folio_realloc_swap(), folio_alloc_phys_swap() ->
__swap_cluster_alloc_entries() initializes the physical swap_table entries
with swp_entry_to_swp_tb_vswap(v) (with SWP_RMAP_CACHE_ONLY clear), and then
folio_realloc_swap() writes VSWAP_SWAPFILE into virtual_table under ci->lock:

    for (i = 0; i < nr; i++) {
        pe.val = phys_entry.val + i;
        __vtable_set(ci_dyn, voff + i, vtable_mk_phys(pe));
    }

However, vswap_mark_cache_only() only sets SWP_RMAP_CACHE_ONLY during
swap_put_entries_cluster() when swap_count transitions from 1 to 0 and
vtable_type(vt) is already VSWAP_SWAPFILE. A vswap slot can already have
swap_count == 0 when folio_realloc_swap() installs VSWAP_SWAPFILE in two
cases:

1. During swap_writeout() or zswap_writeback_entry(), the folio is locked
   while virtual_table is still VSWAP_NONE or VSWAP_ZSWAP. A concurrent PTE
   zap (such as munmap() or MADV_DONTNEED calling free_swap_and_cache_nr() ->
   swap_put_entries_direct() -> swap_put_entries_cluster()) can drop the
   vswap slot's swap_count from 1 to 0 before folio_realloc_swap() sets
   VSWAP_SWAPFILE. At that point vswap_mark_cache_only() sees
   vtable_type(vt) != VSWAP_SWAPFILE, and __try_to_reclaim_swap() cannot
   lock the folio.
2. When swapping out a partially unmapped large folio (mTHP), some subpages
   may already have swap_count == 0 while other subpages have swap_count > 0.

Since swap_count is already 0, swap_put_entries_cluster() will not run on
those slots again. Should folio_realloc_swap() check whether
__swp_tb_get_count(__swap_table_get(ci, voff + i)) == 0 while holding
ci->lock and mark the corresponding physical rmap entry with
SWP_RMAP_CACHE_ONLY?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021060323.553433-1-nphamcs@gmail.com?part=7