On Fri, Sep 11, 2015 at 11:58:13AM +0800, Qu Wenruo wrote: > > > Omar Sandoval wrote on 2015/09/10 20:48 -0700: > >On Fri, Sep 11, 2015 at 09:21:13AM +0800, Qu Wenruo wrote: > >>Hi Omar, > >> > >>Thanks for your patchset. > >>Quite a nice one, and debug-tree can give better output on space cache. > >>With current implement, space cache is near a black box in debug-tree > >>output. > >> > >>And current on disk format is not quite easy to understand.(In fact, space > >>cache is restored in tree root, as a NODATACOW inode, quite wired) > >> > >>Also, it should provide a quite good base for rework inode cache for future > >>development. > >> > >> > >>But I'm still a little concerned about the performance. > >> > >>One of the problem using b-tree is, now we need to use btrfs_search_slot() > >>to do modification, that means we will do level-based tree lock and COW. > >>Personally speaking, I'd like to blame that for the slow metadata > >>performance of btrfs. > >>(Yeah personal experience, may be wrong again) > >> > >>So with the new implement every space cache operation will causing tree lock > >>and cow. > >>Unlike the old wired structure, which is done in a NODATACOW fashion. > >> > >>Hopes I'm wrong about it (and it seems I'm always wrong about all these > >>assumption based performance thing). > >> > >>Thanks, > >>Qu > > > >Hey, Qu, > > > >So the thing about the free space tree is that the B-tree is only > >modified while running delayed refs, so we only incur any overhead > >during a transaction commit. The numbers I got showed that the overhead > >was better than the old free space cache and not too much more than not > >using the cache. Now that I think about it, I only profiled it under > >heavy load, though, it'd probably be a good idea to get some numbers for > >more typical workloads, but I don't currently have access to any > >reasonable hardware. > > > >Thanks, > >Omar > > Great, if its performance is better than old one under heavy load, then I'm > completely OK with it. > > Nice job! Thanks! The v1 post has specific numbers if you want to take a look: http://www.spinics.net/lists/linux-btrfs/msg46713.html. > BTW, don't forget to add btrfs-debug-tree and fsck support for the new > implement. I can't even wait to see these one merged now. Yup, the btrfs-progs patches include both :) The only caveat is that there's no visibility into the bitmap items from btrfs-debug-tree, but that wouldn't be too hard to add. > Thanks, > Qu > > > >>Omar Sandoval wrote on 2015/09/03 12:44 -0700: > >>>Here's version 2 of the the free space B-tree patches, addressing > >>>Josef's review from the last round, which you can find here: > >>>http://www.spinics.net/lists/linux-btrfs/msg46713.html > >>> > >>>Changes from v1->v2: > >>> > >>>- Cleaned up a bunch of unnecessary instances of "if (ret) goto out; ret = 0" > >>>- Added aborts in the free space tree code closer to the site the error > >>> is encountered: where we add or remove block groups, add or remove > >>> free space, and also when we convert formats > >>>- Moved loading of the free space tree into caching_thread() and added a > >>> new patch 4 in preparation for it > >>>- Commented a bunch of stuff in the extent buffer bitmap operations and > >>> refactored some of the complicated logic > >>>- Added sanity tests for the extent buffer bitmap operations and free > >>> space tree (patches 2 and 6) > >>>- Added Josef's Reviewed-by tags > >>> > >>>Omar Sandoval (9): > >>> Btrfs: add extent buffer bitmap operations > >>> Btrfs: add extent buffer bitmap sanity tests > >>> Btrfs: add helpers for read-only compat bits > >>> Btrfs: refactor caching_thread() > >>> Btrfs: introduce the free space B-tree on-disk format > >>> Btrfs: implement the free space B-tree > >>> Btrfs: add free space tree sanity tests > >>> Btrfs: wire up the free space tree to the extent tree > >>> Btrfs: add free space tree mount option > >>> > >>> fs/btrfs/Makefile | 5 +- > >>> fs/btrfs/ctree.h | 107 ++- > >>> fs/btrfs/disk-io.c | 26 + > >>> fs/btrfs/extent-tree.c | 112 ++- > >>> fs/btrfs/extent_io.c | 183 +++- > >>> fs/btrfs/extent_io.h | 10 +- > >>> fs/btrfs/free-space-tree.c | 1501 ++++++++++++++++++++++++++++++++ > >>> fs/btrfs/free-space-tree.h | 71 ++ > >>> fs/btrfs/super.c | 24 +- > >>> fs/btrfs/tests/btrfs-tests.c | 52 ++ > >>> fs/btrfs/tests/btrfs-tests.h | 10 + > >>> fs/btrfs/tests/extent-io-tests.c | 138 ++- > >>> fs/btrfs/tests/free-space-tests.c | 35 +- > >>> fs/btrfs/tests/free-space-tree-tests.c | 570 ++++++++++++ > >>> fs/btrfs/tests/qgroup-tests.c | 20 +- > >>> include/trace/events/btrfs.h | 3 +- > >>> 16 files changed, 2763 insertions(+), 104 deletions(-) > >>> create mode 100644 fs/btrfs/free-space-tree.c > >>> create mode 100644 fs/btrfs/free-space-tree.h > >>> create mode 100644 fs/btrfs/tests/free-space-tree-tests.c > >>> > > -- Omar -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
