[PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device

[PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device

[Coly Li]

The flash device is a special bcache device which doesn't have backing
device and stores all data on cache set. Although its data is treated
as dirty data but the writeback to backing device never happens.

Therefore it is unncessary to always allocate memory for counters
full_dirty_stripes and stripe_sectors_dirty when the bcache device is
on flash only.

This patch avoids to allocate/free memory for full_dirty_stripes and
stripe_sectors_dirty in bcache_device_init() and bcache_device_free().
Also in bcache_dev_sectors_dirty_add(), if the data is written onto
flash device, avoid to update the counters in full_dirty_stripes and
stripe_sectors_dirty because they are not used at all.

Signed-off-by: Coly Li <colyli@suse.de>
Cc: Mingzhe Zou <mingzhe.zou@easystack.cn>
---
 drivers/md/bcache/super.c     | 18 ++++++++++++++----
 drivers/md/bcache/writeback.c |  8 +++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index 077149c4050b..00edc093e544 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -887,12 +887,15 @@ static void bcache_device_free(struct bcache_device *d)
 	}
 
 	bioset_exit(&d->bio_split);
-	kvfree(d->full_dirty_stripes);
-	kvfree(d->stripe_sectors_dirty);
+	if (d->full_dirty_stripes)
+		kvfree(d->full_dirty_stripes);
+	if (d->stripe_sectors_dirty)
+		kvfree(d->stripe_sectors_dirty);
 
 	closure_debug_destroy(&d->cl);
 }
 
+
 static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
 		sector_t sectors, struct block_device *cached_bdev,
 		const struct block_device_operations *ops)
@@ -914,6 +917,10 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
 	}
 	d->nr_stripes = n;
 
+	/* Skip allocating stripes counters for flash device */
+	if (d->c && UUID_FLASH_ONLY(&d->c->uuids[d->id]))
+		goto get_idx;
+
 	n = d->nr_stripes * sizeof(atomic_t);
 	d->stripe_sectors_dirty = kvzalloc(n, GFP_KERNEL);
 	if (!d->stripe_sectors_dirty)
@@ -924,6 +931,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
 	if (!d->full_dirty_stripes)
 		goto out_free_stripe_sectors_dirty;
 
+get_idx:
 	idx = ida_simple_get(&bcache_device_idx, 0,
 				BCACHE_DEVICE_IDX_MAX, GFP_KERNEL);
 	if (idx < 0)
@@ -981,9 +989,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
 out_ida_remove:
 	ida_simple_remove(&bcache_device_idx, idx);
 out_free_full_dirty_stripes:
-	kvfree(d->full_dirty_stripes);
+	if (d->full_dirty_stripes)
+		kvfree(d->full_dirty_stripes);
 out_free_stripe_sectors_dirty:
-	kvfree(d->stripe_sectors_dirty);
+	if (d->stripe_sectors_dirty)
+		kvfree(d->stripe_sectors_dirty);
 	return -ENOMEM;
 
 }
diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 24c049067f61..32a034e74622 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -607,8 +607,14 @@ void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
 	if (stripe < 0)
 		return;
 
-	if (UUID_FLASH_ONLY(&c->uuids[inode]))
+	/*
+	 * The flash device doesn't have backing device to writeback,
+	 * it is unncessary to calculate stripes related stuffs.
+	 */
+	if (UUID_FLASH_ONLY(&c->uuids[inode])) {
 		atomic_long_add(nr_sectors, &c->flash_dev_dirty_sectors);
+		return;
+	}
 
 	stripe_offset = offset & (d->stripe_size - 1);
 
-- 
2.35.3

