| Message ID | 20131108220135.3802.39092@localhost.localdomain (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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, -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 )
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 -- 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
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,
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(-)