Re: [QUESTION] xas_reload() in iter_xarray_populate_pages()

[Matthew Wilcox <willy@infradead.org>]

On Mon, May 26, 2025 at 09:40:06AM +0200, David Hildenbrand wrote:
> On 26.05.25 08:35, Dev Jain wrote:
> > Hello all,
> > 
> > After doing an xas_load() and xas_retry(), we take neither a reference nor a lock
> > on the folio, and we do an xas_reload(). Is this just to reduce the time window
> > for a race?
> > 
> > If the above is true, then, there is a negligible window between xas_load() and
> > xas_reload(), because only xas_retry() exists between them, so why to even reload()?
> 
> The usual sequence for the pagecache is (see filemap_get_entry())
> 
> 1) xas_load(): Load the entry
> 
> 2) xas_retry(): Test if we have to retry immediately
> 
> 3) folio || xa_is_value(folio): check if the entry stores a folio
> 
> 4) folio_try_get(): try getting a folio reference, might get freed
>    concurrently, so a folio_get() is not safe
> 
> 5) folio != xas_reload(&xas): recheck whether the entry was changed
>    concurrently
> 
> iter_xarray_get_pages()->iter_xarray_populate_pages() works on whatever
> xarray was provided to iov_iter_xarray().
> 
> erofs/netfs/orangefs seem to pass the pagecache ... so I would also assume
> that we have to use the same sequence as above.
> 
> Willy and me had a look ad that code in b57f4f4f186d ("iov_iter: convert
> iter_xarray_populate_pages() to use folios").
> 
> But looking at it now, I think that code is incorrect. At least the
> folio_get() and reload-before-folio-get is weird.

Well, I just converted it; I didn't think hard about what it was doing
was right.  I think I may have mentioned to dhowells that I thought the
xas_reload() was unnecessary as the folios are required to be referenced
by the caller.  So if you can look them up, they're guaranteed to not
be replaced [1].

[1] the xas_retry() cannot be skipped as it guards against a rearrangement
of the tree which can happen even with the folio pinned.