Skip to contentSkip to conversation

dm-cache watermarks

PATCH In review Steven Wilton 2 participants 3 messages Mar 6, 2016 – Mar 8, 2016 Device mapper

lore
Share & tools
lore

Steven Wilton

author
#0 lore raw

PATCH v3 1/1 dm-cache watermarks

I realised that I over-thought the original logic. The attached patch is much simpler - write back dirty cache entries at the migration threshold rate when we are over the low watermark, and we write back as fast as possible when we go over the high watermark.

The current behaviour can be replicated by setting the high watermark to 100, and the low watermark to 0.

regards

Steven

drivers/md/dm-cache-target.c +38 -2
index 7dab682..7b8b4a3 100644
@@ -221,6 +221,8 @@ struct cache {
 	struct list_head completed_migrations;
 	struct list_head need_commit_migrations;
 	sector_t migration_threshold;
+	unsigned long writeback_high_watermark;
+	unsigned long writeback_low_watermark;
 	wait_queue_head_t migration_wait;
 	atomic_t nr_allocated_migrations;
 

@@ -1433,6 +1435,20 @@ static bool spare_migration_bandwidth(struct cache *cache)
 	return current_volume < cache->migration_threshold;
 }
 
+static bool writeback_wanted(struct cache *cache)
+{
+	unsigned long dirty_pct = (atomic_read(&cache->nr_dirty) * 100) / from_cblock(cache->cache_size);
+
+	if (dirty_pct >= cache->writeback_high_watermark) {
+		return true;
+
+	} else if (dirty_pct >= cache->writeback_low_watermark && spare_migration_bandwidth(cache)) {
+		return true;
+	}
+
+	return false;
+}
+
 static void inc_hit_counter(struct cache *cache, struct bio *bio)
 {
 	atomic_inc(bio_data_dir(bio) == READ ?

@@ -1673,7 +1689,7 @@ static void writeback_some_dirty_blocks(struct cache *cache)
 
 	memset(&structs, 0, sizeof(structs));
 
-	while (spare_migration_bandwidth(cache)) {
+	while (writeback_wanted(cache)) {
 		if (prealloc_data_structs(cache, &structs))
 			break;
 

@@ -2239,6 +2255,22 @@ static int process_config_option(struct cache *cache, const char *key, const cha
 		return 0;
 	}
 
+	if (!strcasecmp(key, "writeback_high_watermark")) {
+		if (kstrtoul(value, 10, &tmp))
+			return -EINVAL;
+
+		cache->writeback_high_watermark = tmp;
+		return 0;
+	}
+
+	if (!strcasecmp(key, "writeback_low_watermark")) {
+		if (kstrtoul(value, 10, &tmp))
+			return -EINVAL;
+
+		cache->writeback_low_watermark = tmp;
+		return 0;
+	}
+
 	return NOT_CORE_OPTION;
 }
 

@@ -2332,6 +2364,8 @@ static void set_cache_size(struct cache *cache, dm_cblock_t size)
 }
 
 #define DEFAULT_MIGRATION_THRESHOLD 2048
+#define DEFAULT_WRITEBACK_HIGH_WATERMARK 40
+#define DEFAULT_WRITEBACK_LOW_WATERMARK 30
 
 static int cache_create(struct cache_args *ca, struct cache **result)
 {

@@ -2397,6 +2431,8 @@ static int cache_create(struct cache_args *ca, struct cache **result)
 
 	cache->policy_nr_args = ca->policy_argc;
 	cache->migration_threshold = DEFAULT_MIGRATION_THRESHOLD;
+	cache->writeback_high_watermark = DEFAULT_WRITEBACK_HIGH_WATERMARK;
+	cache->writeback_low_watermark = DEFAULT_WRITEBACK_LOW_WATERMARK;
 
 	r = set_config_values(cache, ca->policy_argc, ca->policy_argv);
 	if (r) {

@@ -3103,7 +3139,7 @@ static void cache_status(struct dm_target *ti, status_type_t type,
 			goto err;
 		}
 
-		DMEMIT("2 migration_threshold %llu ", (unsigned long long) cache->migration_threshold);
+		DMEMIT("6 migration_threshold %llu writeback_high_watermark %llu writeback_low_watermark %llu ", (unsigned long long) cache->migration_threshold, (unsigned long long) cache->writeback_high_watermark, (unsigned long long) cache->writeback_low_watermark);
 
 		DMEMIT("%s ", dm_cache_policy_get_name(cache->policy));
 		if (sz < maxlen) {
#1 lore raw
Quoted 11 lines · Steven Wilton

I realised that I over-thought the original logic. The attached patch is much simpler - write back dirty cache entries at the migration threshold rate when we are over the low watermark, and we write back as fast as possible when we go over the high watermark.

The current behaviour can be replicated by setting the high watermark to 100, and the low watermark to 0.

regards

Steven

...

drivers/md/dm-cache-target.c static void set_cache_size(struct cache *cache, dm_cblock_t size) · line 2369

Quoted 11 lines
drivers/md/dm-cache-target.c +2 -0
index 7dab682..7b8b4a3 100644
@@ -2332,6 +2364,8 @@ static void set_cache_size(struct cache *cache, dm_cblock_t size)
 }
 
 #define DEFAULT_MIGRATION_THRESHOLD 2048
+#define DEFAULT_WRITEBACK_HIGH_WATERMARK 40
+#define DEFAULT_WRITEBACK_LOW_WATERMARK 30

These should default to current behavior (100 and 0 respectively, like you mentioned above).

This change needs an accompanying Documentation update. It also needs a proper path header to accurately convey the scope of the change.

Please post v4 with these changes and then I'll take a closer look (as I'm sure Joe will). But please be aware that in general we want less knobs, not more.

Thanks,
Mike

author
#2 lore raw
Quoted 20 lines · Mike Snitzer
Quoted 11 lines · Steven Wilton

I realised that I over-thought the original logic. The attached patch is much simpler - write back dirty cache entries at the migration threshold rate when we are over the low watermark, and we write back as fast as possible when we go over the high watermark.

The current behaviour can be replicated by setting the high watermark to 100, and the low watermark to 0.

regards

Steven

These should default to current behavior (100 and 0 respectively, like you mentioned above).

This change needs an accompanying Documentation update. It also needs a proper path header to accurately convey the scope of the change.

Quoted 8 lines

Please post v4 with these changes and then I'll take a closer look (as I'm sure Joe will). But please be aware that in general we want less knobs, not more.

Thanks,
Mike

Thanks for getting back to me on this. I have just submitted the patch from git (after reading up on how). Can you please confirm that the new patch is in the correct format?

If you want less knobs, then the high watermark may not be much use, and I can remove it from the patch. The initial request was mainly for a review to make sure I have not done anything obviously wrong that will lead to data corruption.

I am happy to run some benchmarks on before and after to quantify any performance gains.

I started looking into this because I noticed an increase in both disk busy time and data being written to the origin device after I enabled dm-cache on a production system. The overall performance feels better even though the origin device is busier, but I am most interested to see what effect the low watermark has on this system.

I have also had a thought on the stats emitted by dm-cache, and think that it would be useful to export the number of write hits on already dirty blocks so we can see how efficient the writeback has been at avoiding writes to the origin. What is your view on changing the output format here? If the output format cannot be changed, would you consider re-defining a write hit in writeback mode as a hit on an already dirty block, since this is the only scenario where I/O is avoided on the origin device?

regards

Steve