Re: [PATCH v6 09/14] mm/mglru: use the common routine for dirty/writeback reactivation

[Kairui Song]

Forwarding sashiko review (the format doesn't seem right, maybe some sashiko bug):

> commit 1f5e2fcf6aa79d5fe07e850245e1292c205ab0fb
> Author: Kairui Song <kasong@xxxxxxxxxxx>
> mm/mglru: use the common routine for dirty/writeback reactivation
> This commit removes MGLRU-specific handling of dirty and writeback folios,
> instead falling back to the classical LRU routine to reactivate them. The
> intention is to improve scan efficiency and keep these folios away from the
> LRU tail until writeback completes.
> > And using the common routine also keeps the folio's referenced bits
> > (tier bits), which could improve metrics in the long term. Also no
> > more need to clean reclaim bit as the common routine will make use
> > of it.
> The commit message notes that this approach keeps the folio's referenced
> (tier) bits.
> When folios are processed by the common routine, they may be marked active
> and subsequently returned to MGLRU via move_folios_to_lru() ->
> lruvec_add_folio() -> lru_gen_add_folio().
> While lru_gen_add_folio() updates the generation, it does not clear
> LRU_REFS_FLAGS. Should the tier bits be explicitly cleared when a folio
> moves to a new generation so tier tracking starts fresh?
> If stale tier bits are retained across generation movements, could this
> artificially inflate access counts and disrupt the eviction algorithm?

I don't think that's an issue, writeback folios are moved to tail (oldest
gen) once writeback is done. And in real tests this seems actually an
improvement.

> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 35e3352a53e3bc..74255efc4ad9dd 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> [ ... ]
> > @@ -4628,21 +4627,6 @@ static bool sort_folio(struct lruvec *lruvec, struct folio *folio, struct scan_c
> >  		return true;
> >  	}
> >  
> > -	dirty = folio_test_dirty(folio);
> > -	writeback = folio_test_writeback(folio);
> > -	if (type == LRU_GEN_FILE && dirty) {
> > -		sc->nr.file_taken += delta;
> > -		if (!writeback)
> > -			sc->nr.unqueued_dirty += delta;
> > -	}
> > -
> > -	/* waiting for writeback */
> > -	if (writeback || (type == LRU_GEN_FILE && dirty)) {
> > -		gen = folio_inc_gen(lruvec, folio, true);
> > -		list_move(&folio->lru, &lrugen->folios[gen][type][zone]);
> > -		return true;
> > -	}
> > -
> >  	return false;
> >  }
> The removed folio_inc_gen() path previously cleared LRU_REFS_FLAGS when
> updating the generation. Can the new fallback approach cause regressions in
> tier tracking by skipping this clearing step?

Same as above, that's not an issue but expected, even an improvement
for many cases.