[replying for completeness to explain what I think is happening for the issue Ted reported] On Mon, May 20, 2024 at 11:54:53AM -0400, Mike Snitzer wrote: > On Mon, May 20, 2024 at 05:44:25PM +0200, Christoph Hellwig wrote: > > On Mon, May 20, 2024 at 11:39:14AM -0400, Mike Snitzer wrote: > > > That's fair. My criticism was more about having to fix up DM targets > > > to cope with the new normal of max_discard_sectors being set as a > > > function of max_hw_discard_sectors and max_user_discard_sectors. > > > > > > With stacked devices in particular it is _very_ hard for the user to > > > know their exerting control over a max discard limit is correct. > > > > The user forcing a limit is always very sketchy, which is why I'm > > not a fan of it. > > > > > Yeah, but my concern is that if a user sets a value that is too low > > > it'll break targets like DM thinp (which Ted reported). So forcibly > > > setting both to indirectly set the required max_discard_sectors seems > > > necessary. Could also be that a user sets the max discard too large (e.g. larger than thinp's BIO_PRISON_MAX_RANGE). > > Dm-think requiring a minimum discard size is a rather odd requirement. > > Is this just a debug asswert, or is there a real technical reason > > for it? If so we can introduce a now to force a minimum size or > > disable user setting the value entirely. > > thinp's discard implementation is constrained by the dm-bio-prison's > constraints. One of the requirements of dm-bio-prison is that a > discard not exceed BIO_PRISON_MAX_RANGE. > > My previous reply is a reasonible way to ensure best effort to respect > a users request but that takes into account the driver provided > discard_granularity. It'll force suboptimal (too small) discards be > issued but at least they'll cover a full thinp block. Given below, this isn't at the heart of the issue Ted reported. So the change to ensure max_discard_sectors is a factor of discard_granularity, while worthwhile, isn't critical to fixing the reported issue. > > > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c > > > index 4793ad2aa1f7..c196f39579af 100644 > > > --- a/drivers/md/dm-thin.c > > > +++ b/drivers/md/dm-thin.c > > > @@ -4497,7 +4499,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 = limits->max_user_discard_sectors = > > > + pool->sectors_per_block * BIO_PRISON_MAX_RANGE; > > > } > > > > Drivers really have no business setting max_user_discard_sector, > > the whole point of the field is to separate device/driver capabilities > > from user policy. So if dm-think really has no way of handling > > smaller discards, we need to ensure they can't be set. > > It can handle smaller so long as they respect discard_granularity. > > > I'm also kinda curious what actually sets a user limit in Ted's case > > as that feels weird. > > I agree, not sure... maybe the fstests is using the knob? Doubt there was anything in fstests setting max discard user limit (max_user_discard_sectors) in Ted's case. blk_set_stacking_limits() sets max_user_discard_sectors to UINT_MAX, so given the use of min(lim->max_hw_discard_sectors, lim->max_user_discard_sectors) I suspect blk_stack_limits() stacks up max_discard_sectors to match the underlying storage's max_hw_discard_sectors. And max_hw_discard_sectors exceeds BIO_PRISON_MAX_RANGE, resulting in dm_cell_key_has_valid_range() triggering on: WARN_ON_ONCE(key->block_end - key->block_begin > BIO_PRISON_MAX_RANGE) Mike