| Message ID | d307f1d95415232dabfb700e79cda73618aa7d50.1600961206.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New rescue mount options | expand |
On 2020/9/24 下午11:32, Josef Bacik wrote: > We're going to be adding more options that require RDONLY, so add a > helper to do the check and error out if we don't have RDONLY set. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/super.c | 23 ++++++++++++++++------- > 1 file changed, 16 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 8840a4fa81eb..f99e89ec46b2 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -458,6 +458,17 @@ static const match_table_t rescue_tokens = { > {Opt_err, NULL}, > }; > > +static bool check_ro_option(struct btrfs_fs_info *fs_info, unsigned long opt, > + const char *opt_name) > +{ > + if (fs_info->mount_opt & opt) { > + btrfs_err(fs_info, "%s must be used with ro mount option", > + opt_name); > + return true; > + } > + return false; > +} > + > static int parse_rescue_options(struct btrfs_fs_info *info, const char *options) > { > char *opts; > @@ -968,14 +979,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > } > } > check: > - /* > - * Extra check for current option against current flag > - */ > - if (btrfs_test_opt(info, NOLOGREPLAY) && !(new_flags & SB_RDONLY)) { > - btrfs_err(info, > - "nologreplay must be used with ro mount option"); > + /* We're read-only, don't have to check. */ > + if (new_flags & SB_RDONLY) > + goto out; > + > + if (check_ro_option(info, BTRFS_MOUNT_NOLOGREPLAY, "nologreplay")) > ret = -EINVAL; > - } > out: > if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) && > !btrfs_test_opt(info, FREE_SPACE_TREE) && >
On 24/09/2020 17:33, Josef Bacik wrote: > We're going to be adding more options that require RDONLY, so add a > helper to do the check and error out if we don't have RDONLY set. > > + /* We're read-only, don't have to check. */ > + if (new_flags & SB_RDONLY) > + goto out; > + Why aren't you moving the SB_RDONLY check into the new check_ro_option() as well? This is what I would have thought this patch does after just reading the commit message. Thanks, Johannes
On 9/28/20 8:37 AM, Johannes Thumshirn wrote: > On 24/09/2020 17:33, Josef Bacik wrote: >> We're going to be adding more options that require RDONLY, so add a >> helper to do the check and error out if we don't have RDONLY set. >> >> + /* We're read-only, don't have to check. */ >> + if (new_flags & SB_RDONLY) >> + goto out; >> + > > Why aren't you moving the SB_RDONLY check into the new check_ro_option() as well? > This is what I would have thought this patch does after just reading the commit message. > To avoid the multiple calls if we're not read only, otherwise it'll be multiple function calls to check that that SB_RDONLY is set. The compiler will probably optimize that away, but I just went with this instead. I'm good either way if people have strong opinions one way or the other. Thanks, Josef
On 28/09/2020 20:23, Josef Bacik wrote: > To avoid the multiple calls if we're not read only, otherwise it'll be multiple > function calls to check that that SB_RDONLY is set. The compiler will probably > optimize that away, but I just went with this instead. I'm good either way if > people have strong opinions one way or the other. The name check_ro_option() really suggests it checking for a read-only option and having multiple calls if RO isn't set really won't be a problem IMHO as we're not really in the fast-path here, but I don't have a strong opinion on it either.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 8840a4fa81eb..f99e89ec46b2 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -458,6 +458,17 @@ static const match_table_t rescue_tokens = { {Opt_err, NULL}, }; +static bool check_ro_option(struct btrfs_fs_info *fs_info, unsigned long opt, + const char *opt_name) +{ + if (fs_info->mount_opt & opt) { + btrfs_err(fs_info, "%s must be used with ro mount option", + opt_name); + return true; + } + return false; +} + static int parse_rescue_options(struct btrfs_fs_info *info, const char *options) { char *opts; @@ -968,14 +979,12 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, } } check: - /* - * Extra check for current option against current flag - */ - if (btrfs_test_opt(info, NOLOGREPLAY) && !(new_flags & SB_RDONLY)) { - btrfs_err(info, - "nologreplay must be used with ro mount option"); + /* We're read-only, don't have to check. */ + if (new_flags & SB_RDONLY) + goto out; + + if (check_ro_option(info, BTRFS_MOUNT_NOLOGREPLAY, "nologreplay")) ret = -EINVAL; - } out: if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) && !btrfs_test_opt(info, FREE_SPACE_TREE) &&
We're going to be adding more options that require RDONLY, so add a helper to do the check and error out if we don't have RDONLY set. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/super.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)