Re:[PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device

[邹明哲]

From: Coly Li <colyli@suse.de>
Date: 2023-06-06 18:52:05
To:  linux-bcache@vger.kernel.org
Cc:  Coly Li <colyli@suse.de>,Mingzhe Zou <mingzhe.zou@easystack.cn>
Subject: [PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device>The flash device is a special bcache device which doesn't have backing
>device and stores all data on cache set. Although its data is treated
>as dirty data but the writeback to backing device never happens.
>
>Therefore it is unncessary to always allocate memory for counters
>full_dirty_stripes and stripe_sectors_dirty when the bcache device is
>on flash only.
>
>This patch avoids to allocate/free memory for full_dirty_stripes and
>stripe_sectors_dirty in bcache_device_init() and bcache_device_free().
>Also in bcache_dev_sectors_dirty_add(), if the data is written onto
>flash device, avoid to update the counters in full_dirty_stripes and
>stripe_sectors_dirty because they are not used at all.
>
>Signed-off-by: Coly Li <colyli@suse.de>
>Cc: Mingzhe Zou <mingzhe.zou@easystack.cn>
>---
> drivers/md/bcache/super.c     | 18 ++++++++++++++----
> drivers/md/bcache/writeback.c |  8 +++++++-
> 2 files changed, 21 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
>index 077149c4050b..00edc093e544 100644
>--- a/drivers/md/bcache/super.c
>+++ b/drivers/md/bcache/super.c
>@@ -887,12 +887,15 @@ static void bcache_device_free(struct bcache_device *d)
> 	}
> 
> 	bioset_exit(&d->bio_split);
>-	kvfree(d->full_dirty_stripes);
>-	kvfree(d->stripe_sectors_dirty);
>+	if (d->full_dirty_stripes)
>+		kvfree(d->full_dirty_stripes);
>+	if (d->stripe_sectors_dirty)
>+		kvfree(d->stripe_sectors_dirty);
> 
> 	closure_debug_destroy(&d->cl);
> }
> 
>+
> static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> 		sector_t sectors, struct block_device *cached_bdev,
> 		const struct block_device_operations *ops)
>@@ -914,6 +917,10 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> 	}
> 	d->nr_stripes = n;
> 
>+	/* Skip allocating stripes counters for flash device */
>+	if (d->c && UUID_FLASH_ONLY(&d->c->uuids[d->id]))
>+		goto get_idx;
>+
> 	n = d->nr_stripes * sizeof(atomic_t);
> 	d->stripe_sectors_dirty = kvzalloc(n, GFP_KERNEL);
> 	if (!d->stripe_sectors_dirty)
>@@ -924,6 +931,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> 	if (!d->full_dirty_stripes)
> 		goto out_free_stripe_sectors_dirty;
> 
>+get_idx:
> 	idx = ida_simple_get(&bcache_device_idx, 0,
> 				BCACHE_DEVICE_IDX_MAX, GFP_KERNEL);
> 	if (idx < 0)
>@@ -981,9 +989,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> out_ida_remove:
> 	ida_simple_remove(&bcache_device_idx, idx);
> out_free_full_dirty_stripes:
>-	kvfree(d->full_dirty_stripes);
>+	if (d->full_dirty_stripes)
>+		kvfree(d->full_dirty_stripes);
> out_free_stripe_sectors_dirty:
>-	kvfree(d->stripe_sectors_dirty);
>+	if (d->stripe_sectors_dirty)
>+		kvfree(d->stripe_sectors_dirty);
> 	return -ENOMEM;
> 
> }
>diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
>index 24c049067f61..32a034e74622 100644
>--- a/drivers/md/bcache/writeback.c
>+++ b/drivers/md/bcache/writeback.c
>@@ -607,8 +607,14 @@ void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
> 	if (stripe < 0)
> 		return;
> 
>-	if (UUID_FLASH_ONLY(&c->uuids[inode]))
>+	/*
>+	 * The flash device doesn't have backing device to writeback,
>+	 * it is unncessary to calculate stripes related stuffs.
>+	 */
>+	if (UUID_FLASH_ONLY(&c->uuids[inode])) {
> 		atomic_long_add(nr_sectors, &c->flash_dev_dirty_sectors);
>+		return;
>+	}
> 
> 	stripe_offset = offset & (d->stripe_size - 1);
> 

look good, there may be a problem, d->stripe_sectors_dirty will be
a null pointer for flash device:

