Re: [PATCH v11 19/63] page cache: Convert page deletion to XArray

[Matthew Wilcox <willy@infradead.org>]

On Fri, Apr 20, 2018 at 07:00:57AM -0500, Goldwyn Rodrigues wrote:
> On 04/14/2018 09:12 AM, Matthew Wilcox wrote:
> >  
> > -	for (i = 0; i < nr; i++) {
> > -		struct radix_tree_node *node;
> > -		void **slot;
> > -
> > -		__radix_tree_lookup(&mapping->i_pages, page->index + i,
> > -				    &node, &slot);
> > -
> > -		VM_BUG_ON_PAGE(!node && nr != 1, page);
> > -
> > -		radix_tree_clear_tags(&mapping->i_pages, node, slot);
> > -		__radix_tree_replace(&mapping->i_pages, node, slot, shadow,
> > -				workingset_lookup_update(mapping));
> > +	i = nr;
> > +repeat:
> > +	xas_store(&xas, shadow);
> > +	xas_init_tags(&xas);
> > +	if (--i) {
> > +		xas_next(&xas);
> > +		goto repeat;
> >  	}
> 
> Can this be converted into a do {} while (or even for) loop instead?
> Loops are easier to read and understand in such a situation.

I wish I'd fixed this up earlier because our peers made fun of me at
LSFMM for this loop ;-)

The obvious way to write this loop is:

        for (i = 0; i < nr; i++) {
-               struct radix_tree_node *node;
-               void **slot;
-
-               __radix_tree_lookup(&mapping->i_pages, page->index + i,
-                                   &node, &slot);
-
-               VM_BUG_ON_PAGE(!node && nr != 1, page);
-
-               radix_tree_clear_tags(&mapping->i_pages, node, slot);
-               __radix_tree_replace(&mapping->i_pages, node, slot, shadow,
-                               workingset_lookup_update(mapping));
+               xas_store(&xas, shadow);
+               xas_init_tags(&xas);
+               xas_next(&xas);
        }

But since we're storing the same value to every entry and the range is
a power of two, there's a better way to rewrite this:

 {
-       int i, nr;
+       XA_STATE(xas, &mapping->i_pages, page->index);
+       unsigned int nr = 1;

-       /* hugetlb pages are represented by one entry in the radix tree */
-       nr = PageHuge(page) ? 1 : hpage_nr_pages(page);
+       mapping_set_update(&xas, mapping);
+
+       /* hugetlb pages are represented by a single entry in the xarray */
+       if (!PageHuge(page)) {
+               xas_set_order(&xas, page->index, compound_order(page));
+               nr = 1U << compound_order(page);
+       }

        VM_BUG_ON_PAGE(!PageLocked(page), page);
        VM_BUG_ON_PAGE(PageTail(page), page);
        VM_BUG_ON_PAGE(nr != 1 && shadow, page);

-       for (i = 0; i < nr; i++) {
-               struct radix_tree_node *node;
-               void **slot;
-
-               __radix_tree_lookup(&mapping->i_pages, page->index + i,
-                                   &node, &slot);
-
-               VM_BUG_ON_PAGE(!node && nr != 1, page);
-
-               radix_tree_clear_tags(&mapping->i_pages, node, slot);
-               __radix_tree_replace(&mapping->i_pages, node, slot, shadow,
-                               workingset_lookup_update(mapping));
-       }
+       xas_store(&xas, shadow);
+       xas_init_tags(&xas);