• Josef Bacik's avatar
    btrfs: remove bogus BUG_ON in alloc_reserved_tree_block · b7774425
    Josef Bacik authored
    The fix 361048f5 ("Btrfs: fix full backref problem when inserting
    shared block reference") added a delayed ref flushing at subvolume
    creation time in order to avoid hitting this particular BUG_ON().
    
    Before this fix, we were tripping the BUG_ON() by
    
    1. Modify snapshot A, which creates blocks with a normal reference for
       snapshot A, as A is the owner of these blocks.  We now have delayed
       refs for these blocks.
    2. Create a snapshot of A named B, which pushes references for the
       children blocks of the root node for the new root B, thus creating
       more delayed refs for newly allocated blocks.
    3. A is modified, and because the metadata blocks can now be shared, it
       must push FULL_BACKREF references to the children of any block that A
       COWs down it's path to its target key.
    4. Delayed refs are run.  Because these are newly allocated blocks, we
       have ->must_insert_reserved reserved set on the delayed ref head, we
       call into alloc_reserved_tree_block() to add the extent item, and
       then add our ref.  At the time of this fix, we were ordering
       FULL_BACKREF delayed ref operations first, so we'd go to add this
       reference and then BUG_ON() because we didn't have the FULL_BACKREF
       flag set.
    
    The patch fixed this problem by making sure we ran the delayed refs
    before we had the chance to modify A.  This meant that any *new* blocks
    would have had their extent items created _before_ we would ever
    actually COW down and generate FULL_BACKREF entries.  Thus the problem
    went away.
    
    However this BUG_ON() is actually completely bogus.  The existence of a
    full backref doesn't necessarily mean that FULL_BACKREF must be set on
    that block, it must only be set on the actual parent itself.  Consider
    the example provided above.  If we COW down one path from A, any nodes
    are going to have a FULL_BACKREF ref pushed down to _all_ of their
    children, but not all of the children are going to have FULL_BACKREF
    set.  It is completely valid to have an extent item with normal and full
    backrefs without FULL_BACKREF actually set on the block itself.
    
    As a final note, I have been testing with the patch (applied after this
    one)
    
      btrfs: stop running all delayed refs during snapshot
    
    which removed this flushing.  My test was a torture test which did a lot
    of operations while snapshotting and deleting snapshots as well as
    relocation, and I never tripped this BUG_ON().  This is actually because
    at the time of 361048f5, we ordered SHARED keys _before_ normal
    references, and thus they would get run first.  However currently they
    are ordered _after_ normal references, so we'd do the initial creation
    without having a shared reference, and thus not hit this BUG_ON(), which
    explains why I didn't start hitting this problem during my testing with
    my other patch applied.
    Signed-off-by: default avatarJosef Bacik <josef@toxicpanda.com>
    Signed-off-by: default avatarDavid Sterba <dsterba@suse.com>
    b7774425
extent-tree.c 157 KB