btrfs-progs: mkfs: Fix different mixed type by argument sequence

Given a 200G vdd1 and 1G vdd2:

In current code:
mkfs.btrfs -f /dev/vdd1 /dev/vdd2
and
mkfs.btrfs -f /dev/vdd2 /dev/vdd1
will create different "mixed" type.

See [PATCH 2/3] for detail.

This patchset also include some small fixs.

Zhao Lei (3):
btrfs-progs: mkfs: Remove saved_optind in mkfs.btrfs
btrfs-progs: mkfs: Fix different mixed type by argument sequence
btrfs-progs: mkfs: Fix inaccurate mixed information

 mkfs.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

1.8.5.1

No need to use complex logic for iter devs in mkfs.c, as backup optind, increase/decrease optind and reset dev_cnt.

A simple for() loop is enough for above request.

---
 mkfs.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

index 7fa7cfc..cdae94d 100644
@@ -1354,7 +1354,6 @@ int main(int ac, char **av)
 	u64 size_of_data = 0;
 	u64 source_dir_size = 0;
 	int dev_cnt = 0;
-	int saved_optind;
 	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = { 0 };
 	u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
 	struct mkfs_allocation allocation = { 0 };

@@ -1467,7 +1466,6 @@ int main(int ac, char **av)
 	}
 
 	sectorsize = max(sectorsize, (u32)sysconf(_SC_PAGESIZE));
-	saved_optind = optind;
 	dev_cnt = ac - optind;
 	if (dev_cnt == 0)
 		print_usage(1);

@@ -1490,18 +1488,15 @@ int main(int ac, char **av)
 			exit(1);
 		}
 	}
-	
-	while (dev_cnt-- > 0) {
-		file = av[optind++];
+
+	for (i = optind; i < optind + dev_cnt; i++) {
+		file = av[i];
 		if (is_block_device(file) == 1)
 			if (test_dev_for_mkfs(file, force_overwrite))
 				exit(1);
 	}
 
-	optind = saved_optind;
-	dev_cnt = ac - optind;
-
-	file = av[optind++];
+	file = av[optind];
 	ssd = is_ssd(file);
 
 	if (is_vol_small(file) || mixed) {

@@ -1557,7 +1552,7 @@ int main(int ac, char **av)
 			btrfs_min_dev_size(nodesize));
 		exit(1);
 	}
