[PATCH 1/3] bcache: drop obsolete comments

[PATCH 1/3] bcache: drop obsolete comments

[Yaowei Bai]

Unused list was killed by commit 2531d9ee61fa ("bcache: Kill unused freelist")
but left these comments, let's drop them.

This patch doesn't introduce functional change.

Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
---
 drivers/md/bcache/alloc.c | 13 +++----------
 drivers/md/bcache/super.c |  3 ---
 2 files changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 6f77682..c22c260 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -33,13 +33,6 @@
  * If we've got discards enabled, that happens when a bucket moves from the
  * free_inc list to the free list.
  *
- * There is another freelist, because sometimes we have buckets that we know
- * have nothing pointing into them - these we can reuse without waiting for
- * priorities to be rewritten. These come from freed btree nodes and buckets
- * that garbage collection discovered no longer had valid keys pointing into
- * them (because they were overwritten). That's the unused list - buckets on the
- * unused list move to the free list, optionally being discarded in the process.
- *
  * It's also important to ensure that gens don't wrap around - with respect to
  * either the oldest gen in the btree or the gen on disk. This is quite
  * difficult to do in practice, but we explicitly guard against it anyways - if
@@ -323,9 +316,9 @@ static int bch_allocator_thread(void *arg)
 
 	while (1) {
 		/*
-		 * First, we pull buckets off of the unused and free_inc lists,
-		 * possibly issue discards to them, then we add the bucket to
-		 * the free list:
+		 * First, we pull buckets off of the free_inc list, possibly
+		 * issue discards to them, then we add the bucket to the free
+		 * list:
 		 */
 		while (1) {
 			long bucket;
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 26e374f..eba38aa 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -544,9 +544,6 @@ void bch_prio_write(struct cache *ca)
 	atomic_long_add(ca->sb.bucket_size * prio_buckets(ca),
 			&ca->meta_sectors_written);
 
-	//pr_debug("free %zu, free_inc %zu, unused %zu", fifo_used(&ca->free),
-	//	 fifo_used(&ca->free_inc), fifo_used(&ca->unused));
-
 	for (i = prio_buckets(ca) - 1; i >= 0; --i) {
 		long bucket;
 		struct prio_set *p = ca->disk_buckets;
-- 
1.8.3.1

[PATCH 2/3] bcache: use allocator reserves instead of watermarks

[Yaowei Bai]

Commit 78365411b344 ("bcache: Rework allocator reserves") introduced
allocator reserves and dropped watermarks, let's keep this consistent
to avoid confusing.

Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
---
 drivers/md/bcache/alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index c22c260..609df38 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -622,13 +622,13 @@ bool bch_alloc_sectors(struct cache_set *c,
 	spin_lock(&c->data_bucket_lock);
 
 	while (!(b = pick_data_bucket(c, k, write_point, &alloc.key))) {
-		unsigned int watermark = write_prio
+		unsigned int reserve = write_prio
 			? RESERVE_MOVINGGC
 			: RESERVE_NONE;
 
 		spin_unlock(&c->data_bucket_lock);
 
-		if (bch_bucket_alloc_set(c, watermark, &alloc.key, 1, wait))
+		if (bch_bucket_alloc_set(c, reserve, &alloc.key, 1, wait))
 			return false;
 
 		spin_lock(&c->data_bucket_lock);
-- 
1.8.3.1

[PATCH 3/3] bcache: count cache_available_percent accurately

[Yaowei Bai]

The interface cache_available_percent is used to indicate how
many buckets in percent are available to be used to cache data
at a specific moment. It should include the unused and clean
buckets which we get from bch_btree_gc_finish function:

	if (!GC_MARK(b) || GC_MARK(b) == GC_MARK_RECLAIMABLE)
		 c->avail_nbuckets++;

However currently in the allocation code we didn't distinguish
these available buckets with the metadata and dirty buckets, we
just decrease the c->avail_nbuckets everytime we allocate a bucket,
and correct it after a gc completes. With this, in a read-only
scenario, you can observe that cache_available_percent bounces,
it first go down to a number, like 95, and then bounces back to
100. It goes on and on, making users confused.

This patch fixes this problem by decreasing c->avail_nbuckets
only when allocate metadata and dirty buckets. With this patch,
cache_available_percent will always be accurate and avoid the
confusion.

Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
---
 drivers/md/bcache/alloc.c   | 10 +++++-----
 drivers/md/bcache/request.c |  9 ++++++++-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 609df38..dc7f6c2 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -443,17 +443,17 @@ long bch_bucket_alloc(struct cache *ca, unsigned int reserve, bool wait)
 		SET_GC_MARK(b, GC_MARK_METADATA);
 		SET_GC_MOVE(b, 0);
 		b->prio = BTREE_PRIO;
+
+		if (ca->set->avail_nbuckets > 0) {
+			ca->set->avail_nbuckets--;
+			bch_update_bucket_in_use(ca->set, &ca->set->gc_stats);
+		}
 	} else {
 		SET_GC_MARK(b, GC_MARK_RECLAIMABLE);
 		SET_GC_MOVE(b, 0);
 		b->prio = INITIAL_PRIO;
 	}
 
-	if (ca->set->avail_nbuckets > 0) {
-		ca->set->avail_nbuckets--;
-		bch_update_bucket_in_use(ca->set, &ca->set->gc_stats);
-	}
-
 	return r;
 }
 
diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 41adcd1..b69bd8d 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -244,9 +244,16 @@ static void bch_data_insert_start(struct closure *cl)
 		if (op->writeback) {
 			SET_KEY_DIRTY(k, true);
 
-			for (i = 0; i < KEY_PTRS(k); i++)
+			for (i = 0; i < KEY_PTRS(k); i++) {
 				SET_GC_MARK(PTR_BUCKET(op->c, k, i),
 					    GC_MARK_DIRTY);
+
+				if (op->c->avail_nbuckets > 0) {
+					op->c->avail_nbuckets--;
+					bch_update_bucket_in_use(op->c,
+								 &op->c->gc_stats);
+				}
+			}
 		}
 
 		SET_KEY_CSUM(k, op->csum);
-- 
1.8.3.1

Re: [PATCH 1/3] bcache: drop obsolete comments

[Coly Li]

On 2019/7/27 6:19 下午, Yaowei Bai wrote:
> Unused list was killed by commit 2531d9ee61fa ("bcache: Kill unused freelist")
> but left these comments, let's drop them.
> 
> This patch doesn't introduce functional change.
> 
> Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>

Hi Yaowei,


> ---
>  drivers/md/bcache/alloc.c | 13 +++----------
>  drivers/md/bcache/super.c |  3 ---
>  2 files changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index 6f77682..c22c260 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -33,13 +33,6 @@
>   * If we've got discards enabled, that happens when a bucket moves from the
>   * free_inc list to the free list.
>   *
> - * There is another freelist, because sometimes we have buckets that we know
> - * have nothing pointing into them - these we can reuse without waiting for
> - * priorities to be rewritten. These come from freed btree nodes and buckets
> - * that garbage collection discovered no longer had valid keys pointing into
> - * them (because they were overwritten). That's the unused list - buckets on the
> - * unused list move to the free list, optionally being discarded in the process.
> - *
It seems the above comments can still be applied to free_inc list (if
s/freelist/free_inc list), am I right ?

>   * It's also important to ensure that gens don't wrap around - with respect to
>   * either the oldest gen in the btree or the gen on disk. This is quite
>   * difficult to do in practice, but we explicitly guard against it anyways - if
> @@ -323,9 +316,9 @@ static int bch_allocator_thread(void *arg)
>  
>  	while (1) {
>  		/*
> -		 * First, we pull buckets off of the unused and free_inc lists,
> -		 * possibly issue discards to them, then we add the bucket to
> -		 * the free list:
> +		 * First, we pull buckets off of the free_inc list, possibly
> +		 * issue discards to them, then we add the bucket to the free
> +		 * list:
>  		 */

I am OK with this.

>  		while (1) {
>  			long bucket;
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 26e374f..eba38aa 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -544,9 +544,6 @@ void bch_prio_write(struct cache *ca)
>  	atomic_long_add(ca->sb.bucket_size * prio_buckets(ca),
>  			&ca->meta_sectors_written);
>  
> -	//pr_debug("free %zu, free_inc %zu, unused %zu", fifo_used(&ca->free),
> -	//	 fifo_used(&ca->free_inc), fifo_used(&ca->unused));
> -

There is no freelist in the above code, I suggest to not include the
above change into this patch.


>  	for (i = prio_buckets(ca) - 1; i >= 0; --i) {
>  		long bucket;
>  		struct prio_set *p = ca->disk_buckets;
> 

Thanks.

-- 

Coly Li

Re: [PATCH 2/3] bcache: use allocator reserves instead of watermarks

[Coly Li]

On 2019/7/27 6:19 下午, Yaowei Bai wrote:
> Commit 78365411b344 ("bcache: Rework allocator reserves") introduced
> allocator reserves and dropped watermarks, let's keep this consistent
> to avoid confusing.
> 
> Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>

It is OK to me, I will add it to my for-test.

Thanks.

Coly Li

> ---
>  drivers/md/bcache/alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index c22c260..609df38 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -622,13 +622,13 @@ bool bch_alloc_sectors(struct cache_set *c,
>  	spin_lock(&c->data_bucket_lock);
>  
>  	while (!(b = pick_data_bucket(c, k, write_point, &alloc.key))) {
> -		unsigned int watermark = write_prio
> +		unsigned int reserve = write_prio
>  			? RESERVE_MOVINGGC
>  			: RESERVE_NONE;
>  
>  		spin_unlock(&c->data_bucket_lock);
>  
> -		if (bch_bucket_alloc_set(c, watermark, &alloc.key, 1, wait))
> +		if (bch_bucket_alloc_set(c, reserve, &alloc.key, 1, wait))
>  			return false;
>  
>  		spin_lock(&c->data_bucket_lock);
> 

Re: [PATCH 3/3] bcache: count cache_available_percent accurately

[Coly Li]

On 2019/7/27 6:19 下午, Yaowei Bai wrote:
> The interface cache_available_percent is used to indicate how
> many buckets in percent are available to be used to cache data
> at a specific moment. It should include the unused and clean
> buckets which we get from bch_btree_gc_finish function:
> 
> 	if (!GC_MARK(b) || GC_MARK(b) == GC_MARK_RECLAIMABLE)
> 		 c->avail_nbuckets++;
> 
> However currently in the allocation code we didn't distinguish
> these available buckets with the metadata and dirty buckets, we
> just decrease the c->avail_nbuckets everytime we allocate a bucket,
> and correct it after a gc completes. With this, in a read-only
> scenario, you can observe that cache_available_percent bounces,
> it first go down to a number, like 95, and then bounces back to
> 100. It goes on and on, making users confused.

Hmm, I don't feel it could be confused, indeed I feel this is what is
designed for, counting both data/meta data buckets allocation. We can
document in admin-guide/bcache.rst, and notice people that even for
read-only requests, buckets can also be allocated for metadata.

Thanks.

Coly Li


> 
> This patch fixes this problem by decreasing c->avail_nbuckets
> only when allocate metadata and dirty buckets. With this patch,
> cache_available_percent will always be accurate and avoid the
> confusion.
> 
> Signed-off-by: Yaowei Bai <baiyaowei@cmss.chinamobile.com>
> ---
>  drivers/md/bcache/alloc.c   | 10 +++++-----
>  drivers/md/bcache/request.c |  9 ++++++++-
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index 609df38..dc7f6c2 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -443,17 +443,17 @@ long bch_bucket_alloc(struct cache *ca, unsigned int reserve, bool wait)
>  		SET_GC_MARK(b, GC_MARK_METADATA);
>  		SET_GC_MOVE(b, 0);
>  		b->prio = BTREE_PRIO;
> +
> +		if (ca->set->avail_nbuckets > 0) {
> +			ca->set->avail_nbuckets--;
> +			bch_update_bucket_in_use(ca->set, &ca->set->gc_stats);
> +		}
>  	} else {
>  		SET_GC_MARK(b, GC_MARK_RECLAIMABLE);
>  		SET_GC_MOVE(b, 0);
>  		b->prio = INITIAL_PRIO;
>  	}
>  
> -	if (ca->set->avail_nbuckets > 0) {
> -		ca->set->avail_nbuckets--;
> -		bch_update_bucket_in_use(ca->set, &ca->set->gc_stats);
> -	}
> -
>  	return r;
>  }
>  
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 41adcd1..b69bd8d 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -244,9 +244,16 @@ static void bch_data_insert_start(struct closure *cl)
>  		if (op->writeback) {
>  			SET_KEY_DIRTY(k, true);
>  
> -			for (i = 0; i < KEY_PTRS(k); i++)
> +			for (i = 0; i < KEY_PTRS(k); i++) {
>  				SET_GC_MARK(PTR_BUCKET(op->c, k, i),
>  					    GC_MARK_DIRTY);
> +
> +				if (op->c->avail_nbuckets > 0) {
> +					op->c->avail_nbuckets--;
> +					bch_update_bucket_in_use(op->c,
> +								 &op->c->gc_stats);
> +				}
> +			}
>  		}
>  
>  		SET_KEY_CSUM(k, op->csum);
>