lkml.org 
[lkml]   [2022]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v5 03/22] cachefiles: introduce on-demand read mode
    From
    Hi,

    Thanks for reviewing.


    On 3/21/22 9:34 PM, David Howells wrote:
    > Jeffle Xu <jefflexu@linux.alibaba.com> 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

    \
     
     \ /
      Last update: 2022-03-21 15:24    [W:4.987 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site