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@xxxxxxxxxx>
> Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # 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
