[PATCH] btrfs: manage thread_pool mount option as %u

[PATCH] btrfs: manage thread_pool mount option as %u

[Anand Jain]

-o thread_pool is alway unsigned. Manage it that way all around.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ctree.h   |  2 +-
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/super.c   | 13 ++++++-------
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0b738f0a9a23..23b7e6756b15 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -935,7 +935,7 @@ struct btrfs_fs_info {
 	struct btrfs_workqueue *extent_workers;
 	struct task_struct *transaction_kthread;
 	struct task_struct *cleaner_kthread;
-	int thread_pool_size;
+	u32 thread_pool_size;
 
 	struct kobject *space_info_kobj;
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 87d24c903152..3cec2455b603 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2180,7 +2180,7 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
 static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info,
 		struct btrfs_fs_devices *fs_devices)
 {
-	int max_active = fs_info->thread_pool_size;
+	u32 max_active = fs_info->thread_pool_size;
 	unsigned int flags = WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND;
 
 	fs_info->workers =
@@ -2401,7 +2401,7 @@ int open_ctree(struct super_block *sb,
 	int err = -EINVAL;
 	int num_backups_tried = 0;
 	int backup_index = 0;
-	int max_active;
+	u32 max_active;
 	int clear_free_space_tree = 0;
 
 	tree_root = fs_info->tree_root = btrfs_alloc_root(fs_info, GFP_KERNEL);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 02c7766e6849..8112619cac95 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -346,7 +346,7 @@ static const match_table_t tokens = {
 	{Opt_barrier, "barrier"},
 	{Opt_max_inline, "max_inline=%u"},
 	{Opt_alloc_start, "alloc_start=%s"},
-	{Opt_thread_pool, "thread_pool=%d"},
+	{Opt_thread_pool, "thread_pool=%u"},
 	{Opt_compress, "compress"},
 	{Opt_compress_type, "compress=%s"},
 	{Opt_compress_force, "compress-force"},
@@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			ret = match_int(&args[0], &intarg);
 			if (ret) {
 				goto out;
-			} else if (intarg > 0) {
-				info->thread_pool_size = intarg;
-			} else {
+			} else if (intarg == 0) {
 				ret = -EINVAL;
 				goto out;
 			}
+			info->thread_pool_size = intarg;
 			break;
 		case Opt_max_inline:
 			ret = match_int(&args[0], &intarg);
@@ -1317,7 +1316,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 		seq_printf(seq, ",max_inline=%u", info->max_inline);
 	if (info->thread_pool_size !=  min_t(unsigned long,
 					     num_online_cpus() + 2, 8))
-		seq_printf(seq, ",thread_pool=%d", info->thread_pool_size);
+		seq_printf(seq, ",thread_pool=%u", info->thread_pool_size);
 	if (btrfs_test_opt(info, COMPRESS)) {
 		compress_type = btrfs_compress_type2str(info->compress_type);
 		if (btrfs_test_opt(info, FORCE_COMPRESS))
@@ -1723,7 +1722,7 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
 }
 
 static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info,
-				     int new_pool_size, int old_pool_size)
+				     u32 new_pool_size, u32 old_pool_size)
 {
 	if (new_pool_size == old_pool_size)
 		return;
@@ -1791,7 +1790,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 	unsigned long old_opts = fs_info->mount_opt;
 	unsigned long old_compress_type = fs_info->compress_type;
 	u32 old_max_inline = fs_info->max_inline;
-	int old_thread_pool_size = fs_info->thread_pool_size;
+	u32 old_thread_pool_size = fs_info->thread_pool_size;
 	unsigned int old_metadata_ratio = fs_info->metadata_ratio;
 	int ret;
 
-- 
2.7.0

[PATCH] btrfs: manage subvolid mount option as %u

[Anand Jain]

As -o subvolid mount option is an u64 manage it as %u for
token verifications, instead of %s.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 8112619cac95..bf629c8a6a47 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -336,7 +336,7 @@ enum {
 static const match_table_t tokens = {
 	{Opt_degraded, "degraded"},
 	{Opt_subvol, "subvol=%s"},
-	{Opt_subvolid, "subvolid=%s"},
+	{Opt_subvolid, "subvolid=%u"},
 	{Opt_device, "device=%s"},
 	{Opt_nodatasum, "nodatasum"},
 	{Opt_datasum, "datasum"},
@@ -964,8 +964,8 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *opts, *orig, *p;
-	char *num = NULL;
 	int error = 0;
+	u64 subvolid;
 
 	if (!options)
 		return 0;
@@ -995,18 +995,15 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
 			}
 			break;
 		case Opt_subvolid:
-			num = match_strdup(&args[0]);
-			if (num) {
-				*subvol_objectid = memparse(num, NULL);
-				kfree(num);
-				/* we want the original fs_tree */
-				if (!*subvol_objectid)
-					*subvol_objectid =
-						BTRFS_FS_TREE_OBJECTID;
-			} else {
-				error = -EINVAL;
+			error = match_u64(&args[0], &subvolid);
+			if (error)
 				goto out;
-			}
+
+			/* we want the original fs_tree */
+			if (subvolid == 0)
+				subvolid = BTRFS_FS_TREE_OBJECTID;
+
+			*subvol_objectid = subvolid;
 			break;
 		case Opt_subvolrootid:
 			pr_warn("BTRFS: 'subvolrootid' mount option is deprecated and has no effect\n");
-- 
2.7.0

[PATCH] btrfs: manage metadata_ratio mount option as %u

[Anand Jain]

As -o metadata_ratio mount option is unsinged so manage it as %u for
token verifications, instead of %s.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index bf629c8a6a47..b925a649a01a 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -362,7 +362,7 @@ static const match_table_t tokens = {
 	{Opt_norecovery, "norecovery"},
 	{Opt_flushoncommit, "flushoncommit"},
 	{Opt_noflushoncommit, "noflushoncommit"},
-	{Opt_ratio, "metadata_ratio=%d"},
+	{Opt_ratio, "metadata_ratio=%u"},
 	{Opt_discard, "discard"},
 	{Opt_nodiscard, "nodiscard"},
 	{Opt_space_cache, "space_cache"},
@@ -648,16 +648,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			break;
 		case Opt_ratio:
 			ret = match_int(&args[0], &intarg);
-			if (ret) {
-				goto out;
-			} else if (intarg >= 0) {
-				info->metadata_ratio = intarg;
-				btrfs_info(info, "metadata ratio %d",
-					   info->metadata_ratio);
-			} else {
-				ret = -EINVAL;
+			if (ret)
 				goto out;
-			}
+			info->metadata_ratio = intarg;
+			btrfs_info(info, "metadata ratio %u",
+				   info->metadata_ratio);
 			break;
 		case Opt_discard:
 			btrfs_set_and_info(info, DISCARD,
@@ -1369,7 +1364,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 				info->check_integrity_print_mask);
 #endif
 	if (info->metadata_ratio)
-		seq_printf(seq, ",metadata_ratio=%d",
+		seq_printf(seq, ",metadata_ratio=%u",
 				info->metadata_ratio);
 	if (btrfs_test_opt(info, PANIC_ON_FATAL_ERROR))
 		seq_puts(seq, ",fatal_errors=panic");
-- 
2.7.0

[PATCH] btrfs: manage check_int_print_mask mount option as %u

[Anand Jain]

As -o check_int_print_mask mount option is unsinged so manage it as %u
for token verifications, instead of %d.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b925a649a01a..83e11bbacc17 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -382,7 +382,7 @@ static const match_table_t tokens = {
 	{Opt_skip_balance, "skip_balance"},
 	{Opt_check_integrity, "check_int"},
 	{Opt_check_integrity_including_extent_data, "check_int_data"},
-	{Opt_check_integrity_print_mask, "check_int_print_mask=%d"},
+	{Opt_check_integrity_print_mask, "check_int_print_mask=%u"},
 	{Opt_rescan_uuid_tree, "rescan_uuid_tree"},
 	{Opt_fatal_errors, "fatal_errors=%s"},
 	{Opt_commit_interval, "commit=%d"},
@@ -747,17 +747,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 			break;
 		case Opt_check_integrity_print_mask:
 			ret = match_int(&args[0], &intarg);
-			if (ret) {
-				goto out;
-			} else if (intarg >= 0) {
-				info->check_integrity_print_mask = intarg;
-				btrfs_info(info,
-					   "check_integrity_print_mask 0x%x",
-					   info->check_integrity_print_mask);
-			} else {
-				ret = -EINVAL;
+			if (ret)
 				goto out;
-			}
+			info->check_integrity_print_mask = intarg;
+			btrfs_info(info, "check_integrity_print_mask 0x%x",
+				   info->check_integrity_print_mask);
 			break;
 #else
 		case Opt_check_integrity_including_extent_data:
-- 
2.7.0

[PATCH] btrfs: manage commit mount option as %u

[Anand Jain]

As -o commit mount option is unsinged so manage it as %u
for token verifications, instead of %d.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ctree.h |  2 +-
 fs/btrfs/super.c | 26 ++++++++++----------------
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 23b7e6756b15..80d1d4d12f9d 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -788,7 +788,7 @@ struct btrfs_fs_info {
 	unsigned long pending_changes;
 	unsigned long compress_type:4;
 	unsigned int compress_level;
-	int commit_interval;
+	u32 commit_interval;
 	/*
 	 * It is a suggestive number, the read side is safe even it gets a
 	 * wrong number because we will write out the data into a regular
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 83e11bbacc17..0a299a0ded3a 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -385,7 +385,7 @@ static const match_table_t tokens = {
 	{Opt_check_integrity_print_mask, "check_int_print_mask=%u"},
 	{Opt_rescan_uuid_tree, "rescan_uuid_tree"},
 	{Opt_fatal_errors, "fatal_errors=%s"},
-	{Opt_commit_interval, "commit=%d"},
+	{Opt_commit_interval, "commit=%u"},
 #ifdef CONFIG_BTRFS_DEBUG
 	{Opt_fragment_data, "fragment=data"},
 	{Opt_fragment_metadata, "fragment=metadata"},
@@ -777,24 +777,18 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 		case Opt_commit_interval:
 			intarg = 0;
 			ret = match_int(&args[0], &intarg);
-			if (ret < 0) {
-				btrfs_err(info, "invalid commit interval");
-				ret = -EINVAL;
+			if (ret)
 				goto out;
-			}
-			if (intarg > 0) {
-				if (intarg > 300) {
-					btrfs_warn(info,
-						"excessive commit interval %d",
-						intarg);
-				}
-				info->commit_interval = intarg;
-			} else {
+			if (intarg == 0) {
 				btrfs_info(info,
-					   "using default commit interval %ds",
+					   "using default commit interval %us",
 					   BTRFS_DEFAULT_COMMIT_INTERVAL);
-				info->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
+				intarg = BTRFS_DEFAULT_COMMIT_INTERVAL;
+			} else if (intarg > 300) {
+				btrfs_warn(info, "excessive commit interval %d",
+					   intarg);
 			}
+			info->commit_interval = intarg;
 			break;
 #ifdef CONFIG_BTRFS_DEBUG
 		case Opt_fragment_all:
@@ -1363,7 +1357,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 	if (btrfs_test_opt(info, PANIC_ON_FATAL_ERROR))
 		seq_puts(seq, ",fatal_errors=panic");
 	if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
-		seq_printf(seq, ",commit=%d", info->commit_interval);
+		seq_printf(seq, ",commit=%u", info->commit_interval);
 #ifdef CONFIG_BTRFS_DEBUG
 	if (btrfs_test_opt(info, FRAGMENT_DATA))
 		seq_puts(seq, ",fragment=data");
-- 
2.7.0

[PATCH] btrfs: add a comment to mark the deprecated mount option

[Anand Jain]

-o alloc_start and -o subvolrootid are deprecated mount options,
comment them in the tokens list. And leave them as it is. No
functional changes.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 0a299a0ded3a..7771e690460c 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -345,7 +345,7 @@ static const match_table_t tokens = {
 	{Opt_nobarrier, "nobarrier"},
 	{Opt_barrier, "barrier"},
 	{Opt_max_inline, "max_inline=%u"},
-	{Opt_alloc_start, "alloc_start=%s"},
+	{Opt_alloc_start, "alloc_start=%s"}, /* deprecated */
 	{Opt_thread_pool, "thread_pool=%u"},
 	{Opt_compress, "compress"},
 	{Opt_compress_type, "compress=%s"},
@@ -371,7 +371,7 @@ static const match_table_t tokens = {
 	{Opt_user_subvol_rm_allowed, "user_subvol_rm_allowed"},
 	{Opt_enospc_debug, "enospc_debug"},
 	{Opt_noenospc_debug, "noenospc_debug"},
-	{Opt_subvolrootid, "subvolrootid=%d"},
+	{Opt_subvolrootid, "subvolrootid=%d"}, /* deprecated */
 	{Opt_defrag, "autodefrag"},
 	{Opt_nodefrag, "noautodefrag"},
 	{Opt_inode_cache, "inode_cache"},
-- 
2.7.0

[PATCH] btrfs: fix bare unsigned declarations

[Anand Jain]

Kernel style prefers "unsigned int <foo>" over "unsigned <foo>"
and "signed int <foo>" over "signed <foo>".

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/ctree.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 80d1d4d12f9d..c6de1a5281ca 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -993,8 +993,8 @@ struct btrfs_fs_info {
 	struct btrfs_balance_control *balance_ctl;
 	wait_queue_head_t balance_wait_q;
 
-	unsigned data_chunk_allocations;
-	unsigned metadata_ratio;
+	u32 data_chunk_allocations;
+	u32 metadata_ratio;
 
 	void *bdev_holder;
 
-- 
2.7.0

Re: [PATCH] btrfs: manage thread_pool mount option as %u

[Nikolay Borisov]

On 13.02.2018 11:50, Anand Jain wrote:
> -o thread_pool is alway unsigned. Manage it that way all around.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/ctree.h   |  2 +-
>  fs/btrfs/disk-io.c |  4 ++--
>  fs/btrfs/super.c   | 13 ++++++-------
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 0b738f0a9a23..23b7e6756b15 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -935,7 +935,7 @@ struct btrfs_fs_info {
>  	struct btrfs_workqueue *extent_workers;
>  	struct task_struct *transaction_kthread;
>  	struct task_struct *cleaner_kthread;
> -	int thread_pool_size;
> +	u32 thread_pool_size;
>  
>  	struct kobject *space_info_kobj;
>  
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 87d24c903152..3cec2455b603 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2180,7 +2180,7 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
>  static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info,
>  		struct btrfs_fs_devices *fs_devices)
>  {
> -	int max_active = fs_info->thread_pool_size;
> +	u32 max_active = fs_info->thread_pool_size;
>  	unsigned int flags = WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND;
>  
>  	fs_info->workers =
> @@ -2401,7 +2401,7 @@ int open_ctree(struct super_block *sb,
>  	int err = -EINVAL;
>  	int num_backups_tried = 0;
>  	int backup_index = 0;
> -	int max_active;
> +	u32 max_active;
>  	int clear_free_space_tree = 0;
>  
>  	tree_root = fs_info->tree_root = btrfs_alloc_root(fs_info, GFP_KERNEL);
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 02c7766e6849..8112619cac95 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -346,7 +346,7 @@ static const match_table_t tokens = {
>  	{Opt_barrier, "barrier"},
>  	{Opt_max_inline, "max_inline=%u"},
>  	{Opt_alloc_start, "alloc_start=%s"},
> -	{Opt_thread_pool, "thread_pool=%d"},
> +	{Opt_thread_pool, "thread_pool=%u"},
>  	{Opt_compress, "compress"},
>  	{Opt_compress_type, "compress=%s"},
>  	{Opt_compress_force, "compress-force"},
> @@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  			ret = match_int(&args[0], &intarg);
>  			if (ret) {
>  				goto out;
> -			} else if (intarg > 0) {
> -				info->thread_pool_size = intarg;
> -			} else {
> +			} else if (intarg == 0) {

One thing I'm worried about is the fact that match_int parses a signed
int. So If someone pases -1 then it would be parsed to -1 but when you
set it to thread_pool_size the actual value is going to be the 2's
complement of the value i.e. a very large number. So a check for intarg
< 0 is required to avoid that. Same applies to your other patches

>  				ret = -EINVAL;
>  				goto out;
>  			}
> +			info->thread_pool_size = intarg;
>  			break;
>  		case Opt_max_inline:
>  			ret = match_int(&args[0], &intarg);
> @@ -1317,7 +1316,7 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
>  		seq_printf(seq, ",max_inline=%u", info->max_inline);
>  	if (info->thread_pool_size !=  min_t(unsigned long,
>  					     num_online_cpus() + 2, 8))
> -		seq_printf(seq, ",thread_pool=%d", info->thread_pool_size);
> +		seq_printf(seq, ",thread_pool=%u", info->thread_pool_size);
>  	if (btrfs_test_opt(info, COMPRESS)) {
>  		compress_type = btrfs_compress_type2str(info->compress_type);
>  		if (btrfs_test_opt(info, FORCE_COMPRESS))
> @@ -1723,7 +1722,7 @@ static struct dentry *btrfs_mount(struct file_system_type *fs_type, int flags,
>  }
>  
>  static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info,
> -				     int new_pool_size, int old_pool_size)
> +				     u32 new_pool_size, u32 old_pool_size)
>  {
>  	if (new_pool_size == old_pool_size)
>  		return;
> @@ -1791,7 +1790,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
>  	unsigned long old_opts = fs_info->mount_opt;
>  	unsigned long old_compress_type = fs_info->compress_type;
>  	u32 old_max_inline = fs_info->max_inline;
> -	int old_thread_pool_size = fs_info->thread_pool_size;
> +	u32 old_thread_pool_size = fs_info->thread_pool_size;
>  	unsigned int old_metadata_ratio = fs_info->metadata_ratio;
>  	int ret;
>  
> 

Re: [PATCH] btrfs: manage thread_pool mount option as %u

[Anand Jain]

>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 02c7766e6849..8112619cac95 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -346,7 +346,7 @@ static const match_table_t tokens = {
>>   	{Opt_barrier, "barrier"},
>>   	{Opt_max_inline, "max_inline=%u"},
>>   	{Opt_alloc_start, "alloc_start=%s"},
>> -	{Opt_thread_pool, "thread_pool=%d"},
>> +	{Opt_thread_pool, "thread_pool=%u"},
>>   	{Opt_compress, "compress"},
>>   	{Opt_compress_type, "compress=%s"},
>>   	{Opt_compress_force, "compress-force"},
>> @@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>>   			ret = match_int(&args[0], &intarg);
>>   			if (ret) {
>>   				goto out;
>> -			} else if (intarg > 0) {
>> -				info->thread_pool_size = intarg;
>> -			} else {
>> +			} else if (intarg == 0) {
> 
> One thing I'm worried about is the fact that match_int parses a signed
> int. So If someone pases -1 then it would be parsed to -1 but when you
> set it to thread_pool_size the actual value is going to be the 2's
> complement of the value i.e. a very large number. So a check for intarg
> < 0 is required to avoid that. Same applies to your other patches

  That's not true. When -o thread_pool=-1 is passed it would fail
  to match to any token. And same applies to other patches too.

Thanks, Anand

Re: [PATCH] btrfs: manage thread_pool mount option as %u

[Nikolay Borisov]

On 13.02.2018 17:18, Anand Jain wrote:
> 
> 
> 
> 
> 
> 
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 02c7766e6849..8112619cac95 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -346,7 +346,7 @@ static const match_table_t tokens = {
>>>       {Opt_barrier, "barrier"},
>>>       {Opt_max_inline, "max_inline=%u"},
>>>       {Opt_alloc_start, "alloc_start=%s"},
>>> -    {Opt_thread_pool, "thread_pool=%d"},
>>> +    {Opt_thread_pool, "thread_pool=%u"},
>>>       {Opt_compress, "compress"},
>>>       {Opt_compress_type, "compress=%s"},
>>>       {Opt_compress_force, "compress-force"},
>>> @@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info
>>> *info, char *options,
>>>               ret = match_int(&args[0], &intarg);
>>>               if (ret) {
>>>                   goto out;
>>> -            } else if (intarg > 0) {
>>> -                info->thread_pool_size = intarg;
>>> -            } else {
>>> +            } else if (intarg == 0) {
>>
>> One thing I'm worried about is the fact that match_int parses a signed
>> int. So If someone pases -1 then it would be parsed to -1 but when you
>> set it to thread_pool_size the actual value is going to be the 2's
>> complement of the value i.e. a very large number. So a check for intarg
>> < 0 is required to avoid that. Same applies to your other patches
> 
>  That's not true. When -o thread_pool=-1 is passed it would fail
>  to match to any token. And same applies to other patches too.

Indeed, the subtleties of the matching machinery. In that case for the
whole series:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
> Thanks, Anand
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs: manage thread_pool mount option as %u

[David Sterba]

On Tue, Feb 13, 2018 at 05:34:56PM +0200, Nikolay Borisov wrote:
> >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> >>> index 02c7766e6849..8112619cac95 100644
> >>> --- a/fs/btrfs/super.c
> >>> +++ b/fs/btrfs/super.c
> >>> @@ -346,7 +346,7 @@ static const match_table_t tokens = {
> >>>       {Opt_barrier, "barrier"},
> >>>       {Opt_max_inline, "max_inline=%u"},
> >>>       {Opt_alloc_start, "alloc_start=%s"},
> >>> -    {Opt_thread_pool, "thread_pool=%d"},
> >>> +    {Opt_thread_pool, "thread_pool=%u"},
> >>>       {Opt_compress, "compress"},
> >>>       {Opt_compress_type, "compress=%s"},
> >>>       {Opt_compress_force, "compress-force"},
> >>> @@ -596,12 +596,11 @@ int btrfs_parse_options(struct btrfs_fs_info
> >>> *info, char *options,
> >>>               ret = match_int(&args[0], &intarg);
> >>>               if (ret) {
> >>>                   goto out;
> >>> -            } else if (intarg > 0) {
> >>> -                info->thread_pool_size = intarg;
> >>> -            } else {
> >>> +            } else if (intarg == 0) {
> >>
> >> One thing I'm worried about is the fact that match_int parses a signed
> >> int. So If someone pases -1 then it would be parsed to -1 but when you
> >> set it to thread_pool_size the actual value is going to be the 2's
> >> complement of the value i.e. a very large number. So a check for intarg
> >> < 0 is required to avoid that. Same applies to your other patches
> > 
> >  That's not true. When -o thread_pool=-1 is passed it would fail
> >  to match to any token. And same applies to other patches too.
> 
> Indeed, the subtleties of the matching machinery. In that case for the
> whole series:

Yeah, it's not very clear from the sources, but with %u the parser
refuses negative numbers. Even with match_int, the typecasts will not
damage the result when assigning to u32 from intarg.

Re: [PATCH] btrfs: manage subvolid mount option as %u

[David Sterba]

On Tue, Feb 13, 2018 at 05:50:43PM +0800, Anand Jain wrote:
> As -o subvolid mount option is an u64 manage it as %u for
> token verifications, instead of %s.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/super.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 8112619cac95..bf629c8a6a47 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -336,7 +336,7 @@ enum {
>  static const match_table_t tokens = {
>  	{Opt_degraded, "degraded"},
>  	{Opt_subvol, "subvol=%s"},
> -	{Opt_subvolid, "subvolid=%s"},
> +	{Opt_subvolid, "subvolid=%u"},

This gets implemented as simple_strtoul, deep in the perser it uses an
unsigned long long and then casts to usigned long. Long and long-long
are same only on subset of architectures, so we'd need to either extend
teh parser capabilities to really do u64 or we'd have to use the %s and
match_u64.

Though a subvolid larger than full 32bit number is unlikely, for sake of
correctness I think we should stick to the constraints of the subvolid
that allows u64.

>  	{Opt_device, "device=%s"},
>  	{Opt_nodatasum, "nodatasum"},
>  	{Opt_datasum, "datasum"},
> @@ -964,8 +964,8 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
>  {
>  	substring_t args[MAX_OPT_ARGS];
>  	char *opts, *orig, *p;
> -	char *num = NULL;
>  	int error = 0;
> +	u64 subvolid;
>  
>  	if (!options)
>  		return 0;
> @@ -995,18 +995,15 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
>  			}
>  			break;
>  		case Opt_subvolid:
> -			num = match_strdup(&args[0]);
> -			if (num) {
> -				*subvol_objectid = memparse(num, NULL);
> -				kfree(num);
> -				/* we want the original fs_tree */
> -				if (!*subvol_objectid)
> -					*subvol_objectid =
> -						BTRFS_FS_TREE_OBJECTID;
> -			} else {
> -				error = -EINVAL;
> +			error = match_u64(&args[0], &subvolid);

So this is the right way (with the %s), as memparse accepts the size
suffixes (K/M/G/...), that we don't want for a subvolume id.

> +			if (error)
>  				goto out;
> -			}
> +
> +			/* we want the original fs_tree */
> +			if (subvolid == 0)
> +				subvolid = BTRFS_FS_TREE_OBJECTID;
> +
> +			*subvol_objectid = subvolid;
>  			break;
>  		case Opt_subvolrootid:
>  			pr_warn("BTRFS: 'subvolrootid' mount option is deprecated and has no effect\n");
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs: fix bare unsigned declarations

[David Sterba]

On Tue, Feb 13, 2018 at 05:50:48PM +0800, Anand Jain wrote:
> Kernel style prefers "unsigned int <foo>" over "unsigned <foo>"
> and "signed int <foo>" over "signed <foo>".

The changelog does not match the changes, you're switching to u32. I
agree u32 looks more suitable and consistent with the other changes.
Please update the changelog and fixup the temporary variable in
btrfs_remount. Thanks.

[PATCH v2] btrfs: verify subvolid mount parameter

[Anand Jain]

We aren't verifying the parameter passed to the subvolid mount option,
so we won't report and fail the mount if a junk value is specified for
example, -o subvolid=abc.
This patch verifies the subvolid option with match_u64.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2:
 Title changed. Old:
   btrfs: manage subvolid mount option with match_u64
 Change log updated.
 keep the %s as token verification.

 fs/btrfs/super.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 10f7d0fbd50f..0333b8802a02 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -976,8 +976,8 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
 {
 	substring_t args[MAX_OPT_ARGS];
 	char *opts, *orig, *p;
-	char *num = NULL;
 	int error = 0;
+	u64 subvolid;
 
 	if (!options)
 		return 0;
@@ -1007,18 +1007,15 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
 			}
 			break;
 		case Opt_subvolid:
-			num = match_strdup(&args[0]);
-			if (num) {
-				*subvol_objectid = memparse(num, NULL);
-				kfree(num);
-				/* we want the original fs_tree */
-				if (!*subvol_objectid)
-					*subvol_objectid =
-						BTRFS_FS_TREE_OBJECTID;
-			} else {
-				error = -EINVAL;
+			error = match_u64(&args[0], &subvolid);
+			if (error)
 				goto out;
-			}
+
+			/* we want the original fs_tree */
+			if (subvolid == 0)
+				subvolid = BTRFS_FS_TREE_OBJECTID;
+
+			*subvol_objectid = subvolid;
 			break;
 		case Opt_subvolrootid:
 			pr_warn("BTRFS: 'subvolrootid' mount option is deprecated and has no effect\n");
-- 
2.15.0

[PATCH v2] btrfs: fix bare unsigned declarations

[Anand Jain]

We have btrfs_fs_info::data_chunk_allocations and
btrfs_fs_info::metadata_ratio declared as unsigned which would
be unsinged int and kernel style prefers unsigned int over bare
unsigned. So this patch changes them to u32.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v1->v2: Update change log.

 fs/btrfs/ctree.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 8620eea57d6a..32d7b55061d3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -994,8 +994,8 @@ struct btrfs_fs_info {
 	struct btrfs_balance_control *balance_ctl;
 	wait_queue_head_t balance_wait_q;
 
-	unsigned data_chunk_allocations;
-	unsigned metadata_ratio;
+	u32 data_chunk_allocations;
+	u32 metadata_ratio;
 
 	void *bdev_holder;
 
-- 
2.15.0

Re: [PATCH] btrfs: manage subvolid mount option as %u

[Anand Jain]

On 02/14/2018 12:10 AM, David Sterba wrote:
> On Tue, Feb 13, 2018 at 05:50:43PM +0800, Anand Jain wrote:
>> As -o subvolid mount option is an u64 manage it as %u for
>> token verifications, instead of %s.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/super.c | 23 ++++++++++-------------
>>   1 file changed, 10 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 8112619cac95..bf629c8a6a47 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -336,7 +336,7 @@ enum {
>>   static const match_table_t tokens = {
>>   	{Opt_degraded, "degraded"},
>>   	{Opt_subvol, "subvol=%s"},
>> -	{Opt_subvolid, "subvolid=%s"},
>> +	{Opt_subvolid, "subvolid=%u"},
> 
> This gets implemented as simple_strtoul, deep in the perser it uses an
> unsigned long long and then casts to usigned long. Long and long-long
> are same only on subset of architectures, so we'd need to either extend
> teh parser capabilities to really do u64 or we'd have to use the %s and
> match_u64.

  For now I would use %s and match_u64.

> Though a subvolid larger than full 32bit number is unlikely, for sake of
> correctness I think we should stick to the constraints of the subvolid
> that allows u64.

  Agree.

>>   	{Opt_device, "device=%s"},
>>   	{Opt_nodatasum, "nodatasum"},
>>   	{Opt_datasum, "datasum"},
>> @@ -964,8 +964,8 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
>>   {
>>   	substring_t args[MAX_OPT_ARGS];
>>   	char *opts, *orig, *p;
>> -	char *num = NULL;
>>   	int error = 0;
>> +	u64 subvolid;
>>   
>>   	if (!options)
>>   		return 0;
>> @@ -995,18 +995,15 @@ static int btrfs_parse_subvol_options(const char *options, fmode_t flags,
>>   			}
>>   			break;
>>   		case Opt_subvolid:
>> -			num = match_strdup(&args[0]);
>> -			if (num) {
>> -				*subvol_objectid = memparse(num, NULL);
>> -				kfree(num);
>> -				/* we want the original fs_tree */
>> -				if (!*subvol_objectid)
>> -					*subvol_objectid =
>> -						BTRFS_FS_TREE_OBJECTID;
>> -			} else {
>> -				error = -EINVAL;
>> +			error = match_u64(&args[0], &subvolid);
> 
> So this is the right way (with the %s), as memparse accepts the size
> suffixes (K/M/G/...), that we don't want for a subvolume id.

  Fixed in V2. Pls find.

Thanks, Anand


>> +			if (error)
>>   				goto out;
>> -			}
>> +
>> +			/* we want the original fs_tree */
>> +			if (subvolid == 0)
>> +				subvolid = BTRFS_FS_TREE_OBJECTID;
>> +
>> +			*subvol_objectid = subvolid;
>>   			break;
>>   		case Opt_subvolrootid:
>>   			pr_warn("BTRFS: 'subvolrootid' mount option is deprecated and has no effect\n");
>> -- 
>> 2.7.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] btrfs: verify subvolid mount parameter

[David Sterba]

On Thu, Feb 15, 2018 at 01:11:37AM +0800, Anand Jain wrote:
> We aren't verifying the parameter passed to the subvolid mount option,
> so we won't report and fail the mount if a junk value is specified for
> example, -o subvolid=abc.
> This patch verifies the subvolid option with match_u64.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Changelog updated to mention the previous memparse effects.

Re: [PATCH v2] btrfs: fix bare unsigned declarations

[David Sterba]

On Thu, Feb 15, 2018 at 01:13:00AM +0800, Anand Jain wrote:
> We have btrfs_fs_info::data_chunk_allocations and
> btrfs_fs_info::metadata_ratio declared as unsigned which would
> be unsinged int and kernel style prefers unsigned int over bare
> unsigned. So this patch changes them to u32.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Please also update the temporary variable old_metadata_ratio in
btrfs_remount that get assigned from one of the variables.

[PATCH v3] btrfs: fix bare unsigned declarations

[Anand Jain]

We have btrfs_fs_info::data_chunk_allocations and
btrfs_fs_info::metadata_ratio declared as unsigned which would
be unsinged int and kernel style prefers unsigned int over bare
unsigned. So this patch changes them to u32.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v2->v3: @old_metadata_ratio change it to u32
v1->v2: Update change log.

 fs/btrfs/ctree.h | 4 ++--
 fs/btrfs/super.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0b67ee4dcb6e..32ea4cb839c4 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -994,8 +994,8 @@ struct btrfs_fs_info {
 	struct btrfs_balance_control *balance_ctl;
 	wait_queue_head_t balance_wait_q;
 
-	unsigned data_chunk_allocations;
-	unsigned metadata_ratio;
+	u32 data_chunk_allocations;
+	u32 metadata_ratio;
 
 	void *bdev_holder;
 
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index f92f2e1ec00b..8f78fb2b8204 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1737,7 +1737,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data)
 	unsigned long old_compress_type = fs_info->compress_type;
 	u32 old_max_inline = fs_info->max_inline;
 	u32 old_thread_pool_size = fs_info->thread_pool_size;
-	unsigned int old_metadata_ratio = fs_info->metadata_ratio;
+	u32 old_metadata_ratio = fs_info->metadata_ratio;
 	int ret;
 
 	sync_filesystem(sb);
-- 
2.7.0

Re: [PATCH v3] btrfs: fix bare unsigned declarations

[David Sterba]

On Mon, Feb 26, 2018 at 04:46:05PM +0800, Anand Jain wrote:
> We have btrfs_fs_info::data_chunk_allocations and
> btrfs_fs_info::metadata_ratio declared as unsigned which would
> be unsinged int and kernel style prefers unsigned int over bare
> unsigned. So this patch changes them to u32.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2->v3: @old_metadata_ratio change it to u32
> v1->v2: Update change log.

Added to next, thanks.