[PATCH v3] bcache: Separate bch_moving_gc() from bch_btree_gc()

[PATCH v3] bcache: Separate bch_moving_gc() from bch_btree_gc()

[Mingzhe Zou]

From: Mingzhe Zou <zoumingzhe@qq.com>

Moving gc uses cache->heap to defragment disk. Unlike btree gc,
moving gc only takes up part of the disk bandwidth.

The number of heap is constant. However, the buckets released by
each moving gc is limited. So bch_moving_gc() needs to be called
multiple times.

If bch_gc_thread() always calls bch_btree_gc(), it will block
the IO request.This patch allows bch_gc_thread() to only call
bch_moving_gc() when there are many fragments.

Signed-off-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
---
 drivers/md/bcache/bcache.h   |  4 +-
 drivers/md/bcache/btree.c    | 73 ++++++++++++++++++++++++++++++++++--
 drivers/md/bcache/movinggc.c |  2 +
 3 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 5a79bb3c272f..155deff0ce05 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -461,7 +461,8 @@ struct cache {
 	 * until a gc finishes - otherwise we could pointlessly burn a ton of
 	 * cpu
 	 */
-	unsigned int		invalidate_needs_gc;
+	unsigned int		invalidate_needs_gc:1;
+	unsigned int		only_moving_gc:1;
 
 	bool			discard; /* Get rid of? */
 
@@ -629,6 +630,7 @@ struct cache_set {
 	struct gc_stat		gc_stats;
 	size_t			nbuckets;
 	size_t			avail_nbuckets;
+	size_t			fragment_nbuckets;
 
 	struct task_struct	*gc_thread;
 	/* Where in the btree gc currently is */
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index fd121a61f17c..b21192a47a57 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -88,6 +88,7 @@
  * Test module load/unload
  */
 
+#define COPY_GC_PERCENT		5
 #define MAX_NEED_GC		64
 #define MAX_SAVE_PRIO		72
 #define MAX_GC_TIMES		100
@@ -1726,6 +1727,7 @@ static void btree_gc_start(struct cache_set *c)
 
 	mutex_lock(&c->bucket_lock);
 
+	set_gc_sectors(c);
 	c->gc_mark_valid = 0;
 	c->gc_done = ZERO_KEY;
 
@@ -1846,8 +1848,58 @@ static void bch_btree_gc(struct cache_set *c)
 	memcpy(&c->gc_stats, &stats, sizeof(struct gc_stat));
 
 	trace_bcache_gc_end(c);
+}
+
+extern unsigned int bch_cutoff_writeback;
+extern unsigned int bch_cutoff_writeback_sync;
+
+static bool moving_gc_should_run(struct cache_set *c)
+{
+	struct bucket *b;
+	struct cache *ca = c->cache;
+	size_t moving_gc_threshold = ca->sb.bucket_size >> 2, frag_percent;
+	unsigned long used_buckets = 0, frag_buckets = 0, move_buckets = 0;
+	unsigned long dirty_sectors = 0, frag_sectors, used_sectors;
+
+	if (c->gc_stats.in_use > bch_cutoff_writeback_sync)
+		return true;
+
+	mutex_lock(&c->bucket_lock);
+	for_each_bucket(b, ca) {
+		if (GC_MARK(b) != GC_MARK_DIRTY)
+			continue;
+
+		used_buckets++;
+
+		used_sectors = GC_SECTORS_USED(b);
+		dirty_sectors += used_sectors;
+
+		if (used_sectors < ca->sb.bucket_size)
+			frag_buckets++;
+
+		if (used_sectors <= moving_gc_threshold)
+			move_buckets++;
+	}
+	mutex_unlock(&c->bucket_lock);
+
+	c->fragment_nbuckets = frag_buckets;
+
+	if (used_sectors < c->nbuckets * bch_cutoff_writeback / 100)
+		return false;
+
+	if (move_buckets > ca->heap.size)
+		return true;
+
+	frag_sectors = used_buckets * ca->sb.bucket_size - dirty_sectors;
+	frag_percent = div_u64(frag_sectors * 100, ca->sb.bucket_size * c->nbuckets);
+
+	if (frag_percent >= COPY_GC_PERCENT)
+		return true;
 
-	bch_moving_gc(c);
+	if (used_sectors > c->nbuckets * bch_cutoff_writeback_sync / 100)
+		return true;
+
+	return false;
 }
 
 static bool gc_should_run(struct cache_set *c)
@@ -1860,6 +1912,19 @@ static bool gc_should_run(struct cache_set *c)
 	if (atomic_read(&c->sectors_to_gc) < 0)
 		return true;
 
+	/*
+	 * Moving gc uses cache->heap to defragment disk. Unlike btree gc,
+	 * moving gc only takes up part of the disk bandwidth.
+	 * The number of heap is constant. However, the buckets released by
+	 * each moving gc is limited. So bch_moving_gc() needs to be called
+	 * multiple times. If bch_gc_thread() always calls bch_btree_gc(),
+	 * it will block the IO request.
+	 */
+	if (c->copy_gc_enabled && moving_gc_should_run(c)) {
+		ca->only_moving_gc = 1;
+		return true;
+	}
+
 	return false;
 }
 
