dm: use queue_limits_set

#regzbot introduced: 1c0e720228ad

While doing final regression testing before sending a pull request for the ext4 tree, I found a regression which was triggered by generic/347 and generic/405 on on multiple fstests configurations, including both ext4/4k and xfs/4k.

It bisects cleanly to commit 1c0e720228ad ("dm: use queue_limits_set"), and the resulting WARNING is attached below. This stack trace can be seen for both generic/347 and generic/405. And if I revert this commit on top of linux-next, the failure goes away, so it pretty clearly root causes to 1c0e720228ad.

For now, I'll add generic/347 and generic/405 to my global exclude file, but maybe we should consider reverting the commit if it can't be fixed quickly?

Hi Ted,

Quoted 13 lines · Theodore Ts'o

#regzbot introduced: 1c0e720228ad

While doing final regression testing before sending a pull request for the ext4 tree, I found a regression which was triggered by generic/347 and generic/405 on on multiple fstests configurations, including both ext4/4k and xfs/4k.

It bisects cleanly to commit 1c0e720228ad ("dm: use queue_limits_set"), and the resulting WARNING is attached below. This stack trace can be seen for both generic/347 and generic/405. And if I revert this commit on top of linux-next, the failure goes away, so it pretty clearly root causes to 1c0e720228ad.

Quoted 3 lines

For now, I'll add generic/347 and generic/405 to my global exclude file, but maybe we should consider reverting the commit if it can't be fixed quickly?

Commit 1c0e720228ad is a red herring, it switches DM over to using queue_limits_set() which I now see is clearly disregarding DM's desire to disable discards (in blk_validate_limits).

It looks like the combo of commit d690cb8ae14bd ("block: add an API to atomically update queue limits") and 4f563a64732da ("block: add a max_user_discard_sectors queue limit") needs fixing.

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):

Quoted 5 lines

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.

DM targets will need fixes too -- I'll go over it all Monday):

drivers/md/dm-thin.c +0 -0

index 4793ad2aa1f7..c196f39579af 100644

Quoted 6 lines · Mike Snitzer

Quoted 5 lines

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):

Quoted 3 lines

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.

Short of that, dm-cache-target.c and possibly other

Quoted 8 lines

DM targets will need fixes too -- I'll go over it all Monday):

drivers/md/dm-thin.c +0 -0

index 4793ad2aa1f7..c196f39579af 100644
@@ -4099,8 +4099,10 @@ static void pool_io_hints(struct dm_target *ti, struct queue_limits *limits)
 

drivers/md/dm-thin.c static void pool_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4105

Quoted 8 lines

drivers/md/dm-thin.c +4 -2

 	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.

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.

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.

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.

Quoted 6 lines · Mike Snitzer

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.

Quoted 4 lines

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.

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.

Quoted 6 lines

drivers/md/dm-thin.c +0 -0

index 4793ad2aa1f7..c196f39579af 100644
@@ -4497,7 +4499,8 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4504

Quoted 6 lines

drivers/md/dm-thin.c +2 -1

 	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.

I'm also kinda curious what actually sets a user limit in Ted's case as that feels weird.

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Mike Snitzer

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.

Quoted 7 lines

The user forcing a limit is always very sketchy, which is why I'm not a fan of it.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

Quoted 6 lines

drivers/md/dm-thin.c +0 -0

index 4793ad2aa1f7..c196f39579af 100644
@@ -4497,7 +4499,8 @@ static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits)
 

Quoted 11 lines

drivers/md/dm-thin.c static void thin_io_hints(struct dm_target *ti, struct queue_limits *limits) · line 4504

Quoted 6 lines

drivers/md/dm-thin.c +2 -1

 	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.

Quoted 2 lines

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)

Quoted 10 lines · Mike Snitzer

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)

Oh, that makes more sense.

I think you just want to set the max_hw_discard_sectors limit before stacking in the lower device limits so that they can only lower it.

(and in the long run we should just stop stacking the limits except for request based dm which really needs it)

Quoted 12 lines · Christoph Hellwig

Quoted 10 lines · Mike Snitzer

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)

Quoted 7 lines

Oh, that makes more sense.

I think you just want to set the max_hw_discard_sectors limit before stacking in the lower device limits so that they can only lower it.

(and in the long run we should just stop stacking the limits except for request based dm which really needs it)

This is what I staged, I cannot send a patch out right now..

Ted if you need the patch in email (rather than from linux-dm.git) I can send it later tonight. Please see:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.10&id=825d8bbd2f32cb229c3b6653bd454832c3c20acb

Quoted 21 lines · Mike Snitzer

Quoted 12 lines · Christoph Hellwig

Quoted 10 lines · Mike Snitzer

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)

Quoted 7 lines

Oh, that makes more sense.

I think you just want to set the max_hw_discard_sectors limit before stacking in the lower device limits so that they can only lower it.

(and in the long run we should just stop stacking the limits except for request based dm which really needs it)

Quoted 6 lines

This is what I staged, I cannot send a patch out right now..

Ted if you need the patch in email (rather than from linux-dm.git) I can send it later tonight. Please see:

https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.10&id=825d8bbd2f32cb229c3b6653bd454832c3c20acb

From: Mike Snitzer <[email protected]>
Date: Mon, 20 May 2024 13:34:06 -0400
Subject: [PATCH] dm: always manage discard support in terms of max_hw_discard_sectors

Commit 4f563a64732d ("block: add a max_user_discard_sectors queue limit") changed block core to set max_discard_sectors to:
min(lim->max_hw_discard_sectors, lim->max_user_discard_sectors)

Since commit 1c0e720228ad ("dm: use queue_limits_set") it was reported dm-thinp was failing in a few fstests (generic/347 and generic/405) with the first WARN_ON_ONCE in dm_cell_key_has_valid_range() being reported, e.g.:

WARNING: CPU: 1 PID: 30 at drivers/md/dm-bio-prison-v1.c:128 dm_cell_key_has_valid_range+0x3d/0x50

blk_set_stacking_limits() sets max_user_discard_sectors to UINT_MAX, so given how block core now sets max_discard_sectors (detailed above) it follows that blk_stack_limits() stacks up the underlying device's max_hw_discard_sectors and max_discard_sectors is set to match it. If max_hw_discard_sectors exceeds dm's BIO_PRISON_MAX_RANGE, then dm_cell_key_has_valid_range() will trigger the warning with:
WARN_ON_ONCE(key->block_end - key->block_begin > BIO_PRISON_MAX_RANGE)

Aside from this warning, the discard will fail. Fix this and other DM issues by governing discard support in terms of max_hw_discard_sectors instead of max_discard_sectors.

Reported-by Theodore Ts'o <[email protected]>

Fixes 1c0e720228ad ("dm: use queue_limits_set")

Signed-off-by Mike Snitzer <[email protected]>

Quoted 4 lines · Christoph Hellwig

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):

drivers/md/dm-thin.c +0 -0

index 4793ad2aa1f7..c196f39579af 100644

Maybe update blk_validate_limits() to ensure max_discard_sectors is a factor of discard_granularity?