Huang, Ying <ying.huang@xxxxxxxxx> 于2022年12月11日周日 19:40写道：
>
> 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.
Yes, that's more accurate, it's kind of like 'locked the swap device
from being swapped off', so I used some inaccurate word. I'll correct
this in V2.
>
> And I think it's better to add '()' after swap_cache_get_folio to make
> it clear it's a function.
Good suggestion.
>
> > 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.
Yes, that's the right way to do it, my code is buggy here, sorry for
being so careless, I'll fix it.
>
> 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;
