Re: [PATCH] mm for mmotm: Revert skip swap cache feture for synchronous device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 2018-01-03 at 08:56 +0900, Minchan Kim wrote:
> On Tue, Jan 02, 2018 at 02:42:21PM -0800, James Bottomley wrote:
> > 
> > On Tue, 2018-01-02 at 13:22 -0800, Andrew Morton wrote:
> > > 
> > > On Fri, 29 Dec 2017 09:55:07 +0900 Minchan Kim <minchan@xxxxxxxxx
> > > g>
> > > wrote:
> > > 
> > > > 
> > > > 
> > > > James reported a bug of swap paging-in for his testing and
> > > > found it
> > > > at rc5, soon to be -rc5.
> > > > 
> > > > Although we can fix the specific problem at the moment, it may
> > > > have other lurkig bugs so want to have one more cycle in -next
> > > > before merging.
> > > > 
> > > > This patchset reverts 23c47d2ada9f, 08fa93021d80, 8e31f339295f
> > > > completely
> > > > but 79b5f08fa34e partially because the swp_swap_info function
> > > > that
> > > > 79b5f08fa34e introduced is used by [1].
> > > 
> > > Gets a significant reject in do_swap_page().  Could you please
> > > take a
> > > look, redo against current mainline?
> > > 
> > > Or not.  We had a bug and James fixed it.  That's what -rc is
> > > for.  Why not fix the thing and proceed?
> > 
> > My main worry was lack of testing at -rc5, since the bug could
> > essentially be excited by pushing pages out to swap and then trying
> > to
> > access them again ... plus since one serious bug was discovered it
> > wouldn't be unusual for there to be others.  However, because of
> > the
> > IPT stuff, I think Linus is going to take 4.15 over a couple of
> > extra
> > -rc releases, so this is less of a problem.
> 
> Then, Here is right fix patch against current mainline.
> 
> 
> From 012bdb0774744455ab7aa8abd74c8b9ca1cdc009 Mon Sep 17 00:00:00
> 2001
> From: Minchan Kim <minchan@xxxxxxxxxx>
> Date: Wed, 3 Jan 2018 08:25:15 +0900
> Subject: [PATCH] mm: release locked page in do_swap_page
> 
> James reported a bug of swap paging-in for his testing. It is that
> do_swap_page doesn't release locked page so system hang-up happens
> by deadlock of PG_locked.
> 
> It was introduced by [1] because I missed swap cache hit places to
> update swapcache variable to work well with other logics against
> swapcache in do_swap_page.
> 
> This patch fixes it.
> 
> [1] 0bcac06f27d7, mm, swap: skip swapcache for swapin of synchronous
> device
> 
> Link: http://lkml.kernel.org/r/<1514407817.4169.4.camel@HansenPartner
> ship.com>;
> Cc: Hugh Dickins <hughd@xxxxxxxxxx>
> Cc: Sergey Senozhatsky <sergey.senozhatsky@xxxxxxxxx>
> Cc: Huang Ying <ying.huang@xxxxxxxxx>
> Debugged-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> ---
>  mm/memory.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index ca5674cbaff2..793004608332 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2857,8 +2857,11 @@ int do_swap_page(struct vm_fault *vmf)
>  	int ret = 0;
>  	bool vma_readahead = swap_use_vma_readahead();
>  
> -	if (vma_readahead)
> +	if (vma_readahead) {
>  		page = swap_readahead_detect(vmf, &swap_ra);
> +		swapcache = page;
> +	}
> +
>  	if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf-
> >orig_pte)) {
>  		if (page)
>  			put_page(page);
> @@ -2889,9 +2892,12 @@ int do_swap_page(struct vm_fault *vmf)
>  
>  
>  	delayacct_set_flag(DELAYACCT_PF_SWAPIN);
> -	if (!page)
> +	if (!page) {
>  		page = lookup_swap_cache(entry, vma_readahead ? vma
> : NULL,
>  					 vmf->address);
> +		swapcache = page;
> +	}
> +

I've got to say I prefer my version.  The problem with the above is
that if something else gets added to this path and forgets to set
swapcache = page you'll get the locked pages problem back.

Instead of setting swapcache to NULL at the top, don't set it until it
matters, which is just before the second if (!page).  It doesn't matter
before this because you're using it as a signal for the synchronous I/O
path, so why have a whole section of code where you invite people to
get it wrong for no benefit.

James

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]
  Powered by Linux