[PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[Zhao Lei]

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

[PATCH 1/3] btrfs-progs: mkfs: Remove saved_optind in mkfs.btrfs

[Zhao Lei]

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.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 mkfs.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index 7fa7cfc..cdae94d 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -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

[PATCH 2/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[Zhao Lei]

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.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 mkfs.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/mkfs.c b/mkfs.c
index cdae94d..29cab13 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -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

[PATCH 3/3] btrfs-progs: mkfs: Fix inaccurate mixed information

[Zhao Lei]

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.

Signed-off-by: Zhao Lei <zhaolei@cn.fujitsu.com>
---
 mkfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mkfs.c b/mkfs.c
index 29cab13..0064a78 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -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

Re: [PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[David Sterba]

On Tue, Oct 13, 2015 at 08:52:16PM +0800, Zhao Lei wrote:
> 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.

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.

RE: [PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[Zhao Lei]

Hi, David Sterba

> -----Original Message-----
> From: David Sterba [mailto:dsterba@suse.cz]
> Sent: Wednesday, October 14, 2015 12:19 AM
> To: Zhao Lei <zhaolei@cn.fujitsu.com>
> Cc: linux-btrfs@vger.kernel.org
> Subject: Re: [PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by
> argument sequence
> 
> On Tue, Oct 13, 2015 at 08:52:16PM +0800, Zhao Lei wrote:
> > 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.
> 
> 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.

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

Re: [PATCH 0/3] btrfs-progs: mkfs: Fix different mixed type by argument sequence

[David Sterba]

On Wed, Oct 14, 2015 at 12:28:03PM +0800, Zhao Lei wrote:
> 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/vdh2 /dev/vdh1
> btrfs-progs v4.2.2-25-gc6b29b6-dirty
> See http://btrfs.wiki.kernel.org for more information.
> 
> not enough free space

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

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

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.

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

That would be great, thanks.