From: Coly Li <colyli@xxxxxxx>
Date: 2023-06-06 18:52:05
To: linux-bcache@xxxxxxxxxxxxxxx
Cc: Coly Li <colyli@xxxxxxx>,Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
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@xxxxxxx>
>Cc: Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
>---
> 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
>
