Re: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie

[David Howells <dhowells@redhat.com>]

Jeffle Xu <jefflexu@linux.alibaba.com> wrote:

> +	help
> +	  This permits on-demand read mode of cachefiles.  In this mode, when
> +	  cache miss, the cachefiles backend instead of netfs, is responsible
> +	  for fetching data, e.g. through user daemon.

How about:

	help
	  This permits userspace to enable the cachefiles on-demand read mode.
	  In this mode, when a cache miss occurs, responsibility for fetching
	  the data lies with the cachefiles backend instead of with the netfs
	  and is delegated to userspace.

> +	/*
> +	 * 1) Cache has been marked as dead state, and then 2) flush all
> +	 * pending requests in @reqs xarray. The barrier inside set_bit()
> +	 * will ensure that above two ops won't be reordered.
> +	 */

What set_bit()?  What "above two ops"?  And that's not how barriers work; they
provide a partial ordering relative to another pair of barriered ops.

Also, set_bit() can't be relied upon to imply a barrier - see
Documentation/memory-barriers.txt.

> +	if (IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND) &&
> +	    test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags)) {

It might be worth abstracting this into an inline function in internal.h:

	static inline bool cachefiles_in_ondemand_mode(cache)
	{
		return IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND) &&
			test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags)
	}

> +#ifdef CONFIG_CACHEFILES_ONDEMAND

This looks like it ought to be superfluous, given the preceding test - though
I can see why you need it:

> +#ifdef CONFIG_CACHEFILES_ONDEMAND
> +	struct xarray			reqs;		/* xarray of pending on-demand requests */
> +	struct xarray			ondemand_ids;	/* xarray for ondemand_id allocation */
> +	u32				ondemand_id_next;
> +#endif

I'm tempted to say that you should just make them non-conditional.  It's not
like there's likely to be more than one or two cachefiles_cache structs on a
system.

David