Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[dm-thin] Fix a bug in dm_btree_remove that could leave leaf values w…
…ith incorrect reference counts.

Thinp uses a 2 level nested btree to store it's mappings.  This first
level is indexed by thin device, and the second level by logical
block.

Often when we're removing an entry in this mapping tree we need to
rebalance nodes, which can involve shadowing them, possibly creating a
copy if the block is shared.  If we do create a copy then children of
that node need to have their reference counts incremented.  In this
way reference counts percolate down the tree as shared trees diverge.

The rebalance functions were incrementing the children at the
appropriate time, but they were always assuming the children were
internal nodes.  This meant the leaf values (in our case packed
block/flags entries) were not being incremented.

The effect of this was removal of a shared block could result in the
space maps thinking the block was no longer used.  More concretely, if
you have a thin device and a snapshot of it, sending a discard to a
shared region of the thin could corrupt the snapshot.
  • Loading branch information
jthornber committed Mar 6, 2013
1 parent 6bbc70a commit a42dfef
Showing 1 changed file with 24 additions and 22 deletions.
46 changes: 24 additions & 22 deletions drivers/md/persistent-data/dm-btree-remove.c
Expand Up @@ -139,15 +139,8 @@ struct child {
struct btree_node *n;
};

static struct dm_btree_value_type le64_type = {
.context = NULL,
.size = sizeof(__le64),
.inc = NULL,
.dec = NULL,
.equal = NULL
};

static int init_child(struct dm_btree_info *info, struct btree_node *parent,
static int init_child(struct dm_btree_info *info, struct dm_btree_value_type *vt,
struct btree_node *parent,
unsigned index, struct child *result)
{
int r, inc;
Expand All @@ -164,7 +157,7 @@ static int init_child(struct dm_btree_info *info, struct btree_node *parent,
result->n = dm_block_data(result->block);

if (inc)
inc_children(info->tm, result->n, &le64_type);
inc_children(info->tm, result->n, vt);

*((__le64 *) value_ptr(parent, index)) =
cpu_to_le64(dm_block_location(result->block));
Expand Down Expand Up @@ -236,19 +229,19 @@ static void __rebalance2(struct dm_btree_info *info, struct btree_node *parent,
}

static int rebalance2(struct shadow_spine *s, struct dm_btree_info *info,
unsigned left_index)
struct dm_btree_value_type *vt, unsigned left_index)
{
int r;
struct btree_node *parent;
struct child left, right;

parent = dm_block_data(shadow_current(s));

r = init_child(info, parent, left_index, &left);
r = init_child(info, vt, parent, left_index, &left);
if (r)
return r;

r = init_child(info, parent, left_index + 1, &right);
r = init_child(info, vt, parent, left_index + 1, &right);
if (r) {
exit_child(info, &left);
return r;
Expand Down Expand Up @@ -368,7 +361,7 @@ static void __rebalance3(struct dm_btree_info *info, struct btree_node *parent,
}

static int rebalance3(struct shadow_spine *s, struct dm_btree_info *info,
unsigned left_index)
struct dm_btree_value_type *vt, unsigned left_index)
{
int r;
struct btree_node *parent = dm_block_data(shadow_current(s));
Expand All @@ -377,17 +370,17 @@ static int rebalance3(struct shadow_spine *s, struct dm_btree_info *info,
/*
* FIXME: fill out an array?
*/
r = init_child(info, parent, left_index, &left);
r = init_child(info, vt, parent, left_index, &left);
if (r)
return r;

r = init_child(info, parent, left_index + 1, &center);
r = init_child(info, vt, parent, left_index + 1, &center);
if (r) {
exit_child(info, &left);
return r;
}

r = init_child(info, parent, left_index + 2, &right);
r = init_child(info, vt, parent, left_index + 2, &right);
if (r) {
exit_child(info, &left);
exit_child(info, &center);
Expand Down Expand Up @@ -434,7 +427,8 @@ static int get_nr_entries(struct dm_transaction_manager *tm,
}

static int rebalance_children(struct shadow_spine *s,
struct dm_btree_info *info, uint64_t key)
struct dm_btree_info *info,
struct dm_btree_value_type *vt, uint64_t key)
{
int i, r, has_left_sibling, has_right_sibling;
uint32_t child_entries;
Expand Down Expand Up @@ -472,13 +466,13 @@ static int rebalance_children(struct shadow_spine *s,
has_right_sibling = i < (le32_to_cpu(n->header.nr_entries) - 1);

if (!has_left_sibling)
r = rebalance2(s, info, i);
r = rebalance2(s, info, vt, i);

else if (!has_right_sibling)
r = rebalance2(s, info, i - 1);
r = rebalance2(s, info, vt, i - 1);

else
r = rebalance3(s, info, i - 1);
r = rebalance3(s, info, vt, i - 1);

return r;
}
Expand Down Expand Up @@ -529,7 +523,7 @@ static int remove_raw(struct shadow_spine *s, struct dm_btree_info *info,
if (le32_to_cpu(n->header.flags) & LEAF_NODE)
return do_leaf(n, key, index);

r = rebalance_children(s, info, key);
r = rebalance_children(s, info, vt, key);
if (r)
break;

Expand All @@ -550,6 +544,14 @@ static int remove_raw(struct shadow_spine *s, struct dm_btree_info *info,
return r;
}

static struct dm_btree_value_type le64_type = {
.context = NULL,
.size = sizeof(__le64),
.inc = NULL,
.dec = NULL,
.equal = NULL
};

int dm_btree_remove(struct dm_btree_info *info, dm_block_t root,
uint64_t *keys, dm_block_t *new_root)
{
Expand Down

0 comments on commit a42dfef

Please sign in to comment.