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

[Jan Kara <jack@suse.cz>]

On Sat 14-03-20 12:54:53, Matthew Wilcox wrote:
> 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.

OK, fair enough.

> 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.

Fine by me and I agree the change is going to be more robust this way. I
just slightly dislike that you end up with __xas_store() with two
underscores which won't have a good meaning after the series is done but I
can certainly live with that :). If you've time to reorganize the series
this week, then go ahead. I've just returned from a week of vacation so it
will take a while for me to catch up... Thanks for having a look!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR