Re: progs integration branch moved to master (new default leafsize)

Re: progs integration branch moved to master (new default leafsize)

[Chris Mason]

Quoting David Sterba (2013-11-14 07:41:21)
> On Fri, Nov 08, 2013 at 05:01:35PM -0500, Chris Mason wrote:
> > The patch below switches our default mkfs leafsize up to 16K.  This
> > should be a better choice in almost every workload, but now is your
> > chance to complain if it causes trouble.
> 
> We should also turn the extended refs on by default now, it's been
> around since 3.7, I'm not expecting backward compatibility issues in
> general.

[ Sorry for the empty reply on the patch itself, fat fingered the quit
when I decided to reply here instead ]

This is pushed out now, thanks Dave!

-chris

progs integration branch moved to master (new default leafsize)

[Chris Mason]

Hi everyone,

This patch is now the tip of the master branch for btrfs-progs, which
has been updated to include most of the backlogged progs patches.
Please take a look and give it a shake.  This was based on Dave's
integration tree (many thanks Dave!) minus the patches for online dedup.
I've pulled in the coverity fixes and a few others from the list as
well.

The patch below switches our default mkfs leafsize up to 16K.  This
should be a better choice in almost every workload, but now is your
chance to complain if it causes trouble.

--------------------------------
16KB is faster and leads to less metadata fragmentation in almost all
workloads.  It does slightly increase lock contention on the root nodes
in some workloads, but that is best dealt with by adding more subvolumes
(for now).

This uses 16KB or the page size, whichever is bigger.  If you're doing a
mixed block group mkfs, it uses the sectorsize instead.

Since the kernel refuses to mount a mixed block group FS where the
metadata leaf size doesn't match the data sectorsize, this also adds a
similar check during mkfs.

Signed-off-by: Chris Mason <chris.mason@fusionio.com>
---
 mkfs.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/mkfs.c b/mkfs.c
index bf8a831..cd0af9e 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -46,6 +46,8 @@
 
 static u64 index_cnt = 2;
 
+#define DEFAULT_MKFS_LEAF_SIZE 16384
+
 struct directory_name_entry {
 	char *dir_name;
 	char *path;
@@ -1222,7 +1224,7 @@ int main(int ac, char **av)
 	u64 alloc_start = 0;
 	u64 metadata_profile = 0;
 	u64 data_profile = 0;
-	u32 leafsize = sysconf(_SC_PAGESIZE);
+	u32 leafsize = max_t(u32, sysconf(_SC_PAGESIZE), DEFAULT_MKFS_LEAF_SIZE);
 	u32 sectorsize = 4096;
 	u32 nodesize = leafsize;
 	u32 stripesize = 4096;
@@ -1232,6 +1234,7 @@ int main(int ac, char **av)
 	int ret;
 	int i;
 	int mixed = 0;
+	int leaf_forced = 0;
 	int data_profile_opt = 0;
 	int metadata_profile_opt = 0;
 	int discard = 1;
@@ -1269,6 +1272,7 @@ int main(int ac, char **av)
 			case 'n':
 				nodesize = parse_size(optarg);
 				leafsize = parse_size(optarg);
+				leaf_forced = 1;
 				break;
 			case 'L':
 				label = parse_label(optarg);
@@ -1386,8 +1390,21 @@ int main(int ac, char **av)
 				BTRFS_BLOCK_GROUP_RAID0 : 0; /* raid0 or single */
 		}
 	} else {
+		u32 best_leafsize = max_t(u32, sysconf(_SC_PAGESIZE), sectorsize);
 		metadata_profile = 0;
 		data_profile = 0;
+
+		if (!leaf_forced) {
+			leafsize = best_leafsize;
+			nodesize = best_leafsize;
+			if (check_leaf_or_node_size(leafsize, sectorsize))
+				exit(1);
+		}
+		if (leafsize != sectorsize) {
+			fprintf(stderr, "Error: mixed metadata/data block groups "
+				"require metadata blocksizes equal to the sectorsize\n");
+			exit(1);
+		}
 	}
 
 	ret = test_num_disk_vs_raid(metadata_profile, data_profile,
-- 
1.8.2

Re: progs integration branch moved to master (new default leafsize)

[Martin]

On 08/11/13 22:01, Chris Mason wrote:
> Hi everyone,
> 
> This patch is now the tip of the master branch for btrfs-progs, which
> has been updated to include most of the backlogged progs patches.
> Please take a look and give it a shake.  This was based on Dave's
> integration tree (many thanks Dave!) minus the patches for online dedup.
> I've pulled in the coverity fixes and a few others from the list as
> well.
> 
> The patch below switches our default mkfs leafsize up to 16K.  This
> should be a better choice in almost every workload, but now is your
> chance to complain if it causes trouble.

Thanks for that and nicely timely!

Compiling on Gentoo (3.11.5-gentoo, sys-fs/btrfs-progs-9999) gives:


 * QA Notice: Package triggers severe warnings which indicate that it
 *            may exhibit random runtime failures.
 * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
 * volumes.c:1930:5: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]
 * volumes.c:1931:6: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]

 * Please do not file a Gentoo bug and instead report the above QA
 * issues directly to the upstream developers of this software.
 * Homepage: https://btrfs.wiki.kernel.org




> --------------------------------
> 16KB is faster and leads to less metadata fragmentation in almost all
> workloads.  It does slightly increase lock contention on the root nodes
> in some workloads, but that is best dealt with by adding more subvolumes
> (for now).

Interesting and I was wondering about that. Good update.

Also, hopefully that is a little more friendly for SSDs where often you
see improved performance for 8kByte or 16kByte (aligned) writes...


Testing in progress,

Regards,
Martin



> This uses 16KB or the page size, whichever is bigger.  If you're doing a
> mixed block group mkfs, it uses the sectorsize instead.
> 
> Since the kernel refuses to mount a mixed block group FS where the
> metadata leaf size doesn't match the data sectorsize, this also adds a
> similar check during mkfs.
> 
> Signed-off-by: Chris Mason <chris.mason@fusionio.com>
> ---
>  mkfs.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs.c b/mkfs.c
> index bf8a831..cd0af9e 100644
> --- a/mkfs.c
> +++ b/mkfs.c
> @@ -46,6 +46,8 @@
>  
>  static u64 index_cnt = 2;
>  
> +#define DEFAULT_MKFS_LEAF_SIZE 16384
> +
>  struct directory_name_entry {
>  	char *dir_name;
>  	char *path;
> @@ -1222,7 +1224,7 @@ int main(int ac, char **av)
>  	u64 alloc_start = 0;
>  	u64 metadata_profile = 0;
>  	u64 data_profile = 0;
> -	u32 leafsize = sysconf(_SC_PAGESIZE);
> +	u32 leafsize = max_t(u32, sysconf(_SC_PAGESIZE), DEFAULT_MKFS_LEAF_SIZE);
>  	u32 sectorsize = 4096;
>  	u32 nodesize = leafsize;
>  	u32 stripesize = 4096;
> @@ -1232,6 +1234,7 @@ int main(int ac, char **av)
>  	int ret;
>  	int i;
>  	int mixed = 0;
> +	int leaf_forced = 0;
>  	int data_profile_opt = 0;
>  	int metadata_profile_opt = 0;
>  	int discard = 1;
> @@ -1269,6 +1272,7 @@ int main(int ac, char **av)
>  			case 'n':
>  				nodesize = parse_size(optarg);
>  				leafsize = parse_size(optarg);
> +				leaf_forced = 1;
>  				break;
>  			case 'L':
>  				label = parse_label(optarg);
> @@ -1386,8 +1390,21 @@ int main(int ac, char **av)
>  				BTRFS_BLOCK_GROUP_RAID0 : 0; /* raid0 or single */
>  		}
>  	} else {
> +		u32 best_leafsize = max_t(u32, sysconf(_SC_PAGESIZE), sectorsize);
>  		metadata_profile = 0;
>  		data_profile = 0;
> +
> +		if (!leaf_forced) {
> +			leafsize = best_leafsize;
> +			nodesize = best_leafsize;
> +			if (check_leaf_or_node_size(leafsize, sectorsize))
> +				exit(1);
> +		}
> +		if (leafsize != sectorsize) {
> +			fprintf(stderr, "Error: mixed metadata/data block groups "
> +				"require metadata blocksizes equal to the sectorsize\n");
> +			exit(1);
> +		}
>  	}
>  
>  	ret = test_num_disk_vs_raid(metadata_profile, data_profile,

Re: progs integration branch moved to master (new default leafsize)

[Chris Mason]

Quoting Martin (2013-11-08 18:53:06)
> On 08/11/13 22:01, Chris Mason wrote:
> > Hi everyone,
> > 
> > This patch is now the tip of the master branch for btrfs-progs, which
> > has been updated to include most of the backlogged progs patches.
> > Please take a look and give it a shake.  This was based on Dave's
> > integration tree (many thanks Dave!) minus the patches for online dedup.
> > I've pulled in the coverity fixes and a few others from the list as
> > well.
> > 
> > The patch below switches our default mkfs leafsize up to 16K.  This
> > should be a better choice in almost every workload, but now is your
> > chance to complain if it causes trouble.
> 
> Thanks for that and nicely timely!
> 
> Compiling on Gentoo (3.11.5-gentoo, sys-fs/btrfs-progs-9999) gives:
> 
> 
>  * QA Notice: Package triggers severe warnings which indicate that it
>  *            may exhibit random runtime failures.
>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
> break strict-aliasing rules [-Wstrict-aliasing]
>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
> break strict-aliasing rules [-Wstrict-aliasing]

I'm not seeing these warnings with the current master branch, could you
please rerun?

-chris

Re: progs integration branch moved to master (new default leafsize)

[Martin]

On 21/11/13 23:37, Chris Mason wrote:
> Quoting Martin (2013-11-08 18:53:06)
>> On 08/11/13 22:01, Chris Mason wrote:
>>> Hi everyone,
>>>
>>> This patch is now the tip of the master branch for btrfs-progs, which
>>> has been updated to include most of the backlogged progs patches.
>>> Please take a look and give it a shake.  This was based on Dave's
>>> integration tree (many thanks Dave!) minus the patches for online dedup.
>>> I've pulled in the coverity fixes and a few others from the list as
>>> well.
>>>
>>> The patch below switches our default mkfs leafsize up to 16K.  This
>>> should be a better choice in almost every workload, but now is your
>>> chance to complain if it causes trouble.
>>
>> Thanks for that and nicely timely!
>>
>> Compiling on Gentoo (3.11.5-gentoo, sys-fs/btrfs-progs-9999) gives:
>>
>>
>>  * QA Notice: Package triggers severe warnings which indicate that it
>>  *            may exhibit random runtime failures.
>>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
>> strict-aliasing rules [-Wstrict-aliasing]
>>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
>> break strict-aliasing rules [-Wstrict-aliasing]
>>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
>> break strict-aliasing rules [-Wstrict-aliasing]
> 
> I'm not seeing these warnings with the current master branch, could you
> please rerun?

>From just now:


 * QA Notice: Package triggers severe warnings which indicate that it
 *            may exhibit random runtime failures.
 * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
strict-aliasing rules [-Wstrict-aliasing]
 * volumes.c:1930:5: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]
 * volumes.c:1931:6: warning: dereferencing type-punned pointer will
break strict-aliasing rules [-Wstrict-aliasing]

 * Please do not file a Gentoo bug and instead report the above QA
 * issues directly to the upstream developers of this software.
 * Homepage: https://btrfs.wiki.kernel.org

>>> Installing (1 of 1) sys-fs/btrfs-progs-9999


... Which is exactly the same.

This is for Gentoo for:

gcc: x86_64-pc-linux-gnu-4.7.3

# gcc --version
gcc (Gentoo 4.7.3-r1 p1.3, pie-0.5.5) 4.7.3

Kernel: 3.11.9-gentoo

# btrfs version
Btrfs v0.20-rc1-597-g5aff090



And the "-9999" pulls the code in from:

>From git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs
   9f0c53f..5aff090  master     -> master
GIT update -->
   repository:
git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git
   updating from commit:     9f0c53f574b242b0d5988db2972c8aac77ef35a9
   to commit:                5aff090a3951e7d787b32bb5c49adfec65091385
 cmds-filesystem.c | 79
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs.c            | 18 +++++++++---------
 2 files changed, 88 insertions(+), 9 deletions(-)
   branch:                   master
   storage directory:
"/usr/portage/distfiles/egit-src/btrfs-progs.git"
   checkout type:            bare repository
Cloning into
'/var/tmp/portage/sys-fs/btrfs-progs-9999/work/btrfs-progs-9999'...
done.
Checking connectivity... done
Branch branch-master set up to track remote branch master from origin.
Switched to a new branch 'branch-master'


Hope that helps,

Regards,
Martin

Re: progs integration branch moved to master (new default leafsize)

[Chris Mason]

