Re: [PATCH 5/7] gup: Use folios for gup_devmap

[Matthew Wilcox <willy@infradead.org>]

On Thu, Apr 25, 2024 at 11:49:01AM +0200, David Hildenbrand wrote:
> >   	while ((*nr) - nr_start) {
> > -		struct page *page = pages[--(*nr)];
> > +		struct folio *folio = page_folio(pages[--(*nr)]);
> > -		ClearPageReferenced(page);
> 
> I stumbled over that likely unwarranted ClearPageReferenced() recently as
> well: what if the page was already referenced before we called
> SetPageReferenced?

Yes, it seems bizarre.  I imagine the person who wrote this was trying
to undo everything, but no other cleanup path calls
folio_clear_referenced().  I don't object to taking it out.

> > -		if (flags & FOLL_PIN)
> > -			unpin_user_page(page);
> > -		else
> > -			put_page(page);
> > +		folio_clear_referenced(folio);
> > +		gup_put_folio(folio, 1, flags);
> 
> For !FOLL_PIN, we wouldn't have done the
> 	if (!put_devmap_managed_page_refs(&folio->page, refs))
> 		folio_put_refs(folio, refs);
> 
> Magic in gup_put_folio()
> 
> ... was that a BUG?

I think so.  But probably nobody noticed because it's an error path.
Also nobody noticed that we shouldn't have called SetPageReferenced()
before getting the refcount on the page.  I didn't bother mentioning it
in the changelog because I think it's harmless, just conceptually wrong.