[00/44,v5] Cleanup how we handle root refs, part 1

Message

Josef Bacik Jan. 24, 2020, 2:32 p.m. UTC

v4->v5:
- split out the btrfs_free_fs_info() moving around into it's own patch.
- updated a comment in btrfs_get_root() to describe why we are initializing part
  of the fs_info

v3->v4:
- "btrfs: hold a ref on the root in build_backref_tree" I fixed it so the
  backref nodes hold the ref for their respective roots, but I missed a case
  where we grab the reloc_root, and thus freed the reloc root too soon and it
  resulted in tears when I ran my stress testing on my tiny box.

v2->v3:
- Rebased onto the latest misc-next, so the snapshot aware defrag related
  patches got dropped.

v1->v2:
- Fixed a error missed put in an error condition in relink_extent_backref
- Added "btrfs: make the init of static elements in fs_info" so that we could
  clean up fs_info init and make the leak detectors work for the self tests.

-- original email --
In testing with the recent fsstress I stumbled upon a deadlock in how we deal
with disappearing subvolumes.  We sort of half-ass a srcu lock to protect us,
but it's used inconsistently so doesn't really provide us with actual
protection, mostly it just makes us feel good.

In order to do away with this srcu thing we need to have proper ref counting for
our roots.  We currently refcount them, but only to handle the actual kfree, it
doesn't really control the lifetime of the root.  And again, this is not done in
any sort of consistent manner so it doesn't actually protect us.

This is the first set of patches, and yes I realize there are a lot of them.
Most of them are just "hold a ref on the root" in all of the call sites that
called btrfs_read_fs_root*() variations.  Now that we're going to actually hold
references to roots we need to make sure we put the reference when we're done
with them, so these patches go through each callsite and make sure we drop the
references appropriately.

Then there's a variety of cleanups and consolidations to make things clearer and
make it so we only have 1 place to get roots.

Finally there's the root leak detection patch.  I used this with a bunch of
testing to make sure I was never leaking roots with these patches.  I've been
testing these for several weeks cleaning up all the corners, so they should be
in relatively good shape.  Most of the patches are small so straightforward to
review.

This is just part 1, this is the prep work we need to make the root lifetime a
little saner, and will allow us to drop the subvol srcu, as well as the inode
rbtree.  It doesn't really fundamentally change how roots are handled other than
making the refcounting actually protect us from freeing the root while we're
using it.  That work will come later.  Thanks,

Josef

Comments

David Sterba Feb. 4, 2020, 11:48 p.m. UTC | #1

On Fri, Jan 24, 2020 at 09:32:17AM -0500, Josef Bacik wrote:
> v4->v5:
> - split out the btrfs_free_fs_info() moving around into it's own patch.
> - updated a comment in btrfs_get_root() to describe why we are initializing part
>   of the fs_info

Patches 1-9 are preparatory and don't change the root ref logic so I'll
add them to misc-next now, the rest will follow.

David Sterba Feb. 5, 2020, 3:48 p.m. UTC | #2

On Fri, Jan 24, 2020 at 09:32:17AM -0500, Josef Bacik wrote:
> v4->v5:
> - split out the btrfs_free_fs_info() moving around into it's own patch.
> - updated a comment in btrfs_get_root() to describe why we are initializing part
>   of the fs_info

I've commented under the patches, small things that I'd rather fixup
in my branch once you look at them, no need to resend anything.

As the code is split, some changes are removed so even if there's
something called in wrong order, it lasts only a few patches. For
clarity I'd still like to have committed patches that don't have such
things left.

David Sterba Feb. 6, 2020, 4:59 p.m. UTC | #3

On Wed, Feb 05, 2020 at 04:48:41PM +0100, David Sterba wrote:
> On Fri, Jan 24, 2020 at 09:32:17AM -0500, Josef Bacik wrote:
> > v4->v5:
> > - split out the btrfs_free_fs_info() moving around into it's own patch.
> > - updated a comment in btrfs_get_root() to describe why we are initializing part
> >   of the fs_info
> 
> I've commented under the patches, small things that I'd rather fixup
> in my branch once you look at them, no need to resend anything.
> 
> As the code is split, some changes are removed so even if there's
> something called in wrong order, it lasts only a few patches. For
> clarity I'd still like to have committed patches that don't have such
> things left.

All fixups done, I'll add the branch to misc-next.