On Fri, Apr 30, 2021 at 09:50:29AM -0700, Linus Torvalds wrote:
> Wait, what?
>
> On Thu, Apr 29, 2021 at 10:55 PM Andrew Morton
> <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > + if (!mapping_needs_writeback(mapping))
> > + return false;
> > + if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY) &&
> > + !mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK))
> > + return false;
> > + if (end_byte < start_byte)
> > + return false;
>
> Ok, good, get rid of the trivial cases first.
>
> > + 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.
> Why isn't this just using xas_for_each_marked(), or maybe even just a
> single "xa_find()", since all it cares about is "is _any_ page.."
We don't have an iterator that looks for either mark A or mark B,
so we'd have to iterate twice, once to look for dirty and once to
look for writeback. For the common case of there being no pages in
the range, that'd be slower.
I could add an iterator that returns the next entry in the given range
that has any mark set (almost trivial; OR the mark bitmaps together
before scanning them). But I don't know why we care about the Locked
case, so I don't know if it'd be the right thing to do.
xa_find() isn't quite right because that'll return shadow entries, and
we do want to skip those.