```
static inline uint64_t bcache_dev_sectors_dirty(struct bcache_device *d)
{
	uint64_t i, ret = 0;

	for (i = 0; i < d->nr_stripes; i++)
		ret += atomic_read(d->stripe_sectors_dirty + i);

	return ret;
}

static void flash_dev_free(struct closure *cl)
{
	struct bcache_device *d = container_of(cl, struct bcache_device, cl);

	mutex_lock(&bch_register_lock);
	atomic_long_sub(bcache_dev_sectors_dirty(d),
			&d->c->flash_dev_dirty_sectors);
	del_gendisk(d->disk);
	bcache_device_free(d);
	mutex_unlock(&bch_register_lock);
	kobject_put(&d->kobj);
}
```

The only use of c->flash_dev_dirty_sectorsis to calculate cache_sectors:

```
/* Rate limiting */
static uint64_t __calc_target_rate(struct cached_dev *dc)
{
	struct cache_set *c = dc->disk.c;

	/*
	 * This is the size of the cache, minus the amount used for
	 * flash-only devices
	 */
	uint64_t cache_sectors = c->nbuckets * c->cache->sb.bucket_size -
				atomic_long_read(&c->flash_dev_dirty_sectors);
```

Do we still need to update the dirty_data of flash device. Whether to
directly use the size of flash device as a dirty_data.

mingzhe
>-- 
>2.35.3
>

