Re: [PATCH 07/10] mm/khugepaged: minor reorderings in collapse_shmem()

["Kirill A. Shutemov" <kirill@shutemov.name>]

On Mon, Nov 26, 2018 at 03:27:52PM -0800, Hugh Dickins wrote:
> Several cleanups in collapse_shmem(): most of which probably do not
> really matter, beyond doing things in a more familiar and reassuring
> order.  Simplify the failure gotos in the main loop, and on success
> update stats while interrupts still disabled from the last iteration.
> 
> Fixes: f3f0e1d2150b2 ("khugepaged: add support of collapse for tmpfs/shmem pages")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Cc: stable@vger.kernel.org # 4.8+
> ---
>  mm/khugepaged.c | 72 ++++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 40 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 1c402d33547e..9d4e9ff1af95 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -1329,10 +1329,10 @@ static void collapse_shmem(struct mm_struct *mm,
>  		goto out;
>  	}
>  
> +	__SetPageLocked(new_page);
> +	__SetPageSwapBacked(new_page);
>  	new_page->index = start;
>  	new_page->mapping = mapping;
> -	__SetPageSwapBacked(new_page);
> -	__SetPageLocked(new_page);
>  	BUG_ON(!page_ref_freeze(new_page, 1));
>  
>  	/*
> @@ -1366,13 +1366,13 @@ static void collapse_shmem(struct mm_struct *mm,
>  			if (index == start) {
>  				if (!xas_next_entry(&xas, end - 1)) {
>  					result = SCAN_TRUNCATED;
> -					break;
> +					goto xa_locked;
>  				}
>  				xas_set(&xas, index);
>  			}
>  			if (!shmem_charge(mapping->host, 1)) {
>  				result = SCAN_FAIL;
> -				break;
> +				goto xa_locked;
>  			}
>  			xas_store(&xas, new_page + (index % HPAGE_PMD_NR));
>  			nr_none++;
> @@ -1387,13 +1387,12 @@ static void collapse_shmem(struct mm_struct *mm,
>  				result = SCAN_FAIL;
>  				goto xa_unlocked;
>  			}
> -			xas_lock_irq(&xas);
> -			xas_set(&xas, index);
>  		} else if (trylock_page(page)) {
>  			get_page(page);
> +			xas_unlock_irq(&xas);
>  		} else {
>  			result = SCAN_PAGE_LOCK;
> -			break;
> +			goto xa_locked;
>  		}
>  
>  		/*

I'm puzzled by locking change here.

Isn't the change responsible for the bug you are fixing in 09/10?

IIRC, my intend for the locking scheme was to protect against
truncate-repopulate race.

What do I miss?

The rest of the patch *looks* okay, but I found it hard to follow.
Splitting it up would make it easier.

-- 
 Kirill A. Shutemov