-	for (i = saved_optind; i < saved_optind + dev_cnt; i++) {
+	for (i = optind; i < optind + dev_cnt; i++) {
 		char *path;
 
 		path = av[i];

@@ -1588,8 +1583,6 @@ int main(int ac, char **av)
 		printf("See %s for more information.\n\n", PACKAGE_URL);
 	}
 
-	dev_cnt--;
-
 	if (!source_dir_set) {
 		/*
 		 * open without O_EXCL so that the problem should not

@@ -1720,13 +1713,10 @@ int main(int ac, char **av)
 	if (is_block_device(file) == 1)
 		btrfs_register_one_device(file);
 
-	if (dev_cnt == 0)
-		goto raid_groups;
-
-	while (dev_cnt-- > 0) {
+	for (i = optind + 1; i < optind + dev_cnt; i++) {
 		int old_mixed = mixed;
 
-		file = av[optind++];
+		file = av[i];
 
 		/*
 		 * open without O_EXCL so that the problem should not

@@ -1771,7 +1761,6 @@ int main(int ac, char **av)
 			btrfs_register_one_device(file);
 	}
 
-raid_groups:
 	if (!source_dir_set) {
 		ret = create_raid_groups(trans, root, data_profile,
 				 metadata_profile, mixed, &allocation);

1.8.5.1

Given a 200G vdd1 and 1G vdd2:

In current code:

  # mkfs.btrfs -f /dev/vdd1 /dev/vdd2
  SMALL VOLUME: forcing mixed metadata/data groups
  btrfs-progs v4.1.2
  See http://btrfs.wiki.kernel.org for more information.

  Label:              (null)
  UUID:               7aa6fc75-ce23-4033-9d47-fd046afa2992
  Node size:          4096
  Sector size:        4096
  Filesystem size:    1.20GiB
  Block group profiles:
    Data+Metadata:    single            8.00MiB
    System:           single            4.00MiB
  SSD detected:       no
  Incompat features:  mixed-bg, extref, skinny-metadata
  Number of devices:  2
  Devices:
     ID        SIZE  PATH
      1   200.29MiB  /dev/vdd1
      2     1.00GiB  /dev/vdd2
  #
  # mkfs.btrfs -f /dev/vdd2 /dev/vdd1
  btrfs-progs v4.1.2
  See http://btrfs.wiki.kernel.org for more information.

  Label:              (null)
  UUID:               ac659809-66c1-427d-934d-bd4c209c91a8
  Node size:          16384
  Sector size:        4096
  Filesystem size:    1.20GiB
  Block group profiles:
    Data:             RAID0           136.00MiB
    Metadata:         RAID1            69.38MiB
    System:           RAID1            12.00MiB
  SSD detected:       no
  Incompat features:  extref, skinny-metadata
  Number of devices:  2
  Devices:
     ID        SIZE  PATH
      1     1.00GiB  /dev/vdd2
      2   200.29MiB  /dev/vdd1

We can see:
mkfs.btrfs -f /dev/vdd1 /dev/vdd2
and
mkfs.btrfs -f /dev/vdd2 /dev/vdd1
have different "mixed" type.

Reason:
Current code determine "is to use mixed-type" only by first device.

Fix:
Use mixed-type only if all device are small.

---
 mkfs.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

index cdae94d..29cab13 100644
@@ -1358,6 +1358,7 @@ int main(int ac, char **av)
 	u64 features = BTRFS_MKFS_DEFAULT_FEATURES;
 	struct mkfs_allocation allocation = { 0 };
 	struct btrfs_mkfs_config mkfs_cfg;
+	int large_device_cnt = 0;
 
 	while(1) {
 		int c;

@@ -1494,17 +1495,25 @@ int main(int ac, char **av)
 		if (is_block_device(file) == 1)
 			if (test_dev_for_mkfs(file, force_overwrite))
 				exit(1);
+		ret = is_vol_small(file);
+		if (ret < 0) {
+			error("Failed to check size for '%s': %s",
+			      file, strerror(-ret));
+			exit(1);
+		}
+		large_device_cnt += (!ret);
+		ret = 0;
 	}
 
-	file = av[optind];
-	ssd = is_ssd(file);
-
-	if (is_vol_small(file) || mixed) {
+	if (!large_device_cnt || mixed) {
 		if (verbose)
 			printf("SMALL VOLUME: forcing mixed metadata/data groups\n");
 		mixed = 1;
 	}
 
+	file = av[optind];
+	ssd = is_ssd(file);
+
 	/*
 	* Set default profiles according to number of added devices.
 	* For mixed groups defaults are single/single.

1.8.5.1

In current code, with a "BIG VOLUME" /dev/vdd2:

 # ./mkfs.btrfs -f -M /dev/vdd2
 SMALL VOLUME: forcing mixed metadata/data groups
 ...

This patch changed above output to:
Using mixed metadata/data groups

And the "SMALL VOLUME" output only when we exactly using SMALL VOLUME.

---
 mkfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index 29cab13..0064a78 100644
@@ -1505,7 +1505,10 @@ int main(int ac, char **av)
 		ret = 0;
 	}
 
-	if (!large_device_cnt || mixed) {
+	if (mixed) {
+		if (verbose)
+			printf("Using mixed metadata/data groups\n");
+	} else if (!large_device_cnt) {
 		if (verbose)
 			printf("SMALL VOLUME: forcing mixed metadata/data groups\n");
 		mixed = 1;

1.8.5.1

I think combining large and small devices was not intended use for the mixed-bg, nevertheless current behaviour is not right.

Chandan is working on dropping the forced mixed-bg completely. We've discussed that on IRC, I'm ok with it but this needs more testing. So far it looks fine, small filesystems get created and usable, though some tuning might be needed.

My intentions for 4.3 is to take Chandan's work provided that we test it enough. There are like 3 weeks left. In case of problems, I'll take this patchset so at least we get the inconsisten behaviour fixed.

Hi, David Sterba

Thanks for explanation.

If 4.3 released with dropping mixed-bg, the PATCH 1/3 maybe still necessary, I'll rebase after 4.3.

And another problem:
This time the vdh1 is changed to 100M:

[root@kerneldev progs]# ./mkfs.btrfs -f /dev/vdh1 /dev/vdh2 btrfs-progs v4.2.2-25-gc6b29b6-dirty
See http://btrfs.wiki.kernel.org for more information.

Label:              (null)
UUID:               4465aed7-d33c-4b76-9723-a76df74547fb
Node size:          16384
Sector size:        4096
Filesystem size:    1.20GiB
Block group profiles:
  Data+Metadata:    RAID0            72.00MiB
  System:           RAID1            12.00MiB
SSD detected:       no
Incompat features:  mixed-bg, extref, skinny-metadata
Number of devices:  2
Devices:
   ID        SIZE  PATH
    1   120.55MiB  /dev/vdh1
    2     1.08GiB  /dev/vdh2

[root@kerneldev progs]# ./mkfs.btrfs -f /dev/vdh2 /dev/vdh1 btrfs-progs v4.2.2-25-gc6b29b6-dirty
See http://btrfs.wiki.kernel.org for more information.

not enough free space
[root@kerneldev progs]#

Can be fixed by checking raid support only based on "large disk count".

This fix maybe conflict with Chandan's work, so I'll begin work after he finished.

Thanks
Zhaolei

Yeah, 100M reaches the current default settings of the blockgroups, we'd need some adjustments for such filesystems.

We can try to make it work even in the weird device size combinations, but not at the cost of complicating the code unnecessarily. I don't want to discourage you, so if you eg. make it work with 64M devices that would be good enough though going even lower should be possible.

That would be great, thanks.