__swap_duplicate() is called by

- swap_shmem_alloc(): the page lock of the swap cache is held.

- copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() ->
  swap_duplicate(): the page table lock is held.

- __read_swap_cache_async() -> swapcache_prepare(): enclosed with
  get/put_swap_device() in __read_swap_cache_async() already.

So, it's safe to remove get/put_swap_device() in __swap_duplicate().

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Yu Zhao <yuzhao@google.com>
---
 mm/swapfile.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index e9cce775fb25..4dbaea64635d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3264,9 +3264,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
 	unsigned char has_cache;
 	int err;
 
-	p = get_swap_device(entry);
-	if (!p)
-		return -EINVAL;
+	p = swp_swap_info(entry);
 
 	offset = swp_offset(entry);
 	ci = lock_cluster_or_swap_info(p, offset);
@@ -3313,7 +3311,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
 
 unlock_out:
 	unlock_cluster_or_swap_info(p, ci);
-	put_swap_device(p);
 	return err;
 }
 
-- 
2.39.2

On Mon, May 22, 2023 at 12:09 AM Huang Ying <ying.huang@intel.com> wrote:
>
> __swap_duplicate() is called by
>
> - swap_shmem_alloc(): the page lock of the swap cache is held.
>
> - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() ->
>   swap_duplicate(): the page table lock is held.
>
> - __read_swap_cache_async() -> swapcache_prepare(): enclosed with
>   get/put_swap_device() in __read_swap_cache_async() already.
>
> So, it's safe to remove get/put_swap_device() in __swap_duplicate().
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: Yu Zhao <yuzhao@google.com>
> ---
>  mm/swapfile.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index e9cce775fb25..4dbaea64635d 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3264,9 +3264,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)

I would add a comment above this function stating that the caller
needs to provide protection against swapoff, and refer to the comment
above get_swap_device().

Otherwise, LGTM with David's comment.

Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

>         unsigned char has_cache;
>         int err;
>
> -       p = get_swap_device(entry);
> -       if (!p)
> -               return -EINVAL;
> +       p = swp_swap_info(entry);
>
>         offset = swp_offset(entry);
>         ci = lock_cluster_or_swap_info(p, offset);
> @@ -3313,7 +3311,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>
>  unlock_out:
>         unlock_cluster_or_swap_info(p, ci);
> -       put_swap_device(p);
>         return err;
>  }
>
> --
> 2.39.2
>
>

On 22.05.23 09:09, Huang Ying wrote:
> __swap_duplicate() is called by
> 
> - swap_shmem_alloc(): the page lock of the swap cache is held.

page lock of the swap cache? Did you really mean to say that or am I 
confused?

"Page lock of the page that is in the swap cache?"

> 
> - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one() ->
>    swap_duplicate(): the page table lock is held.
> 
> - __read_swap_cache_async() -> swapcache_prepare(): enclosed with
>    get/put_swap_device() in __read_swap_cache_async() already.
> 
> So, it's safe to remove get/put_swap_device() in __swap_duplicate().



-- 
Thanks,

David / dhildenb

David Hildenbrand <david@redhat.com> writes:

> On 22.05.23 09:09, Huang Ying wrote:
>> __swap_duplicate() is called by
>> - swap_shmem_alloc(): the page lock of the swap cache is held.
>
> page lock of the swap cache? Did you really mean to say that or am I
> confused?
>
> "Page lock of the page that is in the swap cache?"

Sorry for my poor English. Or make it shorter?

"the folio in the swap cache is locked"

Best Regards,
Huang, Ying

>> - copy_nonpresent_pte() -> swap_duplicate() and try_to_unmap_one()
>> ->
>>    swap_duplicate(): the page table lock is held.
>> - __read_swap_cache_async() -> swapcache_prepare(): enclosed with
>>    get/put_swap_device() in __read_swap_cache_async() already.
>> So, it's safe to remove get/put_swap_device() in __swap_duplicate().

On 23.05.23 02:56, Huang, Ying wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 22.05.23 09:09, Huang Ying wrote:
>>> __swap_duplicate() is called by
>>> - swap_shmem_alloc(): the page lock of the swap cache is held.
>>
>> page lock of the swap cache? Did you really mean to say that or am I
>> confused?
>>
>> "Page lock of the page that is in the swap cache?"
> 
> Sorry for my poor English. Or make it shorter?
> 
> "the folio in the swap cache is locked"

Much clearer, thanks.

-- 
Thanks,

David / dhildenb