@@ -1877,8 +1942,10 @@ static int bch_gc_thread(void *arg)
 		    test_bit(CACHE_SET_IO_DISABLE, &c->flags))
 			break;
 
-		set_gc_sectors(c);
-		bch_btree_gc(c);
+		if (!c->cache->only_moving_gc)
+			bch_btree_gc(c);
+
+		bch_moving_gc(c);
 	}
 
 	wait_for_kthread_stop();
diff --git a/drivers/md/bcache/movinggc.c b/drivers/md/bcache/movinggc.c
index 9f32901fdad1..04da088cefe8 100644
--- a/drivers/md/bcache/movinggc.c
+++ b/drivers/md/bcache/movinggc.c
@@ -200,6 +200,8 @@ void bch_moving_gc(struct cache_set *c)
 	struct bucket *b;
 	unsigned long sectors_to_move, reserve_sectors;
 
+	c->cache->only_moving_gc = 0;
+
 	if (!c->copy_gc_enabled)
 		return;
 
-- 
2.17.1.windows.2

Re: [PATCH v3] bcache: Separate bch_moving_gc() from bch_btree_gc()

[Dan Carpenter]

Hi Mingzhe,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mingzhe-Zou/bcache-Separate-bch_moving_gc-from-bch_btree_gc/20230629-194834
base:   linus/master
patch link:    https://lore.kernel.org/r/20230629114740.311-1-mingzhe.zou%40easystack.cn
patch subject: [PATCH v3] bcache: Separate bch_moving_gc() from bch_btree_gc()
config: x86_64-randconfig-m001-20230710 (https://download.01.org/0day-ci/archive/20230716/202307160359.0vHG3r40-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230716/202307160359.0vHG3r40-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202307160359.0vHG3r40-lkp@intel.com/

New smatch warnings:
drivers/md/bcache/btree.c:1887 moving_gc_should_run() error: uninitialized symbol 'used_sectors'.

Old smatch warnings:
drivers/md/bcache/btree.c:1535 btree_gc_rewrite_node() error: 'n' dereferencing possible ERR_PTR()
drivers/md/bcache/btree.c:1551 btree_gc_rewrite_node() error: 'n' dereferencing possible ERR_PTR()

vim +/used_sectors +1887 drivers/md/bcache/btree.c

4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1856  static bool moving_gc_should_run(struct cache_set *c)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1857  {
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1858  	struct bucket *b;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1859  	struct cache *ca = c->cache;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1860  	size_t moving_gc_threshold = ca->sb.bucket_size >> 2, frag_percent;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1861  	unsigned long used_buckets = 0, frag_buckets = 0, move_buckets = 0;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1862  	unsigned long dirty_sectors = 0, frag_sectors, used_sectors;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1863  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1864  	if (c->gc_stats.in_use > bch_cutoff_writeback_sync)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1865  		return true;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1866  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1867  	mutex_lock(&c->bucket_lock);
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1868  	for_each_bucket(b, ca) {
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1869  		if (GC_MARK(b) != GC_MARK_DIRTY)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1870  			continue;

Smatch is complaining that we might not enter the loop or there could be
no GC_MARK_DIRTY entries.

4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1871  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1872  		used_buckets++;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1873  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1874  		used_sectors = GC_SECTORS_USED(b);
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1875  		dirty_sectors += used_sectors;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1876  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1877  		if (used_sectors < ca->sb.bucket_size)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1878  			frag_buckets++;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1879  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1880  		if (used_sectors <= moving_gc_threshold)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1881  			move_buckets++;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1882  	}
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1883  	mutex_unlock(&c->bucket_lock);
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1884  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1885  	c->fragment_nbuckets = frag_buckets;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1886  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29 @1887  	if (used_sectors < c->nbuckets * bch_cutoff_writeback / 100)

It's sort of weird that we are using the used_sectors value from the
last MARK_DIRTY iteration through the loop.  Should it be used_buckets?

4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1888  		return false;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1889  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1890  	if (move_buckets > ca->heap.size)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1891  		return true;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1892  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1893  	frag_sectors = used_buckets * ca->sb.bucket_size - dirty_sectors;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1894  	frag_percent = div_u64(frag_sectors * 100, ca->sb.bucket_size * c->nbuckets);
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1895  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1896  	if (frag_percent >= COPY_GC_PERCENT)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1897  		return true;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1898  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1899  	if (used_sectors > c->nbuckets * bch_cutoff_writeback_sync / 100)
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1900  		return true;
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1901  
4b0cf76f1e36e7 Mingzhe Zou     2023-06-29  1902  	return false;
cafe563591446c Kent Overstreet 2013-03-23  1903  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki