From 9f885b4c12cdb0b43ab83fe74886d80cf4faa6c5 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Wed, 1 Feb 2017 09:48:09 -0800 Subject: [PATCH] Fix item erase augmentation The item cache was getting inconsistent as items were removed. This would manifest in failing to find dirty items that it had counted as it was writing items into the segment and removing deletion items. For a start it wasn't using the augmented rb_erase(). We make a function that everyone uses. There's no augmented rb_replace() so We just augment erase, restart, and insert. (We could probably augment on descent and replace/propagate but that can come later.) Then the augmentation callbacks got the semantics slightly wrong. The rotation callback is named after a caller that happens to use it, not on any implied relationship between the nodes. It actually just recalculates the augmentation value for the two subtrees. Mischief managed. (We'll probably rework the augmentation so the value is for the node and its children and we can get rid of the extra code we have today to support our augmentation value that is sensitive to the difference between the left and write subtrees.) Signed-off-by: Zach Brown --- kmod/src/item.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/kmod/src/item.c b/kmod/src/item.c index fbd7dc36..538b3ce1 100644 --- a/kmod/src/item.c +++ b/kmod/src/item.c @@ -239,10 +239,9 @@ static void scoutfs_item_rb_propagate(struct rb_node *node, static void scoutfs_item_rb_copy(struct rb_node *old, struct rb_node *new) { - struct cached_item *o = container_of(old, struct cached_item, node); struct cached_item *n = container_of(new, struct cached_item, node); - n->dirty = o->dirty; + n->dirty = compute_item_dirty(n); } /* calculate the new parent last as it depends on the old parent */ @@ -251,8 +250,6 @@ static void scoutfs_item_rb_rotate(struct rb_node *old, struct rb_node *new) struct cached_item *o = container_of(old, struct cached_item, node); struct cached_item *n = container_of(new, struct cached_item, node); - BUG_ON(rb_parent(old) != new); - o->dirty = compute_item_dirty(o); n->dirty = compute_item_dirty(n); } @@ -325,6 +322,20 @@ static void clear_item_dirty(struct item_cache *cac, update_dirty_parents(item); } +/* + * Safely erase an item from the tree. Make sure to remove its dirty + * accounting, use the augmented erase, and free it. + */ +static void erase_item(struct super_block *sb, struct item_cache *cac, + struct cached_item *item) +{ + trace_printk("erasing item %p\n", item); + + clear_item_dirty(cac, item); + rb_erase_augmented(&item->node, &cac->items, &scoutfs_item_rb_cb); + free_item(sb, item); +} + /* * Try to insert the given item. If there's already a non-deletion item * with the insertion key then return -EEXIST. An existing deletion @@ -336,11 +347,14 @@ static int insert_item(struct super_block *sb, struct item_cache *cac, struct cached_item *ins) { struct rb_root *root = &cac->items; - struct rb_node **node = &root->rb_node; - struct rb_node *parent = NULL; struct cached_item *item; + struct rb_node *parent; + struct rb_node **node; int cmp; +restart: + node = &root->rb_node; + parent = NULL; while (*node) { parent = *node; item = container_of(*node, struct cached_item, node); @@ -358,10 +372,9 @@ static int insert_item(struct super_block *sb, struct item_cache *cac, if (!item->deletion) return -EEXIST; - clear_item_dirty(cac, item); - rb_replace_node(&item->node, &ins->node, root); - free_item(sb, item); - return 0; + /* sadly there's no augmented replace */ + erase_item(sb, cac, item); + goto restart; } } @@ -1275,10 +1288,8 @@ int scoutfs_item_dirty_seg(struct super_block *sb, struct scoutfs_segment *seg) del = item; item = next_dirty(item); - if (del->deletion) { - rb_erase(&del->node, &cac->items); - free_item(sb, del); - } + if (del->deletion) + erase_item(sb, cac, del); nr_items--; }