Re: dm: set the correct discard_sectors limit

[Mike Snitzer <snitzer@kernel.org>]

On Sat, Mar 09 2024 at 11:41P -0500,
Christoph Hellwig <hch@lst.de> wrote:

> Since commit 0034af036554 ("block: make
> /sys/block/<dev>/queue/discard_max_bytes writeable") there are two
> max_discard_sectors limits, one provided by the driver and one set by the
> user to optionally reduce the size below the hardware or driver limits.
> Usage of the extra hw limits has been a bit convoluted and both were
> set by the same driver API, leading to potential overrides of the user
> setting by the driver updating the limits.
> 
> With the new atomic queue limits API the driver should only set the hw
> limit, but I forgot to convert dm over to that as it was already using
> a scheme where the queue_limits are passed around.  Fix dm to update
> the max_hw_discard_sectors limits.
> 
> Note that this still leaves the non-hw update in place, which should
> be removed to not override the user settings.  As that is a behavior
> change I do not want to do it at the very end of the merge window.

That 2015 commit (0034af036554) was really ham-handed, not sure how
I've remained unaware of this duality (with soft and hard discard
limits) until now BUT there is quite a bit of DM code that only
concerns itself with max_discard_sectors and discard_granularity.

Anyway, I'm not quite sure what you're referring to, only code that is
still setting max_discard_sectors is drivers/md/dm.c:disable_discard

> This fixes a regression where dm bio poison v1 warns about exceeding
> the discard bio size when running xfstests generic/500.

Meaning max_discard_sectors > max_hw_discard_sectors? What changed to
expose this?

Also, typo in above commit message: s/dm bio poison/dm bio prison/


> Fixes: 8e0ef4128694 ("dm: use queue_limits_set")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/md/dm-cache-target.c | 5 +++--
>  drivers/md/dm-clone-target.c | 3 ++-
>  drivers/md/dm-log-writes.c   | 2 +-
>  drivers/md/dm-snap.c         | 2 +-
>  drivers/md/dm-thin.c         | 5 +++--
>  drivers/md/dm.c              | 1 +
>  6 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
> index 911f73f7ebbaa0..71d7824c731862 100644
> --- a/drivers/md/dm-cache-target.c
> +++ b/drivers/md/dm-cache-target.c
> @@ -3394,8 +3394,9 @@ static void set_discard_limits(struct cache *cache, struct queue_limits *limits)
>  
>  	if (!cache->features.discard_passdown) {
>  		/* No passdown is done so setting own virtual limits */
> -		limits->max_discard_sectors = min_t(sector_t, cache->discard_block_size * 1024,
> -						    cache->origin_sectors);
> +		limits->max_hw_discard_sectors =
> +			min_t(sector_t, cache->discard_block_size * 1024,
> +					cache->origin_sectors);
>  		limits->discard_granularity = cache->discard_block_size << SECTOR_SHIFT;
>  		return;
>  	}
> diff --git a/drivers/md/dm-clone-target.c b/drivers/md/dm-clone-target.c
> index 94b2fc33f64be3..861a8ff524154f 100644
> --- a/drivers/md/dm-clone-target.c
> +++ b/drivers/md/dm-clone-target.c
> @@ -2050,7 +2050,8 @@ static void set_discard_limits(struct clone *clone, struct queue_limits *limits)
>  	if (!test_bit(DM_CLONE_DISCARD_PASSDOWN, &clone->flags)) {
>  		/* No passdown is done so we set our own virtual limits */
>  		limits->discard_granularity = clone->region_size << SECTOR_SHIFT;
> -		limits->max_discard_sectors = round_down(UINT_MAX >> SECTOR_SHIFT, clone->region_size);
> +		limits->max_hw_discard_sectors =
> +			round_down(UINT_MAX >> SECTOR_SHIFT, clone->region_size);
>  		return;
>  	}
>  
> diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c
> index f17a6cf2284ecf..8d7df8303d0a18 100644
> --- a/drivers/md/dm-log-writes.c
> +++ b/drivers/md/dm-log-writes.c
> @@ -871,7 +871,7 @@ static void log_writes_io_hints(struct dm_target *ti, struct queue_limits *limit
>  	if (!bdev_max_discard_sectors(lc->dev->bdev)) {
>  		lc->device_supports_discard = false;
>  		limits->discard_granularity = lc->sectorsize;
> -		limits->max_discard_sectors = (UINT_MAX >> SECTOR_SHIFT);
> +		limits->max_hw_discard_sectors = (UINT_MAX >> SECTOR_SHIFT);
>  	}
>  	limits->logical_block_size = bdev_logical_block_size(lc->dev->bdev);
>  	limits->physical_block_size = bdev_physical_block_size(lc->dev->bdev);
> diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
> index bf7a574499a34d..07961e7e8382ab 100644
> --- a/drivers/md/dm-snap.c
> +++ b/drivers/md/dm-snap.c
> @@ -2408,7 +2408,7 @@ static void snapshot_io_hints(struct dm_target *ti, struct queue_limits *limits)
>  
>  		/* All discards are split on chunk_size boundary */
>  		limits->discard_granularity = snap->store->chunk_size;
> -		limits->max_discard_sectors = snap->store->chunk_size;
> +		limits->max_hw_discard_sectors = snap->store->chunk_size;
>  
>  		up_read(&_origins_lock);
>  	}
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 07c7f9795b107b..d6adccda966f92 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -4096,7 +4096,7 @@ static void pool_io_hints(struct dm_target *ti, struct queue_limits *limits)
>  	if (pt->adjusted_pf.discard_enabled) {
>  		disable_discard_passdown_if_not_supported(pt);
>  		if (!pt->adjusted_pf.discard_passdown)
> -			limits->max_discard_sectors = 0;
> +			limits->max_hw_discard_sectors = 0;
>  		/*
>  		 * The pool uses the same discard limits as the underlying data
>  		 * device.  DM core has already set this up.
> @@ -4493,7 +4493,8 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
>  
>  	if (pool->pf.discard_enabled) {
>  		limits->discard_granularity = pool->sectors_per_block << SECTOR_SHIFT;
> -		limits->max_discard_sectors = pool->sectors_per_block * BIO_PRISON_MAX_RANGE;
> +		limits->max_hw_discard_sectors =
> +			pool->sectors_per_block * BIO_PRISON_MAX_RANGE;
>  	}
>  }
>  
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index b5e6a10b9cfde3..de7703070905ff 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1077,6 +1077,7 @@ void disable_discard(struct mapped_device *md)
>  	struct queue_limits *limits = dm_get_queue_limits(md);
>  
>  	/* device doesn't really support DISCARD, disable it */
> +	limits->max_hw_discard_sectors = 0;
>  	limits->max_discard_sectors = 0;
>  }
>  
> -- 
> 2.39.2
> 
>