bcache: Fix a race when freeing btree nodes

This isn't a bulletproof fix; btree_node_free() -> bch_bucket_free() puts the bucket on the unused freelist, where it can be reused right away without any ordering requirements. It would be better to wait on at least a journal write to go down before reusing the bucket. bch_btree_set_root() does this, and inserting into non leaf nodes is completely synchronous so we should be ok, but future patches are just going to get rid of the unused freelist - it was needed in the past for various reasons but shouldn't be anymore. Signed-off-by: Kent Overstreet <kmo@daterainc.com>

bcache: Fix a race when freeing btree nodes
This isn't a bulletproof fix; btree_node_free() -> bch_bucket_free() puts the bucket on the unused freelist, where it can be reused right away without any ordering requirements. It would be better to wait on at least a journal write to go down before reusing the bucket. bch_btree_set_root() does this, and inserting into non leaf nodes is completely synchronous so we should be ok, but future patches are just going to get rid of the unused freelist - it was needed in the past for various reasons but shouldn't be anymore. Signed-off-by: Kent Overstreet <kmo@daterainc.com>
Kent Overstreet
1 parent 4fe6a81670
Showing 1 changed file with 20 additions and 33 deletions Side-by-side Diff
drivers/md/bcache/btree.c
@@ -1006,8 +1006,6 @@
  
 static void btree_node_free(struct btree *b)
 {
-	unsigned i;
-
 	trace_bcache_btree_node_free(b);
  
 	BUG_ON(b == b->c->root);
@@ -1019,14 +1017,6 @@
 	cancel_delayed_work(&b->work);
  
 	mutex_lock(&b->c->bucket_lock);
-
-	for (i = 0; i < KEY_PTRS(&b->key); i++) {
-		BUG_ON(atomic_read(&PTR_BUCKET(b->c, &b->key, i)->pin));
-
-		bch_inc_gen(PTR_CACHE(b->c, &b->key, i),
-			    PTR_BUCKET(b->c, &b->key, i));
-	}
-
 	bch_bucket_free(b->c, &b->key);
 	mca_bucket_free(b);
 	mutex_unlock(&b->c->bucket_lock);
  
  
@@ -1086,16 +1076,19 @@
 {
 	unsigned i;
  
+	mutex_lock(&b->c->bucket_lock);
+
+	atomic_inc(&b->c->prio_blocked);
+
 	bkey_copy(k, &b->key);
 	bkey_copy_key(k, &ZERO_KEY);
  
-	for (i = 0; i < KEY_PTRS(k); i++) {
-		uint8_t g = PTR_BUCKET(b->c, k, i)->gen + 1;
+	for (i = 0; i < KEY_PTRS(k); i++)
+		SET_PTR_GEN(k, i,
+			    bch_inc_gen(PTR_CACHE(b->c, &b->key, i),
+					PTR_BUCKET(b->c, &b->key, i)));
  
-		SET_PTR_GEN(k, i, g);
-	}
-
-	atomic_inc(&b->c->prio_blocked);
+	mutex_unlock(&b->c->bucket_lock);
 }
  
 static int btree_check_reserve(struct btree *b, struct btree_op *op)
@@ -1342,6 +1335,13 @@
 		bch_keylist_add(keylist, &new_nodes[i]->key);
 	}
  
+	closure_sync(&cl);
+
+	/* We emptied out this node */
+	BUG_ON(btree_bset_first(new_nodes[0])->keys);
+	btree_node_free(new_nodes[0]);
+	rw_unlock(true, new_nodes[0]);
+
 	for (i = 0; i < nodes; i++) {
 		if (__bch_keylist_realloc(keylist, bkey_u64s(&r[i].b->key)))
 			goto out_nocoalesce;
  
@@ -1350,13 +1350,9 @@
 		bch_keylist_push(keylist);
 	}
  
-	/* We emptied out this node */
-	BUG_ON(btree_bset_first(new_nodes[0])->keys);
-	btree_node_free(new_nodes[0]);
-	rw_unlock(true, new_nodes[0]);
+	bch_btree_insert_node(b, op, keylist, NULL, NULL);
+	BUG_ON(!bch_keylist_empty(keylist));
  
