lkml.org 
[lkml]   [2022]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v9 02/21] cachefiles: notify user daemon when looking up cookie
From
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

\
 
 \ /
  Last update: 2022-04-21 16:48    [W:0.263 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site