Re: [PATCH mm-unstable] mm: migrate: folio_ref_freeze() under xas_lock_irq()

[Hugh Dickins <hughd@google.com>]

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@huawei.com/
> 
> 
> > 
> > Use "expected_count" for the expected count throughout.
> > 
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > ---
> >   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.

Hugh