* [PATCH v2 0/2] overlayfs: Handle metacopy mount option better @ 2018-10-30 20:26 Vivek Goyal 2018-10-30 20:26 ` [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled Vivek Goyal 2018-10-30 20:26 ` [PATCH v2 2/2] overlayfs: Enable redirect_dir automatically if metacopy=on needs it Vivek Goyal 0 siblings, 2 replies; 4+ messages in thread From: Vivek Goyal @ 2018-10-30 20:26 UTC (permalink / raw) To: linux-unionfs; +Cc: miklos, amir73il, vgoyal This is V2 of the patch I had posted. Right now if user passes metacopy=on and if redirect_dir=on is not specified, metacopy is disabled automatically and a messages is sent to console/logs. This might be little surprising for user. A better behavior probably is to return an error if metacopy can't be enabled. Two patches in the series take care of that. Vivek Goyal (2): overlayfs: Return error if metadata only copy-up can't be enabled overlayfs: Enable redirect_dir automatically if metacopy=on needs it fs/overlayfs/ovl_entry.h | 2 ++ fs/overlayfs/super.c | 73 +++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 7 deletions(-) -- 2.13.6 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled 2018-10-30 20:26 [PATCH v2 0/2] overlayfs: Handle metacopy mount option better Vivek Goyal @ 2018-10-30 20:26 ` Vivek Goyal 2018-10-30 21:16 ` Amir Goldstein 2018-10-30 20:26 ` [PATCH v2 2/2] overlayfs: Enable redirect_dir automatically if metacopy=on needs it Vivek Goyal 1 sibling, 1 reply; 4+ messages in thread From: Vivek Goyal @ 2018-10-30 20:26 UTC (permalink / raw) To: linux-unionfs; +Cc: miklos, amir73il, vgoyal Assume a user wanted to enable metadata only copy-up and passes metacopy=on. If this feature can't be enabled, we disable metacopy=off and just leave a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) or redirect_dir=follow (for non-upper mount). As user does not see mount failure, he/she assumes metadata only copy-up has been enabled but that's not the case. So instead of disabling metacopy, return an error to user and leave a message in logs. That will allow user to fix mount options and retry. This is done only if user specified metacopy=on in mount options. If metacopy is enabled as default either through module command line or kernel Kconfig, that's not enforced and it can be disabled automatically if system configuration does not permit it. Reported-by: Daniel Walsh <dwalsh@redhat.com> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 52 +++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index ec237035333a..23bd7507bf2c 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -20,6 +20,7 @@ struct ovl_config { bool nfs_export; int xino; bool metacopy; + bool metacopy_enforce; }; struct ovl_sb { diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 30adc9d408a0..19f50783d92d 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -468,6 +468,41 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) return 0; } +static int ovl_process_metacopy_dependency(struct ovl_config *config) +{ + if (!config->metacopy) + return 0; + + if (config->upperdir && !config->redirect_dir) { + /* metacopy feature with upper requires redirect_dir=on */ + if (!config->metacopy_enforce) { + pr_warn("overlayfs: metadata only copy up requires" + " \"redirect_dir=on\", falling back to" + " metacopy=off\n"); + config->metacopy = false; + return 0; + } + pr_err("overlayfs: metadata only copy up requires" + " \"redirect_dir=on\".\n"); + return -EINVAL; + } else if (!config->upperdir && !config->redirect_follow) { + if (!config->metacopy_enforce) { + pr_warn("overlayfs: metadata only copy up requires" + " either \"redirect_dir=follow\" or" + " \"redirect_dir=on\" on non-upper mount," + " falling back to metacopy=off\n"); + config->metacopy = false; + return 0; + } + pr_err("overlayfs: metadata only copy up requires either" + " \"redirect_dir=follow\" or \"redirect_dir=on\" on" + " non-upper mount.\n"); + return -EINVAL; + } + + return 0; +} + static int ovl_parse_opt(char *opt, struct ovl_config *config) { char *p; @@ -548,6 +583,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) case OPT_METACOPY_ON: config->metacopy = true; + config->metacopy_enforce = true; break; case OPT_METACOPY_OFF: @@ -572,13 +608,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (err) return err; - /* metacopy feature with upper requires redirect_dir=on */ - if (config->upperdir && config->metacopy && !config->redirect_dir) { - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=on\", falling back to metacopy=off.\n"); - config->metacopy = false; - } else if (config->metacopy && !config->redirect_follow) { - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n"); - config->metacopy = false; + if (config->metacopy) { + err = ovl_process_metacopy_dependency(config); + if (err) + return err; } return 0; @@ -1055,6 +1088,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath) if (err) { ofs->noxattr = true; ofs->config.index = false; + if (ofs->config.metacopy && ofs->config.metacopy_enforce) { + pr_err("overlayfs: can not support metacopy=on as upper fs does not support xattr.\n"); + err = -EINVAL; + goto out; + } ofs->config.metacopy = false; pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n"); err = 0; -- 2.13.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled 2018-10-30 20:26 ` [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled Vivek Goyal @ 2018-10-30 21:16 ` Amir Goldstein 0 siblings, 0 replies; 4+ messages in thread From: Amir Goldstein @ 2018-10-30 21:16 UTC (permalink / raw) To: Vivek Goyal; +Cc: overlayfs, Miklos Szeredi On Tue, Oct 30, 2018 at 10:26 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > Assume a user wanted to enable metadata only copy-up and passes metacopy=on. > If this feature can't be enabled, we disable metacopy=off and just leave > a warning in logs. metacopy=on requires redirect_dir=on (for upper dir) > or redirect_dir=follow (for non-upper mount). > > As user does not see mount failure, he/she assumes metadata only copy-up > has been enabled but that's not the case. > > So instead of disabling metacopy, return an error to user and leave a > message in logs. That will allow user to fix mount options and retry. > This is done only if user specified metacopy=on in mount options. If > metacopy is enabled as default either through module command line or > kernel Kconfig, that's not enforced and it can be disabled automatically > if system configuration does not permit it. > > Reported-by: Daniel Walsh <dwalsh@redhat.com> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- I think we can do better. This could have been much more generic. Maybe I am trying to take this too far, but IMO some effort towards more generic options handling is in order. Some suggestions below. > fs/overlayfs/ovl_entry.h | 1 + > fs/overlayfs/super.c | 52 +++++++++++++++++++++++++++++++++++++++++------- > 2 files changed, 46 insertions(+), 7 deletions(-) > > diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h > index ec237035333a..23bd7507bf2c 100644 > --- a/fs/overlayfs/ovl_entry.h > +++ b/fs/overlayfs/ovl_entry.h > @@ -20,6 +20,7 @@ struct ovl_config { > bool nfs_export; > int xino; > bool metacopy; > + bool metacopy_enforce; This could have been a bitmap, set for every option during parsing with the token enum as index, e.g.: #define OVL_OPT_METACOPY_ON (1<<OPT_METACOPY_ON) #define OVL_OPT_METACOPY_OFF (1<<OPT_METACOPY_OFF) #define OVL_OPT_METACOPY (OVL_OPT_METACOPY_ON|OVL_OPT_METACOPY_OFF) > }; > > struct ovl_sb { > diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c > index 30adc9d408a0..19f50783d92d 100644 > --- a/fs/overlayfs/super.c > +++ b/fs/overlayfs/super.c > @@ -468,6 +468,41 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) > return 0; > } > > +static int ovl_process_metacopy_dependency(struct ovl_config *config) > +{ > + if (!config->metacopy) > + return 0; > + > + if (config->upperdir && !config->redirect_dir) { > + /* metacopy feature with upper requires redirect_dir=on */ > + if (!config->metacopy_enforce) { > + pr_warn("overlayfs: metadata only copy up requires" > + " \"redirect_dir=on\", falling back to" > + " metacopy=off\n"); > + config->metacopy = false; > + return 0; > + } > + pr_err("overlayfs: metadata only copy up requires" > + " \"redirect_dir=on\".\n"); This pattern of warning or error with the same "requires" text repeats 3 times in this patch alone. it could have been a helper: config->metacopy = false; return ovl_option_dependency(config, OVL_OPT_METACOPY, "overlayfs: metadata only copy up requires" " \"redirect_dir=on\".\n"); > + return -EINVAL; > + } else if (!config->upperdir && !config->redirect_follow) { > + if (!config->metacopy_enforce) { > + pr_warn("overlayfs: metadata only copy up requires" > + " either \"redirect_dir=follow\" or" > + " \"redirect_dir=on\" on non-upper mount," > + " falling back to metacopy=off\n"); > + config->metacopy = false; > + return 0; > + } > + pr_err("overlayfs: metadata only copy up requires either" > + " \"redirect_dir=follow\" or \"redirect_dir=on\" on" > + " non-upper mount.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int ovl_parse_opt(char *opt, struct ovl_config *config) > { > char *p; > @@ -548,6 +583,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > > case OPT_METACOPY_ON: > config->metacopy = true; > + config->metacopy_enforce = true; > break; > > case OPT_METACOPY_OFF: > @@ -572,13 +608,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) > if (err) > return err; > > - /* metacopy feature with upper requires redirect_dir=on */ > - if (config->upperdir && config->metacopy && !config->redirect_dir) { > - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=on\", falling back to metacopy=off.\n"); > - config->metacopy = false; > - } else if (config->metacopy && !config->redirect_follow) { > - pr_warn("overlayfs: metadata only copy up requires \"redirect_dir=follow\" on non-upper mount, falling back to metacopy=off.\n"); > - config->metacopy = false; > + if (config->metacopy) { > + err = ovl_process_metacopy_dependency(config); > + if (err) > + return err; > } > > return 0; > @@ -1055,6 +1088,11 @@ static int ovl_make_workdir(struct ovl_fs *ofs, struct path *workpath) > if (err) { > ofs->noxattr = true; > ofs->config.index = false; > + if (ofs->config.metacopy && ofs->config.metacopy_enforce) { > + pr_err("overlayfs: can not support metacopy=on as upper fs does not support xattr.\n"); > + err = -EINVAL; > + goto out; > + } > ofs->config.metacopy = false; > pr_warn("overlayfs: upper fs does not support xattr, falling back to index=off and metacopy=off.\n"); > err = 0; ofs->config.metacopy = false; err = ovl_option_dependency(config, OVL_OPT_METACOPY, "overlayfs: metadata only copy up requires" "upper fs xattr support.\n"); Thanks, Amir. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] overlayfs: Enable redirect_dir automatically if metacopy=on needs it 2018-10-30 20:26 [PATCH v2 0/2] overlayfs: Handle metacopy mount option better Vivek Goyal 2018-10-30 20:26 ` [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled Vivek Goyal @ 2018-10-30 20:26 ` Vivek Goyal 1 sibling, 0 replies; 4+ messages in thread From: Vivek Goyal @ 2018-10-30 20:26 UTC (permalink / raw) To: linux-unionfs; +Cc: miklos, amir73il, vgoyal metacopy feature requires either redirect_dir=on or redirect_dir=follow depending on if upper directory is present or not. Enable this feature automatically, if needed. It can't be enabled automatically enabled if user has disabled this feature explicitly using redirect_dir mount option. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/overlayfs/ovl_entry.h | 1 + fs/overlayfs/super.c | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index 23bd7507bf2c..8667ef60dfa9 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -15,6 +15,7 @@ struct ovl_config { bool default_permissions; bool redirect_dir; bool redirect_follow; + bool redirect_enforce; const char *redirect_mode; bool index; bool nfs_export; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index 19f50783d92d..0d6d69c7012a 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -468,12 +468,29 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) return 0; } +static bool ovl_enable_redirect(struct ovl_config *config, bool follow) { + if (config->redirect_enforce) + return false; + + config->redirect_follow = true; + if (!follow) { + config->redirect_dir = true; + pr_info("overlayfs: Enabling \"redirect_dir=on\".\n"); + } else { + pr_info("overlayfs: Enabling \"redirect_dir=follow\".\n"); + } + return true; +} + static int ovl_process_metacopy_dependency(struct ovl_config *config) { if (!config->metacopy) return 0; if (config->upperdir && !config->redirect_dir) { + if (ovl_enable_redirect(config, false)) + return 0; + /* metacopy feature with upper requires redirect_dir=on */ if (!config->metacopy_enforce) { pr_warn("overlayfs: metadata only copy up requires" @@ -486,6 +503,9 @@ static int ovl_process_metacopy_dependency(struct ovl_config *config) " \"redirect_dir=on\".\n"); return -EINVAL; } else if (!config->upperdir && !config->redirect_follow) { + if (ovl_enable_redirect(config, true)) + return 0; + if (!config->metacopy_enforce) { pr_warn("overlayfs: metadata only copy up requires" " either \"redirect_dir=follow\" or" @@ -551,6 +571,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->redirect_mode = match_strdup(&args[0]); if (!config->redirect_mode) return -ENOMEM; + config->redirect_enforce = true; break; case OPT_INDEX_ON: -- 2.13.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-31 5:21 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-10-30 20:26 [PATCH v2 0/2] overlayfs: Handle metacopy mount option better Vivek Goyal 2018-10-30 20:26 ` [PATCH v2 1/2] overlayfs: Return error if metadata only copy-up can't be enabled Vivek Goyal 2018-10-30 21:16 ` Amir Goldstein 2018-10-30 20:26 ` [PATCH v2 2/2] overlayfs: Enable redirect_dir automatically if metacopy=on needs it Vivek Goyal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox