Re: dm: use queue_limits_set

Re: dm: use queue_limits_set

[Mike Snitzer]

Hi Ted,

On Fri, May 17, 2024 at 10:26:46PM -0400, Theodore Ts'o wrote:
> #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?

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

diff --git a/block/blk-settings.c b/block/blk-settings.c
index cdbaef159c4b..c442f7ec3a6b 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -165,11 +165,13 @@ static int blk_validate_limits(struct queue_limits *lim)
 	lim->max_discard_sectors =
 		min(lim->max_hw_discard_sectors, lim->max_user_discard_sectors);
 
-	if (!lim->max_discard_segments)
-		lim->max_discard_segments = 1;
+	if (lim->max_discard_sectors) {
+		if (!lim->max_discard_segments)
+			lim->max_discard_segments = 1;
 
-	if (lim->discard_granularity < lim->physical_block_size)
-		lim->discard_granularity = lim->physical_block_size;
+		if (lim->discard_granularity < lim->physical_block_size)
+			lim->discard_granularity = lim->physical_block_size;
+	}
 
 	/*
 	 * By default there is no limit on the segment boundary alignment,
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index 88114719fe18..e647e1bcd50c 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -1969,6 +1969,7 @@ int dm_table_set_restrictions(struct dm_table *t, struct request_queue *q,
 		blk_queue_flag_clear(QUEUE_FLAG_NOWAIT, q);
 
 	if (!dm_table_supports_discards(t)) {
+		limits->max_user_discard_sectors = 0;
 		limits->max_hw_discard_sectors = 0;
 		limits->discard_granularity = 0;
 		limits->discard_alignment = 0;

Re: dm: use queue_limits_set

[Mike Snitzer]

On Sun, May 19, 2024 at 01:05:40AM -0400, Mike Snitzer wrote:
> Hi Ted,
> 
> On Fri, May 17, 2024 at 10:26:46PM -0400, Theodore Ts'o wrote:
> > #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?
> 
> 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):

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.  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;
+		}
 		/*
 		 * The pool uses the same discard limits as the underlying data
 		 * device.  DM core has already set this up.
@@ -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;
 	}
 }
 

Re: dm: use queue_limits_set

[Christoph Hellwig]

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.

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.

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.

Re: dm: use queue_limits_set

[Mike Snitzer]

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;
 	}
 }
 

Re: dm: use queue_limits_set

[Christoph Hellwig]

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.

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. 

> 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.

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

Re: dm: use queue_limits_set

[Mike Snitzer]

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.
> 
> 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.
 
> > 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?

Re: dm: use queue_limits_set

[Mike Snitzer]

[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

Re: dm: use queue_limits_set

[Christoph Hellwig]

On Mon, May 20, 2024 at 01:17:46PM -0400, Mike Snitzer wrote:
> 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)

Re: dm: use queue_limits_set

[Mike Snitzer]

On Mon, May 20, 2024 at 10:12:37PM +0200, Christoph Hellwig wrote:
> On Mon, May 20, 2024 at 01:17:46PM -0400, Mike Snitzer wrote:
> > 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)

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

Re: dm: use queue_limits_set

[Mike Snitzer]

On Mon, May 20, 2024 at 06:03:11PM -0400, Mike Snitzer wrote:
> On Mon, May 20, 2024 at 10:12:37PM +0200, Christoph Hellwig wrote:
> > On Mon, May 20, 2024 at 01:17:46PM -0400, Mike Snitzer wrote:
> > > 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)
> 
> 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 <snitzer@kernel.org>
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 <tytso@mit.edu>
Fixes: 1c0e720228ad ("dm: use queue_limits_set")
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 drivers/md/dm-cache-target.c | 5 ++---
 drivers/md/dm-clone-target.c | 4 ++--
 drivers/md/dm-log-writes.c   | 2 +-
 drivers/md/dm-snap.c         | 2 +-
 drivers/md/dm-target.c       | 1 -
 drivers/md/dm-thin.c         | 4 ++--
 drivers/md/dm-zero.c         | 1 -
 drivers/md/dm-zoned-target.c | 1 -
 drivers/md/dm.c              | 2 +-
 9 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index 911f73f7ebba..1f0bc1173230 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -3394,8 +3394,8 @@ 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;
 	}
@@ -3404,7 +3404,6 @@ static void set_discard_limits(struct cache *cache, struct queue_limits *limits)
 	 * cache_iterate_devices() is stacking both origin and fast device limits
 	 * but discards aren't passed to fast device, so inherit origin's limits.
 	 */
-	limits->max_discard_sectors = origin_limits->max_discard_sectors;
 	limits->max_hw_discard_sectors = origin_limits->max_hw_discard_sectors;
 	limits->discard_granularity = origin_limits->discard_granularity;
 	limits->discard_alignment = origin_limits->discard_alignment;
