On Mon, May 20, 2024 at 05:06:53PM +0200, Christoph Hellwig wrote:
> On Sun, May 19, 2024 at 01:42:00AM -0400, Mike Snitzer wrote:
> > > This being one potential fix from code inspection I've done to this
> > > point, please see if it resolves your fstests failures (but I haven't
> > > actually looked at those fstests yet _and_ I still need to review
> > > commits d690cb8ae14bd and 4f563a64732da further -- will do on Monday,
> > > sorry for the trouble):
> >
> > I looked early, this is needed (max_user_discard_sectors makes discard
> > limits stacking suck more than it already did -- imho 4f563a64732da is
> > worthy of revert.
>
> Can you explain why? This actually makes the original addition of the
> user-space controlled max discard limit work. No I'm a bit doubful
> that allowing this control was a good idea, but that ship unfortunately
> has sailed.
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.
> Short of that, dm-cache-target.c and possibly other
> > DM targets will need fixes too -- I'll go over it all Monday):
> >
> > 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
> > @@ -4099,8 +4099,10 @@ 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;
> > + if (!pt->adjusted_pf.discard_passdown) {
> > + limits->max_hw_discard_sectors = 0;
> > + limits->max_user_discard_sectors = 0;
> > + }
>
> I think the main problem here is that dm targets adjust
> max_discard_sectors diretly instead of adjusting max_hw_discard_sectors.
> Im other words we need to switch all places dm targets set
> max_discard_sectors to use max_hw_discard_sectors instead. They should
> not touch max_user_discard_sectors ever.
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.
> This is probably my fault, I actually found this right at the time
> of the original revert of switching dm to the limits API, and then
> let it slip as the patch was reverted. That fact that you readded
> the commit somehow went past my attention window.
It's fine, all we can do now is work through how best to fix it. Open
to suggestions. But this next hunk, which you trimmed in your reply,
_seems_ needed to actually fix the issue Ted reported -- given the
current validate method in blk-settings.c (resharing here to just
continue this thread in a natural way):
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;
}
}
