On 2024/6/26 3:51, Hugh Dickins wrote:
On Tue, 25 Jun 2024, Kefeng Wang wrote:
On 2024/6/25 13:04, Hugh Dickins wrote:
Commit "mm: migrate: split folio_migrate_mapping()" drew attention to
"Note, the folio_ref_freeze() is moved out of xas_lock_irq(), Since the
folio is already isolated and locked during migration, so suppose that
there is no functional change."
That was a mistake. Freezing a folio's refcount to 0 is much like taking
a spinlock: see how filemap_get_entry() takes rcu_read_lock() then spins
around until the folio is unfrozen. If the task freezing is preempted (or
calls cond_resched(), as folio_mc_copy() may do), then it risks deadlock:
in my case, one CPU in zap_pte_range() with free_swap_and_cache_nr()
trying to reclaim swap while PTL is held, all the other CPUs in reclaim
spinning for that PTL.
Oh, thanks for pointing this out, I didn't take that into account.
I'm uncertain whether it's necessary for interrupts to be disabled as
well as preemption, but since they have to be disabled for the page
cache migration, it's much the best to do it all together as before.
So revert to folio_ref_freeze() under xas_lock_irq(): but keep the
preliminary folio_ref_count() check, which does make sense before
trying to copy the folio's data.
This is what my RFC version[1] does, which adds same reference check to
avoid the unnecessary folio_mc_copy().
Should I resend all patches, or Andrew directly pick this one?
Andrew asks for a resend: I was only aiming to fix the bug, and
have no perspective on how much of the series remains worthwhile.
Thanks.
[1]
https://lore.kernel.org/linux-mm/20240129070934.3717659-7-wangkefeng.wang@xxxxxxxxxx/
Use "expected_count" for the expected count throughout.
Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
---
mm/migrate.c | 59 +++++++++++++++++++++++++---------------------------
1 file changed, 28 insertions(+), 31 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 27f070f64f27..8beedbb42a93 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -400,8 +400,8 @@ static int folio_expected_refs(struct address_space
*mapping,
* 2 for folios with a mapping
* 3 for folios with a mapping and PagePrivate/PagePrivate2 set.
*/
-static void __folio_migrate_mapping(struct address_space *mapping,
- struct folio *newfolio, struct folio *folio, int expected_cnt)
+static int __folio_migrate_mapping(struct address_space *mapping,
+ struct folio *newfolio, struct folio *folio, int
expected_count)
We can rename it back to folio_migrate_mapping().
Almost. I did want to remove the __ layer, but internally the last
parameter is expected_count, whereas externally it is extra_count:
each has merit in its place, so I left them as is.
Indeed， I refresh all patches, we still need this
__filemap_migrate_mapping, thanks.
Hugh