diff --git a/drivers/md/dm-clone-target.c b/drivers/md/dm-clone-target.c
index 94b2fc33f64b..2332d9798141 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;
 	}
 
@@ -2059,7 +2060,6 @@ static void set_discard_limits(struct clone *clone, struct queue_limits *limits)
 	 * device limits but discards aren't passed to the source device, so
 	 * inherit destination's limits.
 	 */
-	limits->max_discard_sectors = dest_limits->max_discard_sectors;
 	limits->max_hw_discard_sectors = dest_limits->max_hw_discard_sectors;
 	limits->discard_granularity = dest_limits->discard_granularity;
 	limits->discard_alignment = dest_limits->discard_alignment;
diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c
index f17a6cf2284e..8d7df8303d0a 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 0ace06d1bee3..f40c18da4000 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -2410,7 +2410,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-target.c b/drivers/md/dm-target.c
index 0c4efb0bef8a..652627aea11b 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -249,7 +249,6 @@ static int io_err_iterate_devices(struct dm_target *ti,
 
 static void io_err_io_hints(struct dm_target *ti, struct queue_limits *limits)
 {
-	limits->max_discard_sectors = UINT_MAX;
 	limits->max_hw_discard_sectors = UINT_MAX;
 	limits->discard_granularity = 512;
 }
diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 4793ad2aa1f7..e0528a4f809c 100644
--- a/drivers/md/dm-thin.c
+++ b/drivers/md/dm-thin.c
@@ -4100,7 +4100,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.
@@ -4497,7 +4497,7 @@ 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-zero.c b/drivers/md/dm-zero.c
index 3b13e6eb1aa4..9a0bb623e823 100644
--- a/drivers/md/dm-zero.c
+++ b/drivers/md/dm-zero.c
@@ -61,7 +61,6 @@ static int zero_map(struct dm_target *ti, struct bio *bio)
 
 static void zero_io_hints(struct dm_target *ti, struct queue_limits *limits)
 {
-	limits->max_discard_sectors = UINT_MAX;
 	limits->max_hw_discard_sectors = UINT_MAX;
 	limits->discard_granularity = 512;
 }
diff --git a/drivers/md/dm-zoned-target.c b/drivers/md/dm-zoned-target.c
index 621794a9edd6..12236e6f46f3 100644
--- a/drivers/md/dm-zoned-target.c
+++ b/drivers/md/dm-zoned-target.c
@@ -1001,7 +1001,6 @@ static void dmz_io_hints(struct dm_target *ti, struct queue_limits *limits)
 
 	limits->discard_alignment = 0;
 	limits->discard_granularity = DMZ_BLOCK_SIZE;
-	limits->max_discard_sectors = chunk_sectors;
 	limits->max_hw_discard_sectors = chunk_sectors;
 	limits->max_write_zeroes_sectors = chunk_sectors;
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7d0746b37c8e..3adfc6b83c01 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1086,7 +1086,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_discard_sectors = 0;
+	limits->max_hw_discard_sectors = 0;
 }
 
 void disable_write_zeroes(struct mapped_device *md)
-- 
2.44.0

Re: dm: use queue_limits_set

[Mike Snitzer]

On Mon, May 20, 2024 at 08:45:28PM -0400, Mike Snitzer wrote:
> On Mon, May 20, 2024 at 06:03:11PM -0400, Mike Snitzer wrote:
> > On Mon, May 20, 2024 at 10:12:37PM +0200, Christoph Hellwig wrote:
> > > On Mon, May 20, 2024 at 01:17:46PM -0400, Mike Snitzer wrote:
> > > > 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)
> > 
> > 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 <snitzer@kernel.org>
> 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 <tytso@mit.edu>
> Fixes: 1c0e720228ad ("dm: use queue_limits_set")
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>

With this patch applied, I verified xfstests generic/347 and
generic/405 no longer trigger the dm_cell_key_has_valid_range
WARN_ON_ONCE.  I'm sending the fix to Linus now.

Re: dm: use queue_limits_set

[Mike Snitzer]

On Mon, May 20, 2024 at 11:39:14AM -0400, Mike Snitzer wrote:
> On Mon, May 20, 2024 at 05:06:53PM +0200, Christoph Hellwig wrote:
> 
> > 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;
>  	}
>  }
>  
> 

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

That way thin_io_hints() (and equivalent functions in other DM
targets) just need to be audited/updated to ensure they are setting
both discard_granularity and max_hw_discard_sectors?

Mike

Re: dm: use queue_limits_set

[Christoph Hellwig]

On Mon, May 20, 2024 at 11:47:54AM -0400, Mike Snitzer wrote:
> Maybe update blk_validate_limits() to ensure max_discard_sectors is a
> factor of discard_granularity?

That's probably a good idea.  I'm travelling currently so I'll probably
need a day to two to get to this, feel free to do it yourself if you
are faster.