[block-cache] remove BF_PREVIOUSLY_DIRTY and improve nr_dirty_ accounting.

block-cache/block_cache.cc

@@ -106,19 +106,15 @@ block_cache::complete_io(block &b, int result)
 	b.error_ = result;
 	b.clear_flags(BF_IO_PENDING);
 	nr_io_pending_--;
+	b.unlink();  // b is on the io_pending list
 
-	if (b.error_) {
+	if (b.error_)
-		b.unlink();
 		errored_.push_back(b);
 
-	} else {
+	else {
-		if (b.test_flags(BF_DIRTY)) {
+		if (b.test_flags(BF_DIRTY))
-			b.clear_flags(BF_DIRTY | BF_PREVIOUSLY_DIRTY);
+			b.clear_flags(BF_DIRTY);
-			nr_dirty_--;
+		link_block(b);
-		}
-
-		b.unlink();
-		clean_.push_back(b);
 	}
 }
 
@@ -136,7 +132,7 @@ block_cache::issue_low_level(block &b, enum io_iocb_cmd opcode, const char *desc
 	assert(!b.test_flags(BF_IO_PENDING));
 	b.set_flags(BF_IO_PENDING);
 	nr_io_pending_++;
-	b.unlink();
+	unlink_block(b);
 	io_pending_.push_back(b);
 
 	b.control_block_.aio_lio_opcode = opcode;
@@ -212,24 +208,31 @@ block_cache::wait_io()
  * Clean/dirty list management
  *--------------------------------------------------------------*/
 
-/*
+// Always use these two methods to ensure nr_dirty_ is correct.
- * We're using lru lists atm, but I think it would be worth
+void
- * experimenting with a multiqueue approach.
+block_cache::unlink_block(block &b)
- */
-block_cache::block_list &
-block_cache::__categorise(block &b)
 {
-	if (b.error_)
+	if (b.test_flags(BF_DIRTY))
-		return errored_;
+		nr_dirty_--;
 
-	return b.test_flags(BF_DIRTY) ? dirty_ : clean_;
+	b.unlink();
+}
+
+void
+block_cache::link_block(block &b)
+{
+	if (b.test_flags(BF_DIRTY)) {
+		dirty_.push_back(b);
+		nr_dirty_++;
+	} else
+		clean_.push_back(b);
 }
 
 void
 block_cache::hit(block &b)
 {
-	b.unlink();
+	unlink_block(b);
-	__categorise(b).push_back(b);
+	link_block(b);
 }
 
 /*----------------------------------------------------------------
@@ -299,13 +302,12 @@ block_cache::block *
 block_cache::find_unused_clean_block()
 {
 	for (block &b : clean_) {
-		if (b.ref_count_)
+		if (!b.ref_count_) {
-			continue;
+			unlink_block(b);
-
 			b.unlink_set();
-		b.unlink();
 			return &b;
 		}
+	}
 
 	return NULL;
 }
@@ -472,31 +474,46 @@ block_cache::block *
 block_cache::lookup_or_read_block(block_address index, unsigned flags,
 				  validator::ptr v)
 {
+	block *b = NULL;
 	auto it = block_set_.find(index, cmp_index());
 
 	if (it != block_set_.end()) {
-		if (it->test_flags(BF_IO_PENDING)) {
+		b = &(*it);
+
+		// FIXME: this is insufficient.  We need to also catch a read
+		// lock of a write locked block.  Ref count needs to distinguish.
+		if (b->ref_count_ && (flags & (GF_DIRTY | GF_ZERO))) {
+			std::ostringstream out;
+			out << "attempt to write lock block " << index << " concurrently";
+			throw std::runtime_error(out.str());
+		}
+
+		// FIXME: confusing names, hit, then explicit inc of hit/miss counter.
+		hit(*b);
+		if (b->test_flags(BF_IO_PENDING)) {
 			inc_miss_counter(flags);
-			wait_specific(*it);
+			wait_specific(*b);
 		} else
 			inc_hit_counter(flags);
 
+		unlink_block(*b);
+
 		if (flags & GF_ZERO)
 			zero_block(*it);
 		else {
-			if (it->v_.get() != v.get()) {
+			// has the validator changed?
-				if (it->test_flags(BF_DIRTY))
+			if (b->v_.get() != v.get()) {
-					it->v_->prepare(it->data_, it->index_);
+				if (b->test_flags(BF_DIRTY))
-				v->check(it->data_, it->index_);
+					b->v_->prepare(b->data_, b->index_);
+				v->check(b->data_, b->index_);
 			}
 		}
-		it->v_ = v;
+		b->v_ = v;
-		return &(*it);
 
 	} else {
 		inc_miss_counter(flags);
 
-		block *b = new_block(index);
+		b = new_block(index);
 		if (b) {
 			if (flags & GF_ZERO)
 				zero_block(*b);
@@ -504,13 +521,26 @@ block_cache::lookup_or_read_block(block_address index, unsigned flags,
 				issue_read(*b);
 				wait_specific(*b);
 				v->check(b->data_, b->index_);
+				unlink_block(*b);   // FIXME: what if it's on the error list?
 			}
 
 			b->v_ = v;
 		}
-
-		return (!b || b->error_) ? NULL : b;
 	}
+
+	if (b && !b->error_) {
+		if (flags & GF_BARRIER)
+			b->set_flags(BF_FLUSH);
+
+		if (flags & (GF_DIRTY | GF_ZERO))
+			b->set_flags(BF_DIRTY);
+
+		link_block(*b);
+		return b;
+	}
+
+	return NULL;
+
 }
 
 block_cache::block &
@@ -519,28 +549,11 @@ block_cache::get(block_address index, unsigned flags, validator::ptr v)
 	check_index(index);
 
 	block *b = lookup_or_read_block(index, flags, v);
-
 	if (b) {
-		if (b->ref_count_ && (flags & (GF_DIRTY | GF_ZERO))) {
-			std::ostringstream out;
-			out << "attempt to write lock block " << index << " concurrently";
-			throw std::runtime_error(out.str());
-		}
-
-		// FIXME: this gets called even for new blocks
-		hit(*b);
-
 		if (!b->ref_count_)
 			nr_locked_++;
-
 		b->ref_count_++;
 
-		if (flags & GF_BARRIER)
-			b->set_flags(BF_FLUSH);
-
-		if (flags & GF_DIRTY)
-			b->set_flags(BF_DIRTY);
-
 		return *b;
 	}
 
@@ -552,12 +565,26 @@ block_cache::get(block_address index, unsigned flags, validator::ptr v)
 void
 block_cache::preemptive_writeback()
 {
+	// FIXME: this ignores those blocks that are in the error state.  Track
+	// nr_clean instead?
 	unsigned nr_available = nr_cache_blocks_ - (nr_dirty_ - nr_io_pending_);
 	if (nr_available < (WRITEBACK_LOW_THRESHOLD_PERCENT * nr_cache_blocks_ / 100))
 		writeback((WRITEBACK_HIGH_THRESHOLD_PERCENT * nr_cache_blocks_ / 100) - nr_available);
 
 }
 
+bool
+block_cache::maybe_flush(block &b)
+{
+	if (b.test_flags(BF_FLUSH)) {
+		flush();
+		b.clear_flags(BF_FLUSH);
+		return true;
+	}
+
+	return false;
+}
+
 void
 block_cache::release(block_cache::block &b)
 {
@@ -565,24 +592,11 @@ block_cache::release(block_cache::block &b)
 
 	nr_locked_--;
 
-	if (b.test_flags(BF_FLUSH))
-		flush();
-
 	if (b.test_flags(BF_DIRTY)) {
-		if (!b.test_flags(BF_PREVIOUSLY_DIRTY)) {
+		if (!maybe_flush(b))
-			b.unlink();
-			dirty_.push_back(b);
-			nr_dirty_++;
-			b.set_flags(BF_PREVIOUSLY_DIRTY);
-		}
-
-		if (b.test_flags(BF_FLUSH))
-			flush();
-		else
 			preemptive_writeback();
-
+	} else
-		b.clear_flags(BF_FLUSH);
+		maybe_flush(b);
-	}
 }
 
 int

block-cache/block_cache.h

@@ -51,7 +51,6 @@ namespace bcache {
 			BF_IO_PENDING = (1 << 0),
 			BF_DIRTY = (1 << 1),
 			BF_FLUSH = (1 << 2),
-			BF_PREVIOUSLY_DIRTY = (1 << 3)
 		};
 
 		class block : private boost::noncopyable {
@@ -222,7 +221,8 @@ namespace bcache {
 		void issue_read(block &b);
 		void issue_write(block &b);
 		void wait_io();
-		block_list &__categorise(block &b);
+		void unlink_block(block &b);
+		void link_block(block &b);
 		void hit(block &b);
 		void wait_all();
 		void wait_specific(block &b);
@@ -238,6 +238,7 @@ namespace bcache {
 		void exit_free_list();
 
 		void preemptive_writeback();
+		bool maybe_flush(block_cache::block &b);
 		void release(block_cache::block &block);
 		void check_index(block_address index) const;