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

[Omar Sandoval <osandov@osandov.com>]

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