Re: [PATCH v2 0/9] free space B-tree

[Omar Sandoval <osandov@osandov.com>]

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

> 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