Messages in this thread | | | Date | Thu, 21 Apr 2022 22:47:25 +0800 | Subject | Re: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie | From | JeffleXu <> |
| |
Hi David,
Thanks for reviewing :)
On 4/21/22 9:57 PM, David Howells wrote: > 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()?
"set_bit(CACHEFILES_DEAD, &cache->flags);" in cachefiles_daemon_release()
> What "above two ops"?
The two operations I mentioned in the comment: 1) Cache has been marked as dead state, and then 2) flush all pending requests in @reqs xarray.
> 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.
Yeah, it seems that set_bit() doesn't imply with a memory barrier, though the x86 implementation (arch/x86/boot/bitops.h) indeed implies a barrier, which may misleads me. Thanks for pointing it out. Then maybe a full barrier is needed here before flushing the @reqs xarray.
> >> + 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) > }
Okay, will be fixed in the next version.
> >> +#ifdef CONFIG_CACHEFILES_ONDEMAND > > This looks like it ought to be superfluous, given the preceding test - though > I can see why you need it:
Sorry I can't see the context. But I guess you are referring to the snippet of cachefiles_daemon_poll()?
``` + if (IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND) && + test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags)) { +#ifdef CONFIG_CACHEFILES_ONDEMAND + if (!xa_empty(&cache->reqs)) + mask |= EPOLLIN; ```
Yes the implementation here is indeed not elegant enough. As you described below, if @reqs is defined non-conditionally in struct cachefiles_cache, then the superfluous magic here is not needed then.
> >> +#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.
Okay, sounds reasonable.
-- Thanks, Jeffle
| |