Kairui Song <ryncsn@xxxxxxxxx> writes:
> From: Kairui Song <kasong@xxxxxxxxxxx>
>
> There is only one caller not keep holding a reference or lock the
> swap device while calling this function. Just move the lock out
> of this function, it only used to prevent swapoff, and this helper
> function is very short so there is no performance regression
> issue. Help saves a few cycles.
> Subject: Re: [PATCH 4/5] swap: remove the swap lock in swap_cache_get_folio
I don't think you remove `swap lock` in swap_cache_get_folio(). Just
avoid to inc/dec the reference count.
And I think it's better to add '()' after swap_cache_get_folio to make
it clear it's a function.
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
> mm/shmem.c | 8 +++++++-
> mm/swap_state.c | 8 ++------
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index c1d8b8a1aa3b..0183b6678270 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1725,6 +1725,7 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> struct address_space *mapping = inode->i_mapping;
> struct shmem_inode_info *info = SHMEM_I(inode);
> struct mm_struct *charge_mm = vma ? vma->vm_mm : NULL;
> + struct swap_info_struct *si;
> struct folio *folio = NULL;
> swp_entry_t swap;
> int error;
> @@ -1737,7 +1738,12 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
> return -EIO;
>
> /* Look it up and read it in.. */
> - folio = swap_cache_get_folio(swap, NULL, 0);
> + si = get_swap_device(swap);
> + if (si) {
> + folio = swap_cache_get_folio(swap, NULL, 0);
> + put_swap_device(si);
I'd rather to call put_swap_device() at the end of function. That is,
whenever we get a swap entry without proper lock/reference to prevent
swapoff, we should call get_swap_device() to check its validity and
prevent the swap device from swapoff.
Best Regards,
Huang, Ying
> + }
> +
> if (!folio) {
> /* Or update major stats only when swapin succeeds?? */
> if (fault_type) {
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 19089417abd1..eba388f67741 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -324,19 +324,15 @@ static inline bool swap_use_vma_readahead(void)
> * unlocked and with its refcount incremented - we rely on the kernel
> * lock getting page table operations atomic even if we drop the folio
> * lock before returning.
> + *
> + * Caller must lock the swap device or hold a reference to keep it valid.
> */
> struct folio *swap_cache_get_folio(swp_entry_t entry,
> struct vm_area_struct *vma, unsigned long addr)
> {
> struct folio *folio;
> - struct swap_info_struct *si;
>
> - si = get_swap_device(entry);
> - if (!si)
> - return NULL;
> folio = filemap_get_folio(swap_address_space(entry), swp_offset(entry));
> - put_swap_device(si);
> -
> if (folio) {
> bool vma_ra = swap_use_vma_readahead();
> bool readahead;
