diff mbox series

[1/5] btrfs: unify the ro checking for mount options

Message ID d307f1d95415232dabfb700e79cda73618aa7d50.1600961206.git.josef@toxicpanda.com
State New
Headers show
Series New rescue mount options | expand

Commit Message

Josef Bacik Sept. 24, 2020, 3:32 p.m. UTC
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(-)

Comments

Qu Wenruo Sept. 25, 2020, 12:36 a.m. UTC | #1
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) &&
>
Johannes Thumshirn Sept. 28, 2020, 12:37 p.m. UTC | #2
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
Josef Bacik Sept. 28, 2020, 6:23 p.m. UTC | #3
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
Johannes Thumshirn Sept. 29, 2020, 6:36 a.m. UTC | #4
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 mbox series

Patch

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) &&