Re: [PATCH 8/8] xarray: Don't clear marks in xas_store()

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Jan Kara <jack@suse.cz>
Cc: Matthew Wilcox <willy@infradead.org>,
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH 8/8] xarray: Don't clear marks in xas_store()
Date: Wed, 5 Feb 2020 14:43:44 -0400	[thread overview]
Message-ID: <20200205184344.GB28298@ziepe.ca> (raw)
In-Reply-To: <20200204142514.15826-9-jack@suse.cz>

On Tue, Feb 04, 2020 at 03:25:14PM +0100, Jan Kara wrote:
> When storing NULL in xarray, xas_store() has been clearing all marks
> because it could otherwise confuse xas_for_each_marked(). That is
> however no longer true and no current user relies on this behavior.
> Furthermore it seems as a cleaner API to not do clearing behind caller's
> back in case we store NULL.
> 
> This provides a nice boost to truncate numbers due to saving unnecessary
> tag initialization when clearing shadow entries. Sample benchmark
> showing time to truncate 128 files 1GB each on machine with 64GB of RAM
> (so about half of entries are shadow entries):
> 
>          AVG      STDDEV
> Vanilla  4.825s   0.036
> Patched  4.516s   0.014
> 
> So we can see about 6% reduction in overall truncate time.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
>  lib/xarray.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/lib/xarray.c b/lib/xarray.c
> index 4e32497c51bd..f165e83652f1 100644
> +++ b/lib/xarray.c
> @@ -799,17 +799,8 @@ void *xas_store(struct xa_state *xas, void *entry)
>  		if (xas->xa_sibs)
>  			xas_squash_marks(xas);
>  	}
> -	if (!entry)
> -		xas_init_marks(xas);
>  
>  	for (;;) {
> -		/*
> -		 * Must clear the marks before setting the entry to NULL,
> -		 * otherwise xas_for_each_marked may find a NULL entry and
> -		 * stop early.  rcu_assign_pointer contains a release barrier
> -		 * so the mark clearing will appear to happen before the
> -		 * entry is set to NULL.
> -		 */
>  		rcu_assign_pointer(*slot, entry);

The above removed comment doesn't sound right (the release is paired
with READ_ONCE, which is only an acquire for data dependent accesses),
is this a reflection of the original bug in this thread?

How is RCU mark reading used anyhow?

There is no guarenteed ordering of the mark and the value, so nothing
iterating under RCU over marks can rely on the marks being accurate.

Are the algorithms using this tolerant of that, or provide some kind
of external locking?

This series looks good to me, and does seem to be an improvement.

Actually the clearing of marks by xa_store(, NULL) is creating a very
subtle bug in drivers/infiniband/core/device.c :( Can you add a Fixes
line too:

ib_set_client_data() is assuming the marks for the entry will not
change, but if the caller passed in NULL they get wrongly reset, and
three call sites pass in NULL:
 drivers/infiniband/ulp/srpt/ib_srpt.c
 net/rds/ib.c
 net/smc/smc_ib.c
Fixes: 0df91bb67334 ("RDMA/devices: Use xarray to store the client_data")

Thanks,
Jason