Re: [PATCH 2/8] xarray: Provide xas_erase() helper

[Matthew Wilcox <willy@infradead.org>]

On Tue, Feb 04, 2020 at 03:25:08PM +0100, Jan Kara wrote:
> Currently xas_store() clears marks when stored value is NULL. This is
> somewhat counter-intuitive and also causes measurable performance impact
> when mark clearing is not needed (e.g. because marks are already clear).
> So provide xas_erase() helper (similarly to existing xa_erase()) which
> stores NULL at given index and also takes care of clearing marks. Use
> this helper from __xa_erase() and item_kill_tree() in tools/testing.  In
> the following patches, callers that use the mark-clearing property of
> xas_store() will be converted to xas_erase() and remaining users can
> enjoy better performance.

I (finally!) figured out what I don't like about this series.  You're
changing the semantics of xas_store() without changing the name, so
if we have any new users in flight they'll use the new semantics when
they're expecting the old.

Further, while you've split the patches nicely for review, they're not
good for bisection because the semantic change comes right at the end
of the series, so any problem due to this series is going to bisect to
the end, and not tell us anything useful.

What I think this series should do instead is:

Patch 1:
+#define xas_store(xas, entry)	xas_erase(xas, entry)
-void *xas_store(struct xa_state *xas, void *entry)
+void *__xas_store(struct xa_state *xas, void *entry)
-	if (!entry)
-		xas_init_marks(xas);
+void *xas_erase(struct xa_state *xas, void *entry)
+{
+	xas_init_marks(xas);
+	return __xas_store(xas, entry);
+}
(also documentation changes)

Patches 2-n:
Change each user of xas_store() to either xas_erase() or __xas_store()

Patch n+1:
-#define xas_store(xas, entry)  xas_erase(xas, entry)

Does that make sense?  I'll code it up next week unless you want to.