Re: panic: assertion: parent != NULL in hammer_cursor_removed_node (v2.5.1.187.gc1543-DEV, X86_64)

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:Although the backtrace is garbled, this is most likely the panic
:introduced by the committed fix for http://bugs.dragonflybsd.org/issue1577,
:especially since it happened while running `hammer cleanup' from
:a periodic(8) job.
:For some reason the search facility on our bug tracker can't find it
:with a keyword like `hammer_cursor_removed_node' even with full text search.
:
:The commit f3a4893b has moved the call to hammer_cursor_removed_node()
:in btree_remove() to after the recursive call to itself, but a call to
:btree_remove() moves the cursor up after deleting the sub tree, so it's
:possible that it reaches the root of the tree during the recursion.
:When that happens, cursor->parent is set to NULL and ondisk->parent of
:the current node is also 0.
:
:I have a bandaid for this problem (it's being tested):
:
:HAMMER VFS - don't call hammer_cursor_removed_node when the recursion reached the root of the filesystem

    Ok, if the recursion is successful and winds up removing an internal
    node just below the root node, then when it cursors up again you
    are correct:  It will be at the root node and cursor->parent will
    be NULL.

    For this case what we really want it to do is call
    hammer_cursor_removed_node() with the original (now deleted) node
    as the first argument and the root node as the second argument.

    We absolutely MUST call hammer_cursor_removed_node() for any node
    which is removed, so we can't conditionalize it as in your patch.

    I think I may be calling hammer_cursor_removed_node() with the wrong
    node.  I think I have to use cursor->node/cursor->index instead of
    cursor->parent/cursor->parent_index.  The recursion issues a
    cursor_up in the no-error case after all.

    Lets see.  hammer_btree_remove() recurses and hits a stop where
    (parent->ondisk->count > 1).  So it is able to remove the node and
    does not have to remove the parent.  The last thing it does is
    call hammer_cursor_up():

                /*
                 * cursor->node is invalid, cursor up to make the cursor
                 * valid again.
                 */
                error = hammer_cursor_up(cursor);

    So now cursor->node points at the parent which is still intact (and
    could end up being the root node).   So on the return from the
    recursion we should be using cursor->node, not cursor->parent.

    I'm thinking something like this (see below).  UNTESTED!  I will
    start some tests up today.  It could panic instantly :-)

					-Matt
					Matthew Dillon 
					<dillon@backplane.com>

diff --git a/sys/vfs/hammer/hammer_btree.c b/sys/vfs/hammer/hammer_btree.c
index 6ee1e1a..050d4a2 100644
--- a/sys/vfs/hammer/hammer_btree.c
+++ b/sys/vfs/hammer/hammer_btree.c
@@ -2226,9 +2226,10 @@ btree_remove(hammer_cursor_t cursor)
 			hammer_cursor_deleted_element(cursor->node, 0);
 			error = btree_remove(cursor);
 			if (error == 0) {
+				KKASSERT(node != cursor->node);
 				hammer_cursor_removed_node(
-					node, cursor->parent,
-					cursor->parent_index);
+					node, cursor->node,
+					cursor->index);
 				hammer_modify_node_all(cursor->trans, node);
 				ondisk = node->ondisk;
 				ondisk->type = HAMMER_BTREE_TYPE_DELETED;