On Fri, Apr 23, 2021 at 02:28:01PM +0100, David Howells wrote:
> -#define iterate_all_kinds(i, n, v, I, B, K) { \
> +#define iterate_xarray(i, n, __v, skip, STEP) { \
> + struct page *head = NULL; \
> + size_t wanted = n, seg, offset; \
> + loff_t start = i->xarray_start + skip; \
> + pgoff_t index = start >> PAGE_SHIFT; \
> + int j; \
> + \
> + XA_STATE(xas, i->xarray, index); \
> + \
> + rcu_read_lock(); \
> + xas_for_each(&xas, head, ULONG_MAX) { \
> + if (xas_retry(&xas, head)) \
> + continue; \
OK, now I'm really confused; what's to guarantee that restart will not have
you hit the same entry more than once? STEP might be e.g.
memcpy_to_page(v.bv_page, v.bv_offset,
(from += v.bv_len) - v.bv_len, v.bv_len)
which is clearly not idempotent - from gets incremented, after all.
What am I missing here?
> + if (WARN_ON(xa_is_value(head))) \
> + break; \
> + if (WARN_ON(PageHuge(head))) \
> + break; \
> + for (j = (head->index < index) ? index - head->index : 0; \
> + j < thp_nr_pages(head); j++) { \
> + __v.bv_page = head + j; \
> + offset = (i->xarray_start + skip) & ~PAGE_MASK; \
> + seg = PAGE_SIZE - offset; \
> + __v.bv_offset = offset; \
> + __v.bv_len = min(n, seg); \
> + (void)(STEP); \
> + n -= __v.bv_len; \
> + skip += __v.bv_len; \
> + if (n == 0) \
> + break; \
> + } \
> + if (n == 0) \
> + break; \
> + } \
> + rcu_read_unlock(); \
> + n = wanted - n; \
> +}
On Mon, 2021-04-26 at 18:54 +0000, Al Viro wrote:
> On Fri, Apr 23, 2021 at 02:28:01PM +0100, David Howells wrote:
> > -#define iterate_all_kinds(i, n, v, I, B, K) { \
> > +#define iterate_xarray(i, n, __v, skip, STEP) { \
> > + struct page *head = NULL; \
> > + size_t wanted = n, seg, offset; \
> > + loff_t start = i->xarray_start + skip; \
> > + pgoff_t index = start >> PAGE_SHIFT; \
> > + int j; \
> > + \
> > + XA_STATE(xas, i->xarray, index); \
> > + \
> > + rcu_read_lock(); \
> > + xas_for_each(&xas, head, ULONG_MAX) { \
> > + if (xas_retry(&xas, head)) \
> > + continue; \
>
> OK, now I'm really confused; what's to guarantee that restart will not have
> you hit the same entry more than once? STEP might be e.g.
>
> memcpy_to_page(v.bv_page, v.bv_offset,
> (from += v.bv_len) - v.bv_len, v.bv_len)
>
> which is clearly not idempotent - from gets incremented, after all.
> What am I missing here?
>
Not sure I understand the issue you see. If xas_retry returns true,
we'll restart, but we won't have called STEP yet for that entry. I
don't see how we'd retry there and have an issue with idempotency.
> > + if (WARN_ON(xa_is_value(head))) \
> > + break; \
> > + if (WARN_ON(PageHuge(head))) \
> > + break; \
> > + for (j = (head->index < index) ? index - head->index : 0; \
> > + j < thp_nr_pages(head); j++) { \
> > + __v.bv_page = head + j; \
> > + offset = (i->xarray_start + skip) & ~PAGE_MASK; \
> > + seg = PAGE_SIZE - offset; \
> > + __v.bv_offset = offset; \
> > + __v.bv_len = min(n, seg); \
> > + (void)(STEP); \
> > + n -= __v.bv_len; \
> > + skip += __v.bv_len; \
> > + if (n == 0) \
> > + break; \
> > + } \
> > + if (n == 0) \
> > + break; \
> > + } \
> > + rcu_read_unlock(); \
> > + n = wanted - n; \
> > +}
Al Viro <[email protected]> wrote:
> On Fri, Apr 23, 2021 at 02:28:01PM +0100, David Howells wrote:
> > -#define iterate_all_kinds(i, n, v, I, B, K) { \
> > +#define iterate_xarray(i, n, __v, skip, STEP) { \
> > + struct page *head = NULL; \
> > + size_t wanted = n, seg, offset; \
> > + loff_t start = i->xarray_start + skip; \
> > + pgoff_t index = start >> PAGE_SHIFT; \
> > + int j; \
> > + \
> > + XA_STATE(xas, i->xarray, index); \
> > + \
> > + rcu_read_lock(); \
> > + xas_for_each(&xas, head, ULONG_MAX) { \
> > + if (xas_retry(&xas, head)) \
> > + continue; \
>
> OK, now I'm really confused; what's to guarantee that restart will not have
> you hit the same entry more than once? STEP might be e.g.
>
> memcpy_to_page(v.bv_page, v.bv_offset,
> (from += v.bv_len) - v.bv_len, v.bv_len)
>
> which is clearly not idempotent - from gets incremented, after all.
> What am I missing here?
I really need to defer this question to Willy, but as I understand it,
xas_retry() only restarts the current iteration. Referring to the comment on
xas_reset():
* Resets the error or walk state of the @xas so future walks of the
* array will start from the root. Use this if you have dropped the
* xarray lock and want to reuse the xa_state.
I think that the walk returns to the bottom of the tree and whilst xarray
presents an interface that appears to be a contiguous array, it's actually a
tree internally - and 'root' is the root of the tree, not the head of the
array.
Basically, I think it throws away its cached iteration state - which might
have been modified - and rewalks the tree to get back to the same index.
David
On Mon, Apr 26, 2021 at 08:23:31PM +0100, David Howells wrote:
> I really need to defer this question to Willy, but as I understand it,
> xas_retry() only restarts the current iteration. Referring to the comment on
> xas_reset():
>
> * Resets the error or walk state of the @xas so future walks of the
> * array will start from the root. Use this if you have dropped the
> * xarray lock and want to reuse the xa_state.
>
> I think that the walk returns to the bottom of the tree and whilst xarray
> presents an interface that appears to be a contiguous array, it's actually a
> tree internally - and 'root' is the root of the tree, not the head of the
> array.
>
> Basically, I think it throws away its cached iteration state - which might
> have been modified - and rewalks the tree to get back to the same index.
From RTFS(lib/xarray.c) that looks right. Nevermind the question, then...
Anyway,
Reviewed-by: Al Viro <[email protected]>
on the xarray-related bits (this patch + followups)