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.
