Change the signature of the space_map->new_block() method, introducing

a span_iterator abstraction.

Rename sm_transactional -> sm_careful_alloc.  Still not happy with the name.

Fix failing test.

Makefile.in

@@ -35,7 +35,7 @@ SOURCE=\
 	persistent-data/space_map.cc \
 	persistent-data/space_map_disk.cc \
 	persistent-data/space_map_recursive.cc \
-	persistent-data/space_map_transactional.cc \
+	persistent-data/space_map_careful_alloc.cc \
 	persistent-data/transaction_manager.cc \
 	thin-provisioning/human_readable_format.cc \
 	thin-provisioning/metadata.cc \
@@ -114,7 +114,7 @@ THIN_CHECK_SOURCE=\
 	persistent-data/space_map.cc \
 	persistent-data/space_map_disk.cc \
 	persistent-data/space_map_recursive.cc \
-	persistent-data/space_map_transactional.cc \
+	persistent-data/space_map_careful_alloc.cc \
 	persistent-data/transaction_manager.cc \
 	thin-provisioning/metadata.cc \
 	thin-provisioning/metadata_checker.cc \

persistent-data/space_map.h

@@ -24,12 +24,15 @@
 
 #include <boost/shared_ptr.hpp>
 #include <boost/optional.hpp>
