| Message ID | 20241101155028.11702-5-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes and cleanups to xarray | expand |
On Fri, Nov 01, 2024 at 11:50:26PM +0800, Kemeng Shi wrote: > If xas_find_marked() failed, there is no need to call xas_store() and > xas_clear_mark(). Just call xas_store() and xas_clear_mark() if > xas_find_marked() succeed. No. The point of the xas interfaces is that they turn into no-ops once an error has occurred. > - else > + else { > *id = xas.xa_index; > - xas_store(&xas, entry); > - xas_clear_mark(&xas, XA_FREE_MARK); > + xas_store(&xas, entry); > + xas_clear_mark(&xas, XA_FREE_MARK); > + } > } while (__xas_nomem(&xas, gfp)); > > return xas_error(&xas); > -- > 2.30.0 >
on 11/2/2024 12:02 AM, Matthew Wilcox wrote: > On Fri, Nov 01, 2024 at 11:50:26PM +0800, Kemeng Shi wrote: >> If xas_find_marked() failed, there is no need to call xas_store() and >> xas_clear_mark(). Just call xas_store() and xas_clear_mark() if >> xas_find_marked() succeed. > > No. The point of the xas interfaces is that they turn into no-ops once > an error has occurred. Yes, xas interfaces can tolerate error. The question is do we really need to call xas_store(...) here if we already know there is no room to store new entry. But no insistant on this as it's not a big deal anyway. Will drop this on next version if you still disklike this. Thanks. > >> - else >> + else { >> *id = xas.xa_index; >> - xas_store(&xas, entry); >> - xas_clear_mark(&xas, XA_FREE_MARK); >> + xas_store(&xas, entry); >> + xas_clear_mark(&xas, XA_FREE_MARK); >> + } >> } while (__xas_nomem(&xas, gfp)); >> >> return xas_error(&xas); >> -- >> 2.30.0 >> >
On Mon, Nov 04, 2024 at 09:55:42AM +0800, Kemeng Shi wrote: > > No. The point of the xas interfaces is that they turn into no-ops once > > an error has occurred. > Yes, xas interfaces can tolerate error. The question is do we really need to > call xas_store(...) here if we already know there is no room to store new entry. > But no insistant on this as it's not a big deal anyway. > > Will drop this on next version if you still disklike this. I very much dislike this. The point is to make the callers simpler.
diff --git a/lib/xarray.c b/lib/xarray.c index 3fac3f2cea9d..012d0ef5e847 100644 --- a/lib/xarray.c +++ b/lib/xarray.c @@ -1860,10 +1860,11 @@ int __xa_alloc(struct xarray *xa, u32 *id, void *entry, xas_find_marked(&xas, limit.max, XA_FREE_MARK); if (xas.xa_node == XAS_RESTART) xas_set_err(&xas, -EBUSY); - else + else { *id = xas.xa_index; - xas_store(&xas, entry); - xas_clear_mark(&xas, XA_FREE_MARK); + xas_store(&xas, entry); + xas_clear_mark(&xas, XA_FREE_MARK); + } } while (__xas_nomem(&xas, gfp)); return xas_error(&xas);
If xas_find_marked() failed, there is no need to call xas_store() and xas_clear_mark(). Just call xas_store() and xas_clear_mark() if xas_find_marked() succeed. Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- lib/xarray.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)