Re: [RFC PATCH v3] overlayfs: Provide mount options sync=off/fs to skip sync

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Jul 02, 2020 at 10:26:44AM +0300, Amir Goldstein wrote:
[..]
> If you use bitwise flags, they reflect the ovl_should_xxx queries:
> __OVL_NOSYNC_FILE and __OVL_NOSYNC_FS
> #define OVL_SYNC_FILE(type)     (!((type) & __OVL_NOSYNC_FILE))
> #define OVL_SYNC_FS(type)      (!((type) & __OVL_NOSYNC_FS))
> 
> 
> If you use enum (not bitwise), the distinct enum values reflect the
> mount option:
> OVL_SYNC_ON (=0), OVL_SYNC_OFF, OVL_SYNC_FS
> 
> I am not commenting on this because of some sort of aesthetic taste.
> I am commenting on this because I think it would make parts of the
> patch simpler/clearer (see below).
> 
> As far as I am concerned, for the three possible config values off/fs/on
> the distinct enum values are better.
> Of course, that is *my* opinion. You may disagree.

Hi Amir,

I kept bitwise flags because you had mentioned sync=writeback and this
can co-exist with sync=fs. May be somebody wants sync=copyup down the
line. Though we have not implemented sync=writeback
yet, I thought keeping a bitwise flag will help support multiple sync
options at the same time.

Anyway, sync=off/fs are mutually exclusive and don't need bitwise
flags. So for now I will convert this to just enum. When sombody
introduces a sync option which can co-exist with existing options,
they will need to use bit flags.


[..]
> > +               seq_puts(m, ",sync=fs");
> 
> option #1 (bitwise):
>        if (!ofs->config.sync)
>                seq_puts(m, ",sync=off");
>       else if (!OVL_SYNC_FILE(ofs->config.sync))
>                seq_puts(m, ",sync=fs");
> 
> option #2 (distinct):
> Would be better. See ovl_xino_str[].

Will do.

[..]
> > @@ -588,6 +608,17 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
> >                 config->workdir = NULL;
> >         }
> >
> > +       if (OVL_SYNC_OFF(config->sync) && OVL_SYNC_FS(config->sync)) {
> > +               pr_err("conflicting options: sync=off,sync=fs\n");
> > +               return -EINVAL;
> > +       }
> > +
> 
> We are not warning user that metacopy=off conflicts with metacopy=on,
> we just let the last option overwrite previous ones.

Ok, will drop this check.

Thanks
Vivek