Re: [PATCH 6/8] mm/khugepaged: hoist isolation into collapse_isolate_folio()

[Nico Pache (Red Hat)]

> Signed-off-by: Pedro Falcato <pfalcato@xxxxxxx>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index db1f765b6ead..429e2c5833d0 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2332,6 +2332,69 @@ static enum scan_result prepare_collapse_file_folio(pgoff_t index, struct collap
>  	return ret;
>  }
>  
> +static enum scan_result collapse_isolate_folio(struct collapse_file_state *state)
> +{
> +	struct folio *folio = state->folio;
> +	enum scan_result result;
> +
> +	/*
> +	 * The folio must be locked, so we can drop the i_pages lock
> +	 * without racing with truncate.
> +	 */
> +	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> +
> +	/* make sure the folio is up to date */
> +	if (unlikely(!folio_test_uptodate(folio))) {
> +		result = SCAN_FAIL;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * If file was truncated then extended, or hole-punched, before
> +	 * we locked the first folio, then a THP might be there already.
> +	 * This will be discovered on the first iteration.
> +	 */
> +	if (is_pmd_order(folio_order(folio))) {
> +		result = SCAN_PTE_MAPPED_HUGEPAGE;
> +		goto out_unlock;
> +	}
> +
> +	if (folio_mapping(folio) != state->mapping) {
> +		result = SCAN_TRUNCATED;
> +		goto out_unlock;
> +	}
> +
> +	if (!state->is_shmem && (folio_test_dirty(folio) ||
> +			         folio_test_writeback(folio))) {
> +		/*
> +		 * khugepaged only works on clean file-backed folios,
> +		 * so this folio is dirty because it hasn't been flushed
> +		 * since first write.
> +		 */
> +		result = SCAN_PAGE_DIRTY_OR_WRITEBACK;
> +		goto out_unlock;
> +	}
> +
> +	if (!folio_isolate_lru(folio)) {
> +		result = SCAN_DEL_PAGE_LRU;
> +		goto out_unlock;
> +	}
> +
> +	if (!filemap_release_folio(folio, GFP_KERNEL)) {
> +		result = SCAN_PAGE_HAS_PRIVATE;
> +		folio_putback_lru(folio);
> +		goto out_unlock;
> +	}
> +
> +	if (folio_mapped(folio))
> +		try_to_unmap(folio, TTU_IGNORE_MLOCK | TTU_BATCH_FLUSH);
> +	return SCAN_SUCCEED;
> +out_unlock:
> +	folio_unlock(folio);
> +	folio_put(folio);
> +	return result;
> +}

prepare and isolate now share multiple goto error paths. if you move this into
the prepare function (combining the prepare+isolate). the goto labels at the
bottom become very easy to follow and reduce the code bloat.

With the two additional helpers (shmem vs !shmem) the code becomes very clean,
even without the additional cleanups you did in the previous few patches.

I would suggest doing the very simple restructuring first, then including any
additonal cleanups that you did in this series.

That would not only make it easier to review, but also a lot of the mess
already gets handled just by following my recommended flow.

Cheers,
-- Nico

-- 
Nico Pache (Red Hat) <nico.pache@xxxxxxxxx>