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
