On Sat, Mar 09 2024 at 11:41P -0500,
Christoph Hellwig <hch@xxxxxx> 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@xxxxxx>
> ---
> 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
>
>
