On Mon 19-02-24 21:59:44, Matthew Wilcox wrote:
> 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();
This looks good but it relies on a subtle detail in our writeback handling
that we set the PAGECACHE_TAG_WRITEBACK tag in __folio_start_writeback()
and only after that clear the PAGECACHE_TAG_DIRTY there (although we've
cleared the page dirty bit much earlier in clear_page_dirty_for_io()). So
please add a comment to filemap_range_has_writeback() and to
__folio_start_writeback() about this to at least somewhat reduce the
chances somebody is going to change the code and break this... Thanks!
Honza
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
