Messages in this thread | | | From | David Howells <> | Subject | Re: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie | Date | Thu, 21 Apr 2022 14:57:03 +0100 |
| |
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
| |