Re: [PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device

[Coly Li]

On Tue, Jun 06, 2023 at 08:50:11PM +0800, 邹明哲 wrote:
> From: Coly Li <colyli@suse.de>
> Date: 2023-06-06 18:52:05
> To:  linux-bcache@vger.kernel.org
> Cc:  Coly Li <colyli@suse.de>,Mingzhe Zou <mingzhe.zou@easystack.cn>
> Subject: [PATCH] bcache: don't allocate full_dirty_stripes and stripe_sectors_dirty for flash device>The flash device is a special bcache device which doesn't have backing
> >device and stores all data on cache set. Although its data is treated
> >as dirty data but the writeback to backing device never happens.
> >
> >Therefore it is unncessary to always allocate memory for counters
> >full_dirty_stripes and stripe_sectors_dirty when the bcache device is
> >on flash only.
> >
> >This patch avoids to allocate/free memory for full_dirty_stripes and
> >stripe_sectors_dirty in bcache_device_init() and bcache_device_free().
> >Also in bcache_dev_sectors_dirty_add(), if the data is written onto
> >flash device, avoid to update the counters in full_dirty_stripes and
> >stripe_sectors_dirty because they are not used at all.
> >
> >Signed-off-by: Coly Li <colyli@suse.de>
> >Cc: Mingzhe Zou <mingzhe.zou@easystack.cn>
> >---
> > drivers/md/bcache/super.c     | 18 ++++++++++++++----
> > drivers/md/bcache/writeback.c |  8 +++++++-
> > 2 files changed, 21 insertions(+), 5 deletions(-)
> >
> >diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> >index 077149c4050b..00edc093e544 100644
> >--- a/drivers/md/bcache/super.c
> >+++ b/drivers/md/bcache/super.c
> >@@ -887,12 +887,15 @@ static void bcache_device_free(struct bcache_device *d)
> > 	}
> > 
> > 	bioset_exit(&d->bio_split);
> >-	kvfree(d->full_dirty_stripes);
> >-	kvfree(d->stripe_sectors_dirty);
> >+	if (d->full_dirty_stripes)
> >+		kvfree(d->full_dirty_stripes);
> >+	if (d->stripe_sectors_dirty)
> >+		kvfree(d->stripe_sectors_dirty);
> > 
> > 	closure_debug_destroy(&d->cl);
> > }
> > 
> >+
> > static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> > 		sector_t sectors, struct block_device *cached_bdev,
> > 		const struct block_device_operations *ops)
> >@@ -914,6 +917,10 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> > 	}
> > 	d->nr_stripes = n;
> > 
> >+	/* Skip allocating stripes counters for flash device */
> >+	if (d->c && UUID_FLASH_ONLY(&d->c->uuids[d->id]))
> >+		goto get_idx;
> >+
> > 	n = d->nr_stripes * sizeof(atomic_t);
> > 	d->stripe_sectors_dirty = kvzalloc(n, GFP_KERNEL);
> > 	if (!d->stripe_sectors_dirty)
> >@@ -924,6 +931,7 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> > 	if (!d->full_dirty_stripes)
> > 		goto out_free_stripe_sectors_dirty;
> > 
> >+get_idx:
> > 	idx = ida_simple_get(&bcache_device_idx, 0,
> > 				BCACHE_DEVICE_IDX_MAX, GFP_KERNEL);
> > 	if (idx < 0)
> >@@ -981,9 +989,11 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
> > out_ida_remove:
> > 	ida_simple_remove(&bcache_device_idx, idx);
> > out_free_full_dirty_stripes:
> >-	kvfree(d->full_dirty_stripes);
> >+	if (d->full_dirty_stripes)
> >+		kvfree(d->full_dirty_stripes);
> > out_free_stripe_sectors_dirty:
> >-	kvfree(d->stripe_sectors_dirty);
> >+	if (d->stripe_sectors_dirty)
> >+		kvfree(d->stripe_sectors_dirty);
> > 	return -ENOMEM;
> > 
> > }
> >diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> >index 24c049067f61..32a034e74622 100644
> >--- a/drivers/md/bcache/writeback.c
> >+++ b/drivers/md/bcache/writeback.c
> >@@ -607,8 +607,14 @@ void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
> > 	if (stripe < 0)
> > 		return;
> > 
> >-	if (UUID_FLASH_ONLY(&c->uuids[inode]))
> >+	/*
> >+	 * The flash device doesn't have backing device to writeback,
> >+	 * it is unncessary to calculate stripes related stuffs.
> >+	 */
> >+	if (UUID_FLASH_ONLY(&c->uuids[inode])) {
> > 		atomic_long_add(nr_sectors, &c->flash_dev_dirty_sectors);
> >+		return;
> >+	}
> > 
> > 	stripe_offset = offset & (d->stripe_size - 1);
> > 
> 
> look good, there may be a problem, d->stripe_sectors_dirty will be
> a null pointer for flash device:
> 
> ```
> static inline uint64_t bcache_dev_sectors_dirty(struct bcache_device *d)
> {
> 	uint64_t i, ret = 0;
> 
> 	for (i = 0; i < d->nr_stripes; i++)
> 		ret += atomic_read(d->stripe_sectors_dirty + i);
> 
> 	return ret;
> }
> 
> static void flash_dev_free(struct closure *cl)
> {
> 	struct bcache_device *d = container_of(cl, struct bcache_device, cl);
> 
> 	mutex_lock(&bch_register_lock);
> 	atomic_long_sub(bcache_dev_sectors_dirty(d),
> 			&d->c->flash_dev_dirty_sectors);
> 	del_gendisk(d->disk);
> 	bcache_device_free(d);
> 	mutex_unlock(&bch_register_lock);
> 	kobject_put(&d->kobj);
> }
> ```

You are correct. The above atomic_long_sub() can be avoided, if dirty sectors
of the flash device are not counted by stripe_sectors_dirty counters. This is
something might be improved. Let me think about it.

> 
> The only use of c->flash_dev_dirty_sectorsis to calculate cache_sectors:
> 
> ```
> /* Rate limiting */
> static uint64_t __calc_target_rate(struct cached_dev *dc)
> {
> 	struct cache_set *c = dc->disk.c;
> 
> 	/*
> 	 * This is the size of the cache, minus the amount used for
> 	 * flash-only devices
> 	 */
> 	uint64_t cache_sectors = c->nbuckets * c->cache->sb.bucket_size -
> 				atomic_long_read(&c->flash_dev_dirty_sectors);
> ```
> 
> Do we still need to update the dirty_data of flash device. Whether to
> directly use the size of flash device as a dirty_data.

Yes we have to. The flash device is like kind of thin-provision allocation,
its size is specificed in creation time, but the real occupied space is
indicated by the dirty sectors. So the flash device size can be much larger
than its real consumed space.



-- 
Coly Li