[PATCH] add --data_types for bcachefs device add

[PATCH] add --data_types for bcachefs device add

[Janpieter Sollie]

From: Janpieter Sollie <janpieter.sollie@edpnet.be>

bcachefs format allows to specify data types allowed on a specific device
but this currently is not available when adding a device with bcachefs format.
The patch adds this functionality.  It is kept as minimal as possible,
in the future the device specific OPTS from cmd_format should be imported
instead of specifying them a 2nd time here

Signed-off-by: Janpieter Sollie <janpieter.sollie@kabelmail.de>
---
 cmd_device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/cmd_device.c b/cmd_device.c
index c59d370..2da9b87 100644
--- a/cmd_device.c
+++ b/cmd_device.c
@@ -51,6 +51,7 @@ static void device_add_usage(void)
 	     "  -B, --bucket=size           Bucket size\n"
 	     "  -D, --discard               Enable discards\n"
 	     "  -l, --label=label           Disk label\n"
+	     "  -a, --data_allowed=types    allowed data on this device\n"
 	     "  -f, --force                 Use device even if it appears to already be formatted\n"
 	     "  -h, --help                  Display this help and exit\n"
 	     "\n"
@@ -64,6 +65,7 @@ int cmd_device_add(int argc, char *argv[])
 		{ "bucket",		required_argument,	NULL, 'B' },
 		{ "discard",		no_argument,		NULL, 'D' },
 		{ "label",		required_argument,	NULL, 'l' },
+		{ "data_allowed",	required_argument,	NULL, 'a' },
 		{ "force",		no_argument,		NULL, 'f' },
 		{ "help",		no_argument,		NULL, 'h' },
 		{ NULL }
@@ -90,6 +92,10 @@ int cmd_device_add(int argc, char *argv[])
 		case 'l':
 			dev_opts.label = strdup(optarg);
 			break;
+                case 'a':
+                        dev_opts.data_allowed = bch2_read_flag_list(optarg, bch2_data_types);
+                        if(dev_opts.data_allowed == (u64) -1) die("invalid data_allowed types for device - aborting");
+                        break;
 		case 'f':
 			force = true;
 			break;
-- 
2.39.1

Re: [PATCH] add --data_types for bcachefs device add

[Kent Overstreet]

On Wed, Apr 26, 2023 at 09:28:58AM +0200, Janpieter Sollie wrote:
> From: Janpieter Sollie <janpieter.sollie@edpnet.be>
> 
> bcachefs format allows to specify data types allowed on a specific device
> but this currently is not available when adding a device with bcachefs format.
> The patch adds this functionality.  It is kept as minimal as possible,
> in the future the device specific OPTS from cmd_format should be imported
> instead of specifying them a 2nd time here
> 
> Signed-off-by: Janpieter Sollie <janpieter.sollie@kabelmail.de>
> ---
>  cmd_device.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/cmd_device.c b/cmd_device.c
> index c59d370..2da9b87 100644
> --- a/cmd_device.c
> +++ b/cmd_device.c
> @@ -51,6 +51,7 @@ static void device_add_usage(void)
>  	     "  -B, --bucket=size           Bucket size\n"
>  	     "  -D, --discard               Enable discards\n"
>  	     "  -l, --label=label           Disk label\n"
> +	     "  -a, --data_allowed=types    allowed data on this device\n"
>  	     "  -f, --force                 Use device even if it appears to already be formatted\n"
>  	     "  -h, --help                  Display this help and exit\n"
>  	     "\n"
> @@ -64,6 +65,7 @@ int cmd_device_add(int argc, char *argv[])
>  		{ "bucket",		required_argument,	NULL, 'B' },
>  		{ "discard",		no_argument,		NULL, 'D' },
>  		{ "label",		required_argument,	NULL, 'l' },
> +		{ "data_allowed",	required_argument,	NULL, 'a' },
>  		{ "force",		no_argument,		NULL, 'f' },
>  		{ "help",		no_argument,		NULL, 'h' },
>  		{ NULL }
> @@ -90,6 +92,10 @@ int cmd_device_add(int argc, char *argv[])
>  		case 'l':
>  			dev_opts.label = strdup(optarg);
>  			break;

Wtf!?!?!?

> +                case 'a':
> +                        dev_opts.data_allowed = bch2_read_flag_list(optarg, bch2_data_types);
> +                        if(dev_opts.data_allowed == (u64) -1) die("invalid data_allowed types for device - aborting");
> +                        break;
>  		case 'f':
>  			force = true;
>  			break;
> -- 
> 2.39.1
> 

Re: [PATCH] add --data_types for bcachefs device add

[Janpieter Sollie]

