On 2/19/24 2:59 PM, 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();
Blast from the past, guess I did miss it back then. This looks a lot
more reasonable to me, also in terms of overhead, compared to the
previous one.
Heading out for the rest of the week tomorrow morning, but I can give it
a test spin as well once I'm back.
--
Jens Axboe