-	closure_sync(&cl);
-
 	for (i = 0; i < nodes; i++) {
 		btree_node_free(r[i].b);
 		rw_unlock(true, r[i].b);
@@ -1364,9 +1360,6 @@
 		r[i].b = new_nodes[i];
 	}
  
-	bch_btree_insert_node(b, op, keylist, NULL, NULL);
-	BUG_ON(!bch_keylist_empty(keylist));
-
 	memmove(r, r + 1, sizeof(r[0]) * (nodes - 1));
 	r[nodes - 1].b = ERR_PTR(-EINTR);
  
  
@@ -1456,12 +1449,11 @@
 							       keys.top);
 					bch_keylist_push(&keys);
  
-					btree_node_free(last->b);
-
 					bch_btree_insert_node(b, op, &keys,
 							      NULL, NULL);
 					BUG_ON(!bch_keylist_empty(&keys));
  
+					btree_node_free(last->b);
 					rw_unlock(true, last->b);
 					last->b = n;
  
  
  
  
@@ -1924,26 +1916,21 @@
 		closure_sync(&cl);
 		bch_btree_set_root(n3);
 		rw_unlock(true, n3);
-
-		btree_node_free(b);
 	} else if (!b->parent) {
 		/* Root filled up but didn't need to be split */
 		closure_sync(&cl);
 		bch_btree_set_root(n1);
-
-		btree_node_free(b);
 	} else {
 		/* Split a non root node */
 		closure_sync(&cl);
 		make_btree_freeing_key(b, parent_keys.top);
 		bch_keylist_push(&parent_keys);
  
-		btree_node_free(b);
-
 		bch_btree_insert_node(b->parent, op, &parent_keys, NULL, NULL);
 		BUG_ON(!bch_keylist_empty(&parent_keys));
 	}
  
+	btree_node_free(b);
 	rw_unlock(true, n1);
  
 	bch_time_stats_update(&b->c->btree_split_time, start_time);
...	...	@@ -1006,8 +1006,6 @@
1006	1006
1007	1007	static void btree_node_free(struct btree *b)
1008	1008	{
1009		- unsigned i;
1010		-
1011	1009	trace_bcache_btree_node_free(b);
1012	1010
1013	1011	BUG_ON(b == b->c->root);
...	...	@@ -1019,14 +1017,6 @@
1019	1017	cancel_delayed_work(&b->work);
1020	1018
1021	1019	mutex_lock(&b->c->bucket_lock);
1022		-
1023		- for (i = 0; i < KEY_PTRS(&b->key); i++) {
1024		- BUG_ON(atomic_read(&PTR_BUCKET(b->c, &b->key, i)->pin));
1025		-
1026		- bch_inc_gen(PTR_CACHE(b->c, &b->key, i),
1027		- PTR_BUCKET(b->c, &b->key, i));
1028		- }
1029		-
1030	1020	bch_bucket_free(b->c, &b->key);
1031	1021	mca_bucket_free(b);
1032	1022	mutex_unlock(&b->c->bucket_lock);
1033	1023
1034	1024
...	...	@@ -1086,16 +1076,19 @@
1086	1076	{
1087	1077	unsigned i;
1088	1078
	1079	+ mutex_lock(&b->c->bucket_lock);
	1080	+
	1081	+ atomic_inc(&b->c->prio_blocked);
	1082	+
1089	1083	bkey_copy(k, &b->key);
1090	1084	bkey_copy_key(k, &ZERO_KEY);
1091	1085
1092		- for (i = 0; i < KEY_PTRS(k); i++) {
1093		- uint8_t g = PTR_BUCKET(b->c, k, i)->gen + 1;
	1086	+ for (i = 0; i < KEY_PTRS(k); i++)
	1087	+ SET_PTR_GEN(k, i,
	1088	+ bch_inc_gen(PTR_CACHE(b->c, &b->key, i),
	1089	+ PTR_BUCKET(b->c, &b->key, i)));
1094	1090
1095		- SET_PTR_GEN(k, i, g);
1096		- }
1097		-
1098		- atomic_inc(&b->c->prio_blocked);
	1091	+ mutex_unlock(&b->c->bucket_lock);
1099	1092	}
1100	1093
1101	1094	static int btree_check_reserve(struct btree b, struct btree_op op)
...	...	@@ -1342,6 +1335,13 @@
1342	1335	bch_keylist_add(keylist, &new_nodes[i]->key);
1343	1336	}
1344	1337
	1338	+ closure_sync(&cl);
	1339	+
	1340	+ /* We emptied out this node */
	1341	+ BUG_ON(btree_bset_first(new_nodes[0])->keys);
	1342	+ btree_node_free(new_nodes[0]);
	1343	+ rw_unlock(true, new_nodes[0]);
	1344	+
