Re: [PATCH 2/3] bcache: allocate stripe memory when partial_stripes_expensive is true

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 12, 2022 at 11:20:49AM +0800, mingzhe.zou@xxxxxxxxxxxx wrote:
> From: mingzhe <mingzhe.zou@xxxxxxxxxxxx>
> 
> Currently, bcache_device (cached_dev and flash_dev) always allocate
> memory for stripe_sectors_dirty and full_dirty_stripes, regardless of
> whether partial_stripes_expensive is true or not. When the device's
> partial_stripes_expensive is false, only bcache_dev_sectors_dirty_add()
> will use stripe_sectors_dirty.
> 
> When stripe_size is 0, it is forced to 2^31, which is about 1T (2^31*512).
> However, some non-raid devices (such as rbd) will provide non-zero io_opt.
> In https://bugzilla.redhat.com/show_bug.cgi?id=1783075, some block devices
> which large capacity (e.g. 8TB) but small io_opt size (e.g. 8 sectors), the
> nr_stripes will be very large. Even though the overflow bug is fixed in
> 65f0f017e7be and 7a1481267999, it still returns an error when register.
> 
> I don't think it's necessary to allocate stripe memory for devices where
> partial_stripes_expensive is false. This patch will allocate stripe memory
> when partial_stripes_expensive is true.
> 
> Signed-off-by: mingzhe <mingzhe.zou@xxxxxxxxxxxx>
> ---
>  drivers/md/bcache/super.c     | 31 ++++++++++++++++++++++---------
>  drivers/md/bcache/writeback.c | 14 ++++++++++----
>  2 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index a91a1c3f4055..83b5bbb5dcc8 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -887,15 +887,20 @@ 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)
> +			      sector_t sectors, bool enable_stripe,
> +			      struct block_device *cached_bdev,
> +			      const struct block_device_operations *ops)

The extra enable_stripe parameter might not be necessary. You have parameter cached_bdev,
dc->partial_stripes_expensive can be read without an extra parameter.
 

>  {
>  	struct request_queue *q;
>  	const size_t max_stripes = min_t(size_t, INT_MAX,
> @@ -903,6 +908,9 @@ static int bcache_device_init(struct bcache_device *d, unsigned int block_size,
>  	uint64_t n;
>  	int idx;
>  
> +	if (!enable_stripe)
> +		goto skip_stripe;
> +

Since the stripes related  calculation and allocation might not be mandatory, the
calculation of max_stripes even can be delayed to here.


>  	if (!d->stripe_size)
>  		d->stripe_size = 1 << 31;
>  
> @@ -924,6 +932,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;
>  
> +skip_stripe:
>  	idx = ida_simple_get(&bcache_device_idx, 0,
>  				BCACHE_DEVICE_IDX_MAX, GFP_KERNEL);
>  	if (idx < 0)
> @@ -982,9 +991,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;
>  


The above code looks fine.


>  }
> @@ -1397,6 +1408,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>  	int ret;
>  	struct io *io;
>  	struct request_queue *q = bdev_get_queue(dc->bdev);
> +	sector_t sectors = dc->bdev->bd_part->nr_sects - dc->sb.data_offset;
>  
>  	__module_get(THIS_MODULE);
>  	INIT_LIST_HEAD(&dc->list);
> @@ -1423,8 +1435,9 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
>  			q->limits.raid_partial_stripes_expensive;
>  
>  	ret = bcache_device_init(&dc->disk, block_size,
> -			 bdev_nr_sectors(dc->bdev) - dc->sb.data_offset,
> -			 dc->bdev, &bcache_cached_ops);
> +				 bdev_nr_sectors(dc->bdev) - dc->sb.data_offset,
> +				 dc->partial_stripes_expensive,
> +				 dc->bdev, &bcache_cached_ops);
>  	if (ret)
>  		return ret;
>  
> @@ -1535,7 +1548,7 @@ static int flash_dev_run(struct cache_set *c, struct uuid_entry *u)
>  
>  	kobject_init(&d->kobj, &bch_flash_dev_ktype);
>  
> -	if (bcache_device_init(d, block_bytes(c->cache), u->sectors,
> +	if (bcache_device_init(d, block_bytes(c->cache), u->sectors, false,
>  			NULL, &bcache_flash_ops))
>  		goto err;
>  

As I said the extra parameter enable_stripe is unnecessary, the above change can be avoided.

> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 7b5009e8b4ff..3f4af7ce6936 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -758,6 +758,7 @@ static void read_dirty(struct cached_dev *dc)
>  void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
>  				  uint64_t offset, int nr_sectors)
>  {
> +	struct cached_dev *dc = NULL;
>  	struct bcache_device *d = c->devices[inode];
>  	unsigned int stripe_offset, sectors_dirty;
>  	int stripe;
> @@ -765,14 +766,19 @@ void bcache_dev_sectors_dirty_add(struct cache_set *c, unsigned int inode,
>  	if (!d)
>  		return;
>  
> -	stripe = offset_to_stripe(d, offset);
> -	if (stripe < 0)
> -		return;
> -
>  	atomic_long_add(nr_sectors, &d->dirty_sectors);
>  
>  	if (UUID_FLASH_ONLY(&c->uuids[inode]))
>  		atomic_long_add(nr_sectors, &c->flash_dev_dirty_sectors);
> +	else
> +		dc = container_of(d, struct cached_dev, disk);
> +
> +	if (!dc || !dc->partial_stripes_expensive)
> +		return;
> +
> +	stripe = offset_to_stripe(d, offset);
> +	if (stripe < 0)
> +		return;
>  
>  	stripe_offset = offset & (d->stripe_size - 1);
> 

Once you changed the first patch, the above change might be modified to follow the previous change
as well.

 
> -- 
> 2.17.1
> 

-- 
Coly Li



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux ARM Kernel]     [Linux Filesystem Development]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux