Re: [PATCH v4 09/19] mm, swap: swap entry of a bad slot should not be considered as swapped out

[Baoquan He <bhe@redhat.com>]

On 12/05/25 at 03:29am, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> When checking if a swap entry is swapped out, we simply check if the
> bitwise result of the count value is larger than 0. But SWAP_MAP_BAD
> will also be considered as a swao count value larger than 0.
> 
> SWAP_MAP_BAD being considered as a count value larger than 0 is useful
> for the swap allocator: they will be seen as a used slot, so the
> allocator will skip them. But for the swapped out check, this
> isn't correct.
> 
> There is currently no observable issue. The swapped out check is only
> useful for readahead and folio swapped-out status check. For readahead,
> the swap cache layer will abort upon checking and updating the swap map.
> For the folio swapped out status check, the swap allocator will never
> allocate an entry of bad slots to folio, so that part is fine too. The
> worst that could happen now is redundant allocation/freeing of folios
> and waste CPU time.
> 
> This also makes it easier to get rid of swap map checking and update
> during folio insertion in the swap cache layer.

Will swap_entry_swapped() be called in other places in phase 3 of swap
table?

I checked the code of the current phase 2, in all three places you
convert it with swp_offset(entry), is it necessary? Why don't you keep
the swap_entry_swapped(si, entry)? Surely, this is trivial.

mm/swap_state.c <<swap_cache_alloc_folio>>
if (!swap_entry_swapped(si, swp_offset(entry)))
mm/swapfile.c <<folio_swapped>>
return swap_entry_swapped(si, swp_offset(entry));
mm/swapfile.c <<swap_free_hibernation_slot>>
             WARN_ON(swap_entry_swapped(si, offset));

> 
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  include/linux/swap.h |  6 ++++--
>  mm/swap_state.c      |  4 ++--
>  mm/swapfile.c        | 22 +++++++++++-----------
>  3 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index bf72b548a96d..936fa8f9e5f3 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -466,7 +466,8 @@ int find_first_swap(dev_t *device);
>  extern unsigned int count_swap_pages(int, int);
>  extern sector_t swapdev_block(int, pgoff_t);
>  extern int __swap_count(swp_entry_t entry);
> -extern bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry);
> +extern bool swap_entry_swapped(struct swap_info_struct *si,
> +			       unsigned long offset);
>  extern int swp_swapcount(swp_entry_t entry);
>  struct backing_dev_info;
>  extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
> @@ -535,7 +536,8 @@ static inline int __swap_count(swp_entry_t entry)
>  	return 0;
>  }
>  
> -static inline bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry)
> +static inline bool swap_entry_swapped(struct swap_info_struct *si,
> +				      unsigned long offset)
>  {
>  	return false;
>  }
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 8c429dc33ca9..0c5aad537716 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -527,8 +527,8 @@ struct folio *swap_cache_alloc_folio(swp_entry_t entry, gfp_t gfp_mask,
>  	if (folio)
>  		return folio;
>  
> -	/* Skip allocation for unused swap slot for readahead path. */
> -	if (!swap_entry_swapped(si, entry))
> +	/* Skip allocation for unused and bad swap slot for readahead. */
> +	if (!swap_entry_swapped(si, swp_offset(entry)))
>  		return NULL;
>  
>  	/* Allocate a new folio to be added into the swap cache. */
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index e23287c06f1c..5a766d4fcaa5 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1766,21 +1766,21 @@ int __swap_count(swp_entry_t entry)
>  	return swap_count(si->swap_map[offset]);
>  }
>  
> -/*
> - * How many references to @entry are currently swapped out?
> - * This does not give an exact answer when swap count is continued,
> - * but does include the high COUNT_CONTINUED flag to allow for that.
> +/**
> + * swap_entry_swapped - Check if the swap entry at @offset is swapped.
> + * @si: the swap device.
> + * @offset: offset of the swap entry.
>   */
> -bool swap_entry_swapped(struct swap_info_struct *si, swp_entry_t entry)
> +bool swap_entry_swapped(struct swap_info_struct *si, unsigned long offset)
>  {
> -	pgoff_t offset = swp_offset(entry);
>  	struct swap_cluster_info *ci;
>  	int count;
>  
>  	ci = swap_cluster_lock(si, offset);
>  	count = swap_count(si->swap_map[offset]);
>  	swap_cluster_unlock(ci);
> -	return !!count;
> +
> +	return count && count != SWAP_MAP_BAD;
>  }
>  
>  /*
> @@ -1866,7 +1866,7 @@ static bool folio_swapped(struct folio *folio)
>  		return false;
>  
>  	if (!IS_ENABLED(CONFIG_THP_SWAP) || likely(!folio_test_large(folio)))
> -		return swap_entry_swapped(si, entry);
> +		return swap_entry_swapped(si, swp_offset(entry));
>  
>  	return swap_page_trans_huge_swapped(si, entry, folio_order(folio));
>  }
> @@ -3677,10 +3677,10 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
>  		count = si->swap_map[offset + i];
>  
>  		/*
> -		 * swapin_readahead() doesn't check if a swap entry is valid, so the
> -		 * swap entry could be SWAP_MAP_BAD. Check here with lock held.
> +		 * Allocator never allocates bad slots, and readahead is guarded
> +		 * by swap_entry_swapped.
>  		 */
> -		if (unlikely(swap_count(count) == SWAP_MAP_BAD)) {
> +		if (WARN_ON(swap_count(count) == SWAP_MAP_BAD)) {
>  			err = -ENOENT;
>  			goto unlock_out;
>  		}
> 
> -- 
> 2.52.0
>