On Wed, 9 Sep 2020, Alexander Duyck wrote:
> On Tue, Sep 8, 2020 at 6:01 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > On Mon, Aug 24, 2020 at 08:55:04PM +0800, Alex Shi wrote:
> > > +++ b/mm/vmscan.c
> > > @@ -1688,10 +1688,13 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan,
> > >
> > > if (!TestClearPageLRU(page)) {
> > > /*
> > > - * This page may in other isolation path,
> > > - * but we still hold lru_lock.
> > > + * This page is being isolated in another
> > > + * thread, but we still hold lru_lock. The
> > > + * other thread must be holding a reference
> > > + * to the page so this should never hit a
> > > + * reference count of 0.
> > > */
> > > - put_page(page);
> > > + WARN_ON(put_page_testzero(page));
> > > goto busy;
> >
> > I read Hugh's review and that led me to take a look at this. We don't
> > do it like this. Use the same pattern as elsewhere in mm:
> >
> > page_ref_sub(page, nr);
> > VM_BUG_ON_PAGE(page_count(page) <= 0, page);
> >
> >
>
> Actually for this case page_ref_dec(page) would make more sense
> wouldn't it? Otherwise I agree that would be a better change if that
> is the way it has been handled before. I just wasn't familiar with
> those other spots.
After overnight reflection, my own preference would be simply to
drop this patch. I think we are making altogether too much of a
fuss here over what was simply correct as plain put_page()
(and further from correct if we change it to leak the page in an
unforeseen circumstance).
And if Alex's comment was not quite grammatically correct, never mind,
it said as much as was worth saying. I got more worried by his
placement of the "busy:" label, but that does appear to work correctly.
There's probably a thousand places where put_page() is used, where
it would be troublesome if it were the final put_page(): this one
bothered you because you'd been looking at isolate_migratepages_block(),
and its necessary avoidance of lru_lock recursion on put_page();
but let's just just leave this put_page() as is.
Hugh