Op 26/04/2023 om 16:42 schreef Kent Overstreet:
> On Wed, Apr 26, 2023 at 09:28:58AM +0200, Janpieter Sollie wrote:
>> From: Janpieter Sollie <janpieter.sollie@edpnet.be>
>>
>> bcachefs format allows to specify data types allowed on a specific device
>> but this currently is not available when adding a device with bcachefs format.
>> The patch adds this functionality.  It is kept as minimal as possible,
>> in the future the device specific OPTS from cmd_format should be imported
>> instead of specifying them a 2nd time here
>>
>> Signed-off-by: Janpieter Sollie <janpieter.sollie@kabelmail.de>
>> ---
>>   cmd_device.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/cmd_device.c b/cmd_device.c
>> index c59d370..2da9b87 100644
>> --- a/cmd_device.c
>> +++ b/cmd_device.c
>> @@ -51,6 +51,7 @@ static void device_add_usage(void)
>>   	     "  -B, --bucket=size           Bucket size\n"
>>   	     "  -D, --discard               Enable discards\n"
>>   	     "  -l, --label=label           Disk label\n"
>> +	     "  -a, --data_allowed=types    allowed data on this device\n"
>>   	     "  -f, --force                 Use device even if it appears to already be formatted\n"
>>   	     "  -h, --help                  Display this help and exit\n"
>>   	     "\n"
>> @@ -64,6 +65,7 @@ int cmd_device_add(int argc, char *argv[])
>>   		{ "bucket",		required_argument,	NULL, 'B' },
>>   		{ "discard",		no_argument,		NULL, 'D' },
>>   		{ "label",		required_argument,	NULL, 'l' },
>> +		{ "data_allowed",	required_argument,	NULL, 'a' },
>>   		{ "force",		no_argument,		NULL, 'f' },
>>   		{ "help",		no_argument,		NULL, 'h' },
>>   		{ NULL }
>> @@ -90,6 +92,10 @@ int cmd_device_add(int argc, char *argv[])
>>   		case 'l':
>>   			dev_opts.label = strdup(optarg);
>>   			break;
> Wtf!?!?!?
>
>> +                case 'a':
>> +                        dev_opts.data_allowed = bch2_read_flag_list(optarg, bch2_data_types);
>> +                        if(dev_opts.data_allowed == (u64) -1) die("invalid data_allowed types for device - aborting");
>> +                        break;
>>   		case 'f':
>>   			force = true;
>>   			break;
>> -- 
>> 2.39.1
>>

I know you said it would be better to move the read_flag_list_or_die() function to a shared library,
but as I said, this is a minimal version,
when I figure out a proper way to share OPTS,
it will revert to using  read_flag_list_or_die().
For now, I thought not moving too much is the best thing while analyzing the situation for a 
clean solution

[PATCH] add --data_types for bcachefs device add

[Janpieter Sollie]

From: Janpieter Sollie <janpieter.sollie@edpnet.be>

bcachefs format allows to specify data types allowed on a specific device,
this is not available when adding a device with bcachefs device add.
The patch adds this functionality.  It is kept as minimal as possible,
the device specific OPTS from cmd_format should be imported
instead of specifying them a 2nd time here

Signed-off-by: Janpieter Sollie <janpieter.sollie@kabelmail.de>
---
 cmd_device.c  |  7 +++++++
 cmd_format.c  | 10 ----------
 libbcachefs.h | 10 ++++++++++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/cmd_device.c b/cmd_device.c
index c59d370..4a296a1 100644
--- a/cmd_device.c
+++ b/cmd_device.c
@@ -51,6 +51,7 @@ static void device_add_usage(void)
 	     "  -B, --bucket=size           Bucket size\n"
 	     "  -D, --discard               Enable discards\n"
 	     "  -l, --label=label           Disk label\n"
+	     "  -a, --data_allowed=types    allowed data on this device\n"
 	     "  -f, --force                 Use device even if it appears to already be formatted\n"
 	     "  -h, --help                  Display this help and exit\n"
 	     "\n"
@@ -64,6 +65,7 @@ int cmd_device_add(int argc, char *argv[])
 		{ "bucket",		required_argument,	NULL, 'B' },
 		{ "discard",		no_argument,		NULL, 'D' },
 		{ "label",		required_argument,	NULL, 'l' },
+		{ "data_allowed",	required_argument,	NULL, 'a' },
 		{ "force",		no_argument,		NULL, 'f' },
 		{ "help",		no_argument,		NULL, 'h' },
 		{ NULL }
@@ -90,6 +92,11 @@ int cmd_device_add(int argc, char *argv[])
 		case 'l':
 			dev_opts.label = strdup(optarg);
 			break;
+                case 'a':
+                        dev_opts.data_allowed =
+                                read_flag_list_or_die(optarg,
+                                        bch2_data_types, "data type");
+                        break;
 		case 'f':
 			force = true;
 			break;
diff --git a/cmd_format.c b/cmd_format.c
index 26a1cd9..3401570 100644
--- a/cmd_format.c
+++ b/cmd_format.c
@@ -102,16 +102,6 @@ static const struct option format_opts[] = {
 };
 #undef x
 
