On Sun, May 09, 2021 at 08:40:25PM +0100, Matthew Wilcox wrote:
> On Fri, Apr 30, 2021 at 09:50:29AM -0700, Linus Torvalds wrote:
> > On Thu, Apr 29, 2021 at 10:55 PM Andrew Morton
> > <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > + rcu_read_lock();
> > > + xas_for_each(&xas, page, max) {
> > > + if (xas_retry(&xas, page))
> > > + continue;
> > > + if (xa_is_value(page))
> > > + continue;
> > > + if (PageDirty(page) || PageLocked(page) || PageWriteback(page))
> > > + break;
> > > + }
> > > + rcu_read_unlock();
> >
> > Whee. This looks very very expensive indeed.
> >
> > Why is is going to each page, why does it care about locked when the
> > simple early cases did not?
>
> I was hoping Jens would answer this, but I guess he missed it so far.
> I don't know why it now cares about whether the page is locked or not.
> I presume it's not even the slightest bit slow for Jens because the
> common case is that there are no pages in the range, and so we never
> check any pages.
Looking at this again, since the change to operate on folios, we're now
living dangerously as we don't have a reference on the folio, so it can
now be freed, turned into a tail page and trigger the VM_BUG_ON_PGFLAGS()
in folio_flags().
Rather than take a refcount on the folio to prevent that from happening,
we can just examine the pagecache flags in the XArray. like this.
Assuming there really isn't a need to check the locked flag (which for
obvious reasons we don't mirror into the xarray marks).
Jens, WDYT?
diff --git a/mm/filemap.c b/mm/filemap.c
index 750e779c23db..09f3851157f9 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -640,10 +640,8 @@ bool filemap_range_has_writeback(struct address_space *mapping,
xas_for_each(&xas, folio, max) {
if (xas_retry(&xas, folio))
continue;
- if (xa_is_value(folio))
- continue;
- if (folio_test_dirty(folio) || folio_test_locked(folio) ||
- folio_test_writeback(folio))
+ if (xas_get_mark(&xas, PAGECACHE_TAG_DIRTY) ||
+ xas_get_mark(&xas, PAGECACHE_TAG_WRITEBACK))
break;
}
rcu_read_unlock();