Quoting Martin (2013-11-22 04:03:41)
> On 21/11/13 23:37, Chris Mason wrote:
> > Quoting Martin (2013-11-08 18:53:06)
> >> On 08/11/13 22:01, Chris Mason wrote:
> >>> Hi everyone,
> >>>
> >>> This patch is now the tip of the master branch for btrfs-progs, which
> >>> has been updated to include most of the backlogged progs patches.
> >>> Please take a look and give it a shake.  This was based on Dave's
> >>> integration tree (many thanks Dave!) minus the patches for online dedup.
> >>> I've pulled in the coverity fixes and a few others from the list as
> >>> well.
> >>>
> >>> The patch below switches our default mkfs leafsize up to 16K.  This
> >>> should be a better choice in almost every workload, but now is your
> >>> chance to complain if it causes trouble.
> >>
> >> Thanks for that and nicely timely!
> >>
> >> Compiling on Gentoo (3.11.5-gentoo, sys-fs/btrfs-progs-9999) gives:
> >>
> >>
> >>  * QA Notice: Package triggers severe warnings which indicate that it
> >>  *            may exhibit random runtime failures.
> >>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
> >> strict-aliasing rules [-Wstrict-aliasing]
> >>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
> >> break strict-aliasing rules [-Wstrict-aliasing]
> >>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
> >> break strict-aliasing rules [-Wstrict-aliasing]
> > 
> > I'm not seeing these warnings with the current master branch, could you
> > please rerun?
> 
> From just now:
> 
> 
>  * QA Notice: Package triggers severe warnings which indicate that it
>  *            may exhibit random runtime failures.
>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
> strict-aliasing rules [-Wstrict-aliasing]
>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
> break strict-aliasing rules [-Wstrict-aliasing]
>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
> break strict-aliasing rules [-Wstrict-aliasing]

Does gentoo modify the optimizations from the Makefile?  We actually
have many strict-aliasing warnings, but I didn't think they came up
until -O2.

At any rate, I'm adding -fno-strict-aliasing just to be sure.

-chris

Re: progs integration branch moved to master (new default leafsize)

[Martin]

On 22/11/13 13:40, Chris Mason wrote:
> Quoting Martin (2013-11-22 04:03:41)

>>  * QA Notice: Package triggers severe warnings which indicate that it
>>  *            may exhibit random runtime failures.
>>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
>> strict-aliasing rules [-Wstrict-aliasing]
>>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
>> break strict-aliasing rules [-Wstrict-aliasing]
>>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
>> break strict-aliasing rules [-Wstrict-aliasing]
> 
> Does gentoo modify the optimizations from the Makefile?  We actually
> have many strict-aliasing warnings, but I didn't think they came up
> until -O2.

For that system, I have -Os set in the Gentoo "make.conf".


> At any rate, I'm adding -fno-strict-aliasing just to be sure.

Good to catch to avoid unexpectedness,


Regards,
Martin

Re: progs integration branch moved to master (new default leafsize)

[Chris Mason]

Quoting Martin (2013-11-22 14:50:17)
> On 22/11/13 13:40, Chris Mason wrote:
> > Quoting Martin (2013-11-22 04:03:41)
> 
> >>  * QA Notice: Package triggers severe warnings which indicate that it
> >>  *            may exhibit random runtime failures.
> >>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
> >> strict-aliasing rules [-Wstrict-aliasing]
> >>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
> >> break strict-aliasing rules [-Wstrict-aliasing]
> >>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
> >> break strict-aliasing rules [-Wstrict-aliasing]
> > 
> > Does gentoo modify the optimizations from the Makefile?  We actually
> > have many strict-aliasing warnings, but I didn't think they came up
> > until -O2.
> 
> For that system, I have -Os set in the Gentoo "make.conf".
> 
> 
> > At any rate, I'm adding -fno-strict-aliasing just to be sure.
> 
> Good to catch to avoid unexpectedness,

Ok, please try with the current master to make sure the options are
being picked up properly.  If you're overriding the
-fno-strict-aliasing, please don't ;)

-chris

Re: progs integration branch moved to master (new default leafsize)

[Martin]

On 22/11/13 19:57, Chris Mason wrote:
> Quoting Martin (2013-11-22 14:50:17)
>> On 22/11/13 13:40, Chris Mason wrote:
>>> Quoting Martin (2013-11-22 04:03:41)
>>
>>>>  * QA Notice: Package triggers severe warnings which indicate that it
>>>>  *            may exhibit random runtime failures.
>>>>  * disk-io.c:91:5: warning: dereferencing type-punned pointer will break
>>>> strict-aliasing rules [-Wstrict-aliasing]
>>>>  * volumes.c:1930:5: warning: dereferencing type-punned pointer will
>>>> break strict-aliasing rules [-Wstrict-aliasing]
>>>>  * volumes.c:1931:6: warning: dereferencing type-punned pointer will
>>>> break strict-aliasing rules [-Wstrict-aliasing]
>>>
>>> Does gentoo modify the optimizations from the Makefile?  We actually
>>> have many strict-aliasing warnings, but I didn't think they came up
>>> until -O2.
>>
>> For that system, I have -Os set in the Gentoo "make.conf".
>>
>>
>>> At any rate, I'm adding -fno-strict-aliasing just to be sure.
>>
>> Good to catch to avoid unexpectedness,
> 
> Ok, please try with the current master to make sure the options are
> being picked up properly.  If you're overriding the
> -fno-strict-aliasing, please don't ;)

