Hi,
Thanks for reviewing.
On 3/21/22 9:34 PM, David Howells wrote:
> Jeffle Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
>
>> Fscache/cachefiles used to serve as a local cache for remote fs. This
>> patch, along with the following patches, introduces a new on-demand read
>> mode for cachefiles, which can boost the scenario where on-demand read
>> semantics is needed, e.g. container image distribution.
>>
>> The essential difference between the original mode and on-demand read
>> mode is that, in the original mode, when cache miss, netfs itself will
>> fetch data from remote, and then write the fetched data into cache file.
>> While in on-demand read mode, a user daemon is responsible for fetching
>> data and then writing to the cache file.
>>
>> This patch only adds the command to enable on-demand read mode. An optional
>> parameter to "bind" command is added. On-demand mode will be turned on when
>> this optional argument matches "ondemand" exactly, i.e. "bind
>> ondemand". Otherwise cachefiles will keep working in the original mode.
>
> You're not really adding a command, per se. Also, I would recommend
> starting the paragraph with a verb. How about:
>
> Make it possible to enable on-demand read mode by adding an
> optional parameter to the "bind" command. On-demand mode will be
> turned on when this parameter is "ondemand", i.e. "bind ondemand".
> Otherwise cachefiles will work in the original mode.
>
> Also, I'd add a note something like the following:
>
> This is implemented as a variation on the bind command so that it
> can't be turned on accidentally in /etc/cachefilesd.conf when
> cachefilesd isn't expecting it.
Alright, looks much better :)
>
>> The following patches will implement the data plane of on-demand read
>> mode.
>
> I would remove this line. If ondemand mode is not fully implemented in
> cachefiles at this point, I would be tempted to move this to the end of the
> cachefiles subset of the patchset. That said, I'm not sure it can be made
> to do anything much before that point.
Alright.
>
>> +#ifdef CONFIG_CACHEFILES_ONDEMAND
>> +static inline void cachefiles_ondemand_open(struct cachefiles_cache *cache)
>> +{
>> + xa_init_flags(&cache->reqs, XA_FLAGS_ALLOC);
>> + rwlock_init(&cache->reqs_lock);
>> +}
>
> Just merge that into the caller.
>
>> +static inline void cachefiles_ondemand_release(struct cachefiles_cache *cache)
>> +{
>> + xa_destroy(&cache->reqs);
>> +}
>
> Ditto.
>
>> +static inline
>> +bool cachefiles_ondemand_daemon_bind(struct cachefiles_cache *cache, char *args)
>> +{
>> + if (!strcmp(args, "ondemand")) {
>> + set_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags);
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> ...
>> + if (!cachefiles_ondemand_daemon_bind(cache, args) && *args) {
>> + pr_err("'bind' command doesn't take an argument\n");
>> + return -EINVAL;
>> + }
>> +
>
> I would merge these together, I think, and say something like "Ondemand
> mode not enabled in kernel" if CONFIG_CACHEFILES_ONDEMAND=n.
>
The reason why I extract all these logic into small sized function is
that, the **callers** can call cachefiles_ondemand_daemon_bind()
directly without any clause like:
```
#ifdef CONFIG_CACHEFILES_ONDEMAND
...
#else
...
```
Another choice is like
```
if (IS_ENABLED(CONFIG_CACHEFILES_ONDEMAND))
...
else
...
```
--
Thanks,
Jeffle
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