1345	1345	for (i = 0; i < nodes; i++) {
1346	1346	if (__bch_keylist_realloc(keylist, bkey_u64s(&r[i].b->key)))
1347	1347	goto out_nocoalesce;
1348	1348
...	...	@@ -1350,13 +1350,9 @@
1350	1350	bch_keylist_push(keylist);
1351	1351	}
1352	1352
1353		- /* We emptied out this node */
1354		- BUG_ON(btree_bset_first(new_nodes[0])->keys);
1355		- btree_node_free(new_nodes[0]);
1356		- rw_unlock(true, new_nodes[0]);
	1353	+ bch_btree_insert_node(b, op, keylist, NULL, NULL);
	1354	+ BUG_ON(!bch_keylist_empty(keylist));
1357	1355
1358		- closure_sync(&cl);
1359		-
1360	1356	for (i = 0; i < nodes; i++) {
1361	1357	btree_node_free(r[i].b);
1362	1358	rw_unlock(true, r[i].b);
...	...	@@ -1364,9 +1360,6 @@
1364	1360	r[i].b = new_nodes[i];
1365	1361	}
1366	1362
1367		- bch_btree_insert_node(b, op, keylist, NULL, NULL);
1368		- BUG_ON(!bch_keylist_empty(keylist));
1369		-
1370	1363	memmove(r, r + 1, sizeof(r[0]) * (nodes - 1));
1371	1364	r[nodes - 1].b = ERR_PTR(-EINTR);
1372	1365
1373	1366
...	...	@@ -1456,12 +1449,11 @@
1456	1449	keys.top);
1457	1450	bch_keylist_push(&keys);
1458	1451
1459		- btree_node_free(last->b);
1460		-
1461	1452	bch_btree_insert_node(b, op, &keys,
1462	1453	NULL, NULL);
1463	1454	BUG_ON(!bch_keylist_empty(&keys));
1464	1455
	1456	+ btree_node_free(last->b);
1465	1457	rw_unlock(true, last->b);
1466	1458	last->b = n;
1467	1459
1468	1460
1469	1461
1470	1462
...	...	@@ -1924,26 +1916,21 @@
1924	1916	closure_sync(&cl);
1925	1917	bch_btree_set_root(n3);
1926	1918	rw_unlock(true, n3);
1927		-
1928		- btree_node_free(b);
1929	1919	} else if (!b->parent) {
1930	1920	/* Root filled up but didn't need to be split */
1931	1921	closure_sync(&cl);
1932	1922	bch_btree_set_root(n1);
1933		-
1934		- btree_node_free(b);
1935	1923	} else {
1936	1924	/* Split a non root node */
1937	1925	closure_sync(&cl);
1938	1926	make_btree_freeing_key(b, parent_keys.top);
1939	1927	bch_keylist_push(&parent_keys);
1940	1928
1941		- btree_node_free(b);
1942		-
1943	1929	bch_btree_insert_node(b->parent, op, &parent_keys, NULL, NULL);
1944	1930	BUG_ON(!bch_keylist_empty(&parent_keys));
1945	1931	}
1946	1932
	1933	+ btree_node_free(b);
1947	1934	rw_unlock(true, n1);
1948	1935
1949	1936	bch_time_stats_update(&b->c->btree_split_time, start_time);