No changes my side for that system and...

btrfs-progs now compiles with no warnings given. That looks like a "fixed".


# emerge -vD btrfs-progs

These are the packages that would be merged, in order:

Calculating dependencies... done!
[ebuild   R   *] sys-fs/btrfs-progs-9999  0 kB

Total: 1 package (1 reinstall), Size of downloads: 0 kB


>>> Verifying ebuild manifests

>>> Emerging (1 of 1) sys-fs/btrfs-progs-9999
>>> Unpacking source...
GIT update -->
   repository:
git://git.kernel.org/pub/scm/linux/kernel/git/mason/btrfs-progs.git
   at the commit:            8116550e16628794b76051b6b8ea503055c08d6f
   branch:                   master
   storage directory:
"/usr/portage/distfiles/egit-src/btrfs-progs.git"
   checkout type:            bare repository
Cloning into
'/var/tmp/portage/sys-fs/btrfs-progs-9999/work/btrfs-progs-9999'...
done.
Checking connectivity... done
Branch branch-master set up to track remote branch master from origin.
Switched to a new branch 'branch-master'

... And then a clean compile. No warnings.


Thanks,
Martin

Re: progs integration branch moved to master (new default leafsize)

[Duncan]

Chris Mason posted on Fri, 22 Nov 2013 08:40:38 -0500 as excerpted:

> Does gentoo modify the optimizations from the Makefile?  We actually
> have many strict-aliasing warnings, but I didn't think they came up
> until -O2.
> 
> At any rate, I'm adding -fno-strict-aliasing just to be sure.

FWIW, Gentoo lets users set their own CFLAGS/CXXFLAGS/LDFLAGS, etc. 

Gentoo recommends -O2 at the global system level and I believe sets that 
as the default before user modification, as well.  -Os is considered a 
viable option, but users setting -O3 or -Ofast or whatever are often 
considered ricers and can be told to try to reproduce bugs with -O2 set.  
And -O1 or lower is not recommended either, simply because it's not 
widely tested.

Individual package builds can and routinely do, however, override or 
filter specific flags where they've been demonstrated to be problematic, 
and in some cases, may add flags such as -fno-strict-aliasing where 
upstream code requires it.

But of course a (normally masked unless a user specifically unmasks it) 
-9999 version is gentoo's way of noting a live-from-upstream-vcs version, 
and that's the case here as well.  btrfs-progs-9999 fetches and builds 
the live-git btrfs-progs direct from upstream git, and as such, it's 
subject to far less QA than ~arch (~x86, ~amd64, etc) aka testing 
packages, which in turn are less stable-tested than stable arch (x86, 
amd64, etc) packages.  Basically, gentooers unmasking -9999 packages are 
expected to be able to deal with any breakage a live-vcs build may have 
and/or trigger.

And it's likely that -fno-strict-aliasing simply hasn't made it to the 
live build yet.  But by the same token, I don't believe it'll override an 
upstream makefile's use of the flag, so now that you've added it...

... And indeed, after just rebuilding it here, with the commit (8116550) 
adding -fno-strict-aliasing as HEAD, I see no further type-punned pointer 
warnings. =:^)

I do however still see this:

cmds-restore.c: In function 'next_leaf':
cmds-restore.c:174:20: warning: array subscript is above array bounds [-
Warray-bounds]
  slot = path->slots[level] + 1;
                    ^

(I am running a newer gcc than Martin, however:
gcc (Gentoo 4.8.1-r1 p1.2, pie-0.5.7) 4.8.1
)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

Re: progs integration branch moved to master (new default leafsize)

[David Sterba]

On Fri, Nov 08, 2013 at 05:01:35PM -0500, Chris Mason wrote:
> The patch below switches our default mkfs leafsize up to 16K.  This
> should be a better choice in almost every workload, but now is your
> chance to complain if it causes trouble.

We should also turn the extended refs on by default now, it's been
around since 3.7, I'm not expecting backward compatibility issues in
general.


david