| Message ID | 20240309164140.719752-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dm: set the correct discard_sectors limit | expand |
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 > >
On Sat, Mar 09, 2024 at 01:33:04PM -0500, Mike Snitzer wrote: > 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. Yes. > 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 I mean all the places that I've updated to set max_discard_sectors really should be setting max_hw_discard_sectors. The rutime- disabling of discard/write_zeroes/secure_discard on failure will get new helpers one the atomic queue limits conversion is finished. > > > 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? queue_limits_set now always recalculates max_discard_sectors from max_hw_discard_sectors and max_user_discard_sectors. So if a driver sets max_discard_sectors but not max_hw_discard_sectors it will simply be overriden.
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; }
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. This fixes a regression where dm bio poison v1 warns about exceeding the discard bio size when running xfstests generic/500. 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(-)