+#include <functional>
 
 //----------------------------------------------------------------
 
 namespace persistent_data {
 	typedef uint32_t ref_t;
 
+	// FIXME: document these methods
+
 	class space_map {
 	public:
 		typedef boost::shared_ptr<space_map> ptr;
@@ -50,8 +53,40 @@ namespace persistent_data {
 		// searched.
 		typedef boost::optional<block_address> maybe_block;
 
-		virtual maybe_block new_block() = 0;
+		typedef std::pair<block_address, block_address> span;
-		virtual maybe_block new_block(block_address begin, block_address end) = 0;
+		typedef boost::optional<span> maybe_span;
+
+		struct span_iterator {
+			typedef boost::optional<span> maybe_span;
+
+			virtual maybe_span first() = 0;
+			virtual maybe_span next() = 0;
+		};
+
+		struct single_span_iterator : public span_iterator {
+			single_span_iterator(span const &s)
+			  : s_(s) {
+			}
+
+			virtual maybe_span first() {
+				return maybe_span(s_);
+			}
+
+			virtual maybe_span next() {
+				return maybe_span();
+			}
+
+		private:
+			span s_;
+		};
+
+		// deliberately not virtual
+		maybe_block new_block() {
+			single_span_iterator it(span(0, get_nr_blocks()));
+			return new_block(it);
+		}
+
+		virtual maybe_block new_block(span_iterator &it) = 0;
 
 		virtual bool count_possibly_greater_than_one(block_address b) const = 0;
 
@@ -64,7 +99,7 @@ namespace persistent_data {
 		};
 
 		virtual void iterate(iterator &it) const {
-			throw std::runtime_error("not implemented");
+			throw std::runtime_error("iterate() not implemented");
 		}
 	};
 
@@ -81,9 +116,10 @@ namespace persistent_data {
 		typedef boost::shared_ptr<checked_space_map> ptr;
 
 		virtual void check(block_counter &counter) const {
-			throw std::runtime_error("not implemented");
+			throw std::runtime_error("'check' not implemented");
 		}
 
+		// FIXME: should this be in the base space_map class?
 		virtual ptr clone() const = 0;
 	};
 

persistent-data/space_map_careful_alloc.cc

@@ -0,0 +1,189 @@
+// Copyright (C) 2011 Red Hat, Inc. All rights reserved.
+//
+// This file is part of the thin-provisioning-tools source.
+//
+// thin-provisioning-tools is free software: you can redistribute it
+// and/or modify it under the terms of the GNU General Public License
+// as published by the Free Software Foundation, either version 3 of
+// the License, or (at your option) any later version.
+//
+// thin-provisioning-tools is distributed in the hope that it will be
+// useful, but WITHOUT ANY WARRANTY; without even the implied warranty
+// of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with thin-provisioning-tools.  If not, see
+// <http://www.gnu.org/licenses/>.
+
+#include "persistent-data/space_map_careful_alloc.h"
+
+#include <set>
+
+//----------------------------------------------------------------
+
+namespace {
+	class sm_careful_alloc : public checked_space_map {
+	private:
+		typedef set<block_address> block_set;
+
+	public:
+		typedef shared_ptr<sm_careful_alloc> ptr;
+
+		sm_careful_alloc(checked_space_map::ptr sm)
+			: sm_(sm) {
+		}
+
+		virtual block_address get_nr_blocks() const {
+			return sm_->get_nr_blocks();
+		}
+
+		virtual block_address get_nr_free() const {
+			return sm_->get_nr_free();
+		}
+
+		virtual ref_t get_count(block_address b) const {
+			return sm_->get_count(b);
+		}
+
+		virtual void set_count(block_address b, ref_t c) {
+			if (!c && sm_->get_count(b))
+				mark_freed(b);
+
+			sm_->set_count(b, c);
+		}
+
+		virtual void commit() {
+			sm_->commit();
+			clear_freed();
+		}
+
+		virtual void inc(block_address b) {
+			if (was_freed(b))
+				throw runtime_error("inc of block freed within current transaction");
+
+			sm_->inc(b);
+		}
+
+		virtual void dec(block_address b) {
+			sm_->dec(b);
+
+			if (!sm_->get_count(b))
+				mark_freed(b);
+		}
+
+		// FIXME: rewrite with tests using the run_list stuff.
+		class no_freed_blocks_iterator : public span_iterator {
+		public:
+			no_freed_blocks_iterator(span_iterator &sub_it,
+						 block_set const &freed_blocks)
+				: sub_it_(sub_it),
+				  freed_blocks_(freed_blocks) {
+			}
+
+			virtual maybe_span first() {
+				current_span_ = sub_it_.first();
+
+				if (current_span_)
+					current_begin_ = current_span_->first;
+
+				return next();
+			}
+
+			virtual maybe_span next() {
+				if (!current_span_)
+					return current_span_;
+
+				if (current_begin_ == current_span_->second) {
+					current_span_ = sub_it_.next();
+					if (!current_span_)
+						return current_span_;
+
+					current_begin_ = current_span_->first;
+				}
+
+				// FIXME: slow
+				while (current_begin_ != current_span_->second &&
+				       freed_blocks_.count(current_begin_))
+					current_begin_++;
+
+				block_address b = current_begin_;
+
+				// FIXME: factor out common code
+				while (current_begin_ != current_span_->second &&
+				       !freed_blocks_.count(current_begin_))
+					current_begin_++;
+
+				block_address e = current_begin_;
+
+				return maybe_span(span(b, e));
+			}
+
+		private:
+			span_iterator &sub_it_;
+			block_set const &freed_blocks_;
+			maybe_span current_span_;
+			block_address current_begin_;
+		};
+
+		virtual maybe_block new_block(span_iterator &it) {
+			no_freed_blocks_iterator filtered_it(it, freed_blocks_);
+			return sm_->new_block(filtered_it);
+		}
+
+		virtual bool count_possibly_greater_than_one(block_address b) const {
+			return sm_->count_possibly_greater_than_one(b);
+		}
+
+		virtual void extend(block_address extra_blocks) {
+			return sm_->extend(extra_blocks);
+		}
+
+		virtual void iterate(iterator &it) const {
+			sm_->iterate(it);
+		}
+
+		virtual size_t root_size() const {
+			return sm_->root_size();
+		}
+
+		virtual void copy_root(void *dest, size_t len) const {
+			return sm_->copy_root(dest, len);
+		}
+
+		virtual void check(block_counter &counter) const {
+			return sm_->check(counter);
+		}
+
+		virtual checked_space_map::ptr clone() const {
+			return checked_space_map::ptr(new sm_careful_alloc(sm_));
+		}
+
+	private:
+		void clear_freed() {
+			freed_blocks_.clear();
+		}
+
+		void mark_freed(block_address b) {
+			freed_blocks_.insert(b);
+		}
+
+		bool was_freed(block_address b) const {
+			return freed_blocks_.count(b) > 0;
+		}
+
+		checked_space_map::ptr sm_;
+		block_set freed_blocks_;
+	};
+}
+
+//----------------------------------------------------------------
+
+checked_space_map::ptr
+persistent_data::create_careful_alloc_sm(checked_space_map::ptr sm)
+{
+	return checked_space_map::ptr(new sm_careful_alloc(sm));
+}
+
+
+//----------------------------------------------------------------

persistent-data/space_map_careful_alloc.h

@@ -16,20 +16,18 @@
 // with thin-provisioning-tools.  If not, see
 // <http://www.gnu.org/licenses/>.
 
-#ifndef SPACE_MAP_TRANSACTIONAL_H
+#ifndef SPACE_MAP_CAREFUL_ALLOC_H
-#define SPACE_MAP_TRANSACTIONAL_H
+#define SPACE_MAP_CAREFUL_ALLOC_H
 
 #include "space_map.h"
 
 //----------------------------------------------------------------
 
 namespace persistent_data {
-
-	// FIXME: change name 'transactional' is so vague.
-
 	// This space map ensures no blocks are allocated which have been
-	// freed within the current transaction.
+	// freed within the current transaction.  This is a common
-	checked_space_map::ptr create_transactional_sm(checked_space_map::ptr sm);
+	// requirement when we want resilience to crashes.
+	checked_space_map::ptr create_careful_alloc_sm(checked_space_map::ptr sm);
 }
 
 //----------------------------------------------------------------

persistent-data/space_map_core.h

@@ -72,17 +72,19 @@ namespace persistent_data {
 				nr_free_++;
 		}
 
-		maybe_block new_block() {
+		maybe_block new_block(span_iterator &it) {
-			return new_block(0, counts_.size());
+			for (maybe_span ms = it.first(); ms; ms = it.next()) {
-		}
+				for (block_address b = ms->first; b < ms->second; b++) {
+					if (b >= counts_.size())
+						throw std::runtime_error("block out of bounds");
 
-		maybe_block new_block(block_address begin, block_address end) {
+					if (!counts_[b]) {
-			for (block_address i = begin; i < std::min<block_address>(end, counts_.size()); i++)
+						counts_[b] = 1;
-				if (counts_[i] == 0) {
+						nr_free_--;
-					counts_[i] = 1;
+						return maybe_block(b);
-					nr_free_--;
+					}
-					return i;
 				}
+			}
 
 			return maybe_block();
 		}
@@ -92,7 +94,7 @@ namespace persistent_data {
 		}
 
 		void extend(block_address extra_blocks) {
-			throw std::runtime_error("not implemented");
+			throw std::runtime_error("'extend' not implemented");
 		}
 
 		// FIXME: meaningless, but this class is only used for testing
@@ -102,7 +104,7 @@ namespace persistent_data {
 
 		// FIXME: meaningless, but this class is only used for testing
 		virtual void copy_root(void *dest, size_t len) const {
-			throw std::runtime_error("not implemented");
+			throw std::runtime_error("'copy root' not implemented");
 		}
 
 		checked_space_map::ptr clone() const {

persistent-data/space_map_disk.cc

@@ -23,7 +23,7 @@
 #include "math_utils.h"
 #include "space_map_disk_structures.h"
 #include "space_map_recursive.h"
-#include "space_map_transactional.h"
+#include "space_map_careful_alloc.h"
 #include "transaction_manager.h"
 
 using namespace boost;
@@ -309,31 +309,31 @@ namespace {
 			set_count(b, old - 1);
 		}
 
-		maybe_block new_block() {
+		// FIXME: keep track of the lowest free block so we
-			// FIXME: keep track of the lowest free block so we
+		// can start searching from a suitable place.
-			// can start searching from a suitable place.
+		maybe_block new_block(span_iterator &it) {
-			return new_block(0, nr_blocks_);
+			for (maybe_span ms = it.first(); ms; ms = it.next()) {
-		}
+				block_address begin = ms->first;
+				block_address end = ms->second;
 
-		maybe_block new_block(block_address begin, block_address end) {
+				block_address begin_index = begin / ENTRIES_PER_BLOCK;
-			block_address begin_index = begin / ENTRIES_PER_BLOCK;
+				block_address end_index = div_up<block_address>(end, ENTRIES_PER_BLOCK);
-			block_address end_index = div_up<block_address>(end, ENTRIES_PER_BLOCK);
 
-			for (block_address index = begin_index; index < end_index; index++) {
+				for (block_address index = begin_index; index < end_index; index++) {
-				index_entry ie = indexes_->find_ie(index);
+					index_entry ie = indexes_->find_ie(index);
 
-				bitmap bm(tm_, ie);
+					bitmap bm(tm_, ie);
-				unsigned bit_begin = (index == begin_index) ? (begin % ENTRIES_PER_BLOCK) : 0;
+					unsigned bit_begin = (index == begin_index) ? (begin % ENTRIES_PER_BLOCK) : 0;
-				unsigned bit_end = (index == end_index - 1) ? (end % ENTRIES_PER_BLOCK) : ENTRIES_PER_BLOCK;
+					unsigned bit_end = (index == end_index - 1) ? (end % ENTRIES_PER_BLOCK) : ENTRIES_PER_BLOCK;
 
-				optional<unsigned> maybe_b = bm.find_free(bit_begin, bit_end);
+					optional<unsigned> maybe_b = bm.find_free(bit_begin, bit_end);
-				if (maybe_b) {
+					if (maybe_b) {
-					block_address b = *maybe_b;
+						indexes_->save_ie(index, bm.get_ie());
-					indexes_->save_ie(index, bm.get_ie());
+						nr_allocated_++;
-					nr_allocated_++;
+						block_address b = (index * ENTRIES_PER_BLOCK) + *maybe_b;
-					b = (index * ENTRIES_PER_BLOCK) + b;
+						assert(get_count(b) == 1);
-					assert(get_count(b) == 1);
+						return b;
-					return b;
+					}
 				}
 			}
 
@@ -705,7 +705,7 @@ persistent_data::create_metadata_sm(transaction_manager::ptr tm, block_address n
 	checked_space_map::ptr sm(new sm_disk(store, tm));
 	sm->extend(nr_blocks);
 	sm->commit();
-	return create_transactional_sm(
+	return create_careful_alloc_sm(
 		create_recursive_sm(sm));
 }
 
@@ -719,7 +719,7 @@ persistent_data::open_metadata_sm(transaction_manager::ptr tm, void *root)
 	sm_root_traits::unpack(d, v);
 	block_address nr_indexes = div_up<block_address>(v.nr_blocks_, ENTRIES_PER_BLOCK);
 	index_store::ptr store(new metadata_index_store(tm, v.bitmap_root_, nr_indexes));
-	return create_transactional_sm(
+	return create_careful_alloc_sm(
 		create_recursive_sm(
 			checked_space_map::ptr(new sm_disk(store, tm, v))));
 }

persistent-data/space_map_recursive.cc

@@ -103,19 +103,11 @@ namespace {
 			}
 		}
 
-		// new_block must not recurse.
+		virtual maybe_block
-		virtual boost::optional<block_address>
+		new_block(span_iterator &it) {
-		new_block() {
+			cant_recurse("new_block()");
-			cant_recurse("new_block");
 			recursing_lock lock(*this);
-			return sm_->new_block();
+			return sm_->new_block(it);
-		}
-
-		virtual boost::optional<block_address>
-		new_block(block_address begin, block_address end) {
-			cant_recurse("new_block(range)");
-			recursing_lock lock(*this);
-			return sm_->new_block(begin, end);
 		}
 
 		virtual bool count_possibly_greater_than_one(block_address b) const {

persistent-data/space_map_transactional.cc

@@ -1,131 +0,0 @@
-// Copyright (C) 2011 Red Hat, Inc. All rights reserved.
-//
-// This file is part of the thin-provisioning-tools source.
-//
-// thin-provisioning-tools is free software: you can redistribute it
-// and/or modify it under the terms of the GNU General Public License
-// as published by the Free Software Foundation, either version 3 of
-// the License, or (at your option) any later version.
-//
-// thin-provisioning-tools is distributed in the hope that it will be
-// useful, but WITHOUT ANY WARRANTY; without even the implied warranty
-// of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License along
-// with thin-provisioning-tools.  If not, see
-// <http://www.gnu.org/licenses/>.
-
-#include "persistent-data/space_map_transactional.h"
-
-//----------------------------------------------------------------
-
-namespace {
-	class sm_transactional : public checked_space_map {
-	public:
-		typedef shared_ptr<sm_transactional> ptr;
-
-		sm_transactional(checked_space_map::ptr sm)
-			: sm_(sm),
-			  committed_(sm_->clone()),
-			  allocated_(0),
-			  search_start_(0) {
-		}
-
-		virtual block_address get_nr_blocks() const {
-			return committed_->get_nr_blocks();
-		}
-
-		virtual block_address get_nr_free() const {
-			return committed_->get_nr_free() - allocated_;
-		}
-
-		virtual ref_t get_count(block_address b) const {
-			return sm_->get_count(b);
-		}
-
-		virtual void set_count(block_address b, ref_t c) {
-			sm_->set_count(b, c);
-		}
-
-		virtual void commit() {
-			sm_->commit();
-			committed_ = sm_->clone();
-			allocated_ = 0;
-			search_start_ = 0;
-		}
-
-		virtual void inc(block_address b) {
-			// FIXME: this may do an implicit allocation, so
-			// search_start_ and allocated_ will be wrong.
-			sm_->inc(b);
-		}
-
-		virtual void dec(block_address b) {
-			sm_->dec(b);
-		}
-
-		virtual maybe_block new_block() {
-			return new_block(0, sm_->get_nr_blocks());
-		}
-
-		virtual maybe_block new_block(block_address begin, block_address end) {
-			if (end <= search_start_)
-				return maybe_block();
-
-			maybe_block mb = committed_->new_block(max(search_start_, begin), end);
-			if (mb) {
-				allocated_++;
-				search_start_ = *mb + 1;
-			} else
-				search_start_ = end;
-
-			return mb;
-		}
-
-		virtual bool count_possibly_greater_than_one(block_address b) const {
-			return sm_->count_possibly_greater_than_one(b);
-		}
-
-		virtual void extend(block_address extra_blocks) {
-			return sm_->extend(extra_blocks);
-		}
-
-		virtual void iterate(iterator &it) const {
-			sm_->iterate(it);
-		}
-
-		virtual size_t root_size() const {
-			return sm_->root_size();
-		}
-
-		virtual void copy_root(void *dest, size_t len) const {
-			return sm_->copy_root(dest, len);
-		}
-
-		virtual void check(block_counter &counter) const {
-			return sm_->check(counter);
-		}
-
-		virtual checked_space_map::ptr clone() const {
-			return checked_space_map::ptr(new sm_transactional(sm_));
-		}
-
-	private:
-		checked_space_map::ptr sm_;
-		checked_space_map::ptr committed_;
-		block_address allocated_;
-		block_address search_start_;
-	};
-}
-
-//----------------------------------------------------------------
-
-checked_space_map::ptr
-persistent_data::create_transactional_sm(checked_space_map::ptr sm)
-{
-	return checked_space_map::ptr(new sm_transactional(sm));
-}
-
-
-//----------------------------------------------------------------

unit-tests/space_map_t.cc

@@ -18,7 +18,7 @@
 
 #include "persistent-data/space_map_disk.h"
 #include "persistent-data/space_map_core.h"
-#include "persistent-data/space_map_transactional.h"
+#include "persistent-data/space_map_careful_alloc.h"
 
 #define BOOST_TEST_MODULE SpaceMapDiskTests
 #include <boost/test/included/unit_test.hpp>
@@ -52,11 +52,11 @@ namespace {
 		}
 	};
 
-	class sm_transactional_creator {
+	class sm_careful_alloc_creator {
 	public:
 		static space_map::ptr
 		create() {
-			return create_transactional_sm(
+			return create_careful_alloc_sm(
 				checked_space_map::ptr(
 					new core_map(NR_BLOCKS)));
 		}
@@ -259,9 +259,9 @@ BOOST_AUTO_TEST_CASE(test_sm_core)
 	do_tests<sm_core_creator>(space_map_tests);
 }
 
-BOOST_AUTO_TEST_CASE(test_sm_transactional)
+BOOST_AUTO_TEST_CASE(test_sm_careful_alloc)
 {
-	do_tests<sm_transactional_creator>(space_map_tests);
+	do_tests<sm_careful_alloc_creator>(space_map_tests);
 }
 
 BOOST_AUTO_TEST_CASE(test_sm_disk)