-u64 read_flag_list_or_die(char *opt, const char * const list[],
-			  const char *msg)
-{
-	u64 v = bch2_read_flag_list(opt, list);
-	if (v == (u64) -1)
-		die("Bad %s %s", msg, opt);
-
-	return v;
-}
-
 int cmd_format(int argc, char *argv[])
 {
 	DARRAY(struct dev_opts) devices = { 0 };
diff --git a/libbcachefs.h b/libbcachefs.h
index 4bb51bd..de92f82 100644
--- a/libbcachefs.h
+++ b/libbcachefs.h
@@ -98,6 +98,16 @@ struct bchfs_handle bcache_fs_open(const char *);
 struct bchfs_handle bchu_fs_open_by_dev(const char *, int *);
 int bchu_dev_path_to_idx(struct bchfs_handle, const char *);
 
+static inline u64 read_flag_list_or_die(char *opt, const char * const list[],
+                          const char *msg)
+{
+        u64 v = bch2_read_flag_list(opt, list);
+        if (v == (u64) -1)
+                die("Bad %s %s", msg, opt);
+
+        return v;
+}
+
 static inline void bchu_disk_add(struct bchfs_handle fs, char *dev)
 {
 	struct bch_ioctl_disk i = { .dev = (unsigned long) dev, };
-- 
2.39.1

Re: [PATCH] add --data_types for bcachefs device add

[Kent Overstreet]

On Tue, Apr 25, 2023 at 11:08:59AM +0200, Janpieter Sollie wrote:
> Hi everyone,
> 
> bcachefs format allows to specify data types allowed on a specific device
> (eg: to enforce caching on cheap SSDs while btree info is kept for SSDs with higher TBW),
> but this currently is not available when adding a device with bcachefs format.
> 
> The patch below adds this functionality.  It is kept as minimal as possible,

git am said this was corrupt - were you using git send-email? If that's
not an option, just include a git request-pull style link to your git
repo.

> but in the future maybe the OPTS from bcachefs format can be imported
> instead of the specific opts of device add

Yeah that's the right thing to do.

> 
> Kind regards,
> 
> Janpieter Sollie
> 
> diff --git a/cmd_device.c b/cmd_device.c
> index c59d370..1e8137b 100644
> --- a/cmd_device.c
> +++ b/cmd_device.c
> @@ -41,6 +41,16 @@int device_usage(void)
>        return 0;
> }
> 
> +static u64 read_flag_list_or_die(char *opt, const char * const list[],
> +                          const char *msg)
> +{
> +        u64 v = bch2_read_flag_list(opt, list);
> +        if (v == (u64) -1)
> +                die("Bad %s %s", msg, opt);
> +
> +        return v;
> +}

Don't cut and paste, move the existing version of this to the
appropriate utility/lib file.

Rest of the patch is fine.

[PATCH] add --data_types for bcachefs device add

[Janpieter Sollie]

Hi everyone,

bcachefs format allows to specify data types allowed on a specific device
(eg: to enforce caching on cheap SSDs while btree info is kept for SSDs with higher TBW),
but this currently is not available when adding a device with bcachefs format.

The patch below adds this functionality.  It is kept as minimal as possible,
but in the future maybe the OPTS from bcachefs format can be imported
instead of the specific opts of device add

Kind regards,

Janpieter Sollie

diff --git a/cmd_device.c b/cmd_device.c
index c59d370..1e8137b 100644
--- a/cmd_device.c
+++ b/cmd_device.c
@@ -41,6 +41,16 @@int device_usage(void)
        return 0;
}

+static u64 read_flag_list_or_die(char *opt, const char * const list[],
+                          const char *msg)
+{
+        u64 v = bch2_read_flag_list(opt, list);
+        if (v == (u64) -1)
+                die("Bad %s %s", msg, opt);
+
+        return v;
+}
+
static void device_add_usage(void)
{
        puts("bcachefs device add - add a device to an existing filesystem\n"
@@ -51,6 +61,7 @@static void device_add_usage(void)
             "  -B, --bucket=size           Bucket size\n"
             "  -D, --discard               Enable discards\n"
             "  -l, --label=label           Disk label\n"
+     "  -a, --data_allowed=types    allowed data on this device\n"
             "  -f, --force                 Use device even if it appears to already be 
formatted\n"
             "  -h, --help                  Display this help and exit\n"
             "\n"
@@ -64,6 +75,7 @@int cmd_device_add(int argc, char *argv[])
                { "bucket",             required_argument,      NULL, 'B' },
                { "discard",            no_argument,            NULL, 'D' },
                { "label",              required_argument,      NULL, 'l' },
+{ "data_allowed",       required_argument,      NULL, 'a' },
                { "force",              no_argument,            NULL, 'f' },
                { "help",               no_argument,            NULL, 'h' },
                { NULL }
@@ -90,6 +102,11 @@int cmd_device_add(int argc, char *argv[])
                case 'l':
                        dev_opts.label = strdup(optarg);
                        break;
+                case 'a':
+                        dev_opts.data_allowed =
+                                read_flag_list_or_die(optarg,
+                                        bch2_data_types, "data type");
+                        break;
                case 'f':
                        force = true;
                        break;