Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 21 Aug 2021 19:48:09 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     linux-kernel@vger.kernel.org
Cc:     linux-mm@kvack.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v14 112/138] mm/filemap: Convert filemap_get_read_batch
 to use folios
Message-ID: <YSFKab7Md+L0bZMg@casper.infradead.org>
References: <20210715033704.692967-1-willy@infradead.org>
 <20210715033704.692967-113-willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210715033704.692967-113-willy@infradead.org>
Precedence: bulk

On Thu, Jul 15, 2021 at 04:36:38AM +0100, Matthew Wilcox (Oracle) wrote:
>  	rcu_read_lock();
> -	for (head = xas_load(&xas); head; head = xas_next(&xas)) {
> -		if (xas_retry(&xas, head))
> +	for (folio = xas_load(&xas); folio; folio = xas_next(&xas)) {
> +		if (xas_retry(&xas, folio))
>  			continue;
> -		if (xas.xa_index > max || xa_is_value(head))
> +		if (xas.xa_index > max || xa_is_value(folio))
>  			break;
> -		if (!page_cache_get_speculative(head))
> +		if (!folio_try_get_rcu(folio))
>  			goto retry;
>  
> -		/* Has the page moved or been split? */
> -		if (unlikely(head != xas_reload(&xas)))
> +		if (unlikely(folio != xas_reload(&xas)))
>  			goto put_page;
>  
> -		if (!pagevec_add(pvec, head))
> +		if (!pagevec_add(pvec, &folio->page))
>  			break;
> -		if (!PageUptodate(head))
> +		if (!folio_test_uptodate(folio))
>  			break;
> -		if (PageReadahead(head))
> +		if (folio_test_readahead(folio))
>  			break;
> -		xas.xa_index = head->index + thp_nr_pages(head) - 1;
> +		xas.xa_index = folio->index + folio_nr_pages(folio) - 1;
>  		xas.xa_offset = (xas.xa_index >> xas.xa_shift) & XA_CHUNK_MASK;
>  		continue;

It's not a bug in _this_ patch, but these last two lines become a bug
once the page cache is converted to store folios as multi-index entries
(as opposed to now when it replicates an order-N entry 2^N times).
I should not have used xas.xa_shift (which is the shift of the entry
we're looking for and is always 0), but xas.xa_node->shift (which is
the shift of the entry that we found).

If you have an order-7 page, occupying (say) indices 128-255, we set
xa_index to 255, but instead of setting xa_offset to 3, we set it to 63.
That tricks __xas_next() into going up to the parent node, and then back
down, which might mean that we terminate the scan early, or that we skip
over all the other entries in the node.  What I actually noticed was a
crash where we ended up loading an internal entry out of the XArray.

It's all a bit complicated really.  That calls for a helper, and this is
my current candidate:

+static inline void xas_advance(struct xa_state *xas, unsigned long index)
+{
+       unsigned char shift = xas_is_node(xas) ? xas->xa_node->shift : 0;
+
+       xas->xa_index = index;
+       xas->xa_offset = (index >> shift) & XA_CHUNK_MASK;
+}
...
-               xas.xa_index = folio->index + folio_nr_pages(folio) - 1;
-               xas.xa_offset = (xas.xa_index >> xas.xa_shift) & XA_CHUNK_MASK;
+               xas_advance(&xas, folio->index + folio_nr_pages(folio) - 1);

This is coming up on 4 hours of continuous testing using generic/559.
Without it, it would usually crash in about 40 minutes.