Re: [PATCH 2/2] nvme: drop WARN_ON_ONCE on write_stream bounds check

[Keith Busch]

On Mon, Jul 27, 2026 at 09:19:27PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jul 27, 2026 at 08:24:40AM -0600, Keith Busch wrote:
> > On Sat, Jul 25, 2026 at 03:51:11PM +0200, Hari Mishal wrote:
> > > write_stream is validated against bdev_max_write_streams() in both
> > > generic block direct I/O (block/fops.c) and F2FS before a bio
> > > carrying it is ever built, so write_stream > nr_plids shouldn't be
> > > reachable through any current legitimate path. The remaining users
> > > of bio->bi_write_stream elsewhere in the block layer only copy an
> > > already-validated value between bios (bio.c, blk-crypto-fallback.c)
> > > or compare it for merge eligibility (blk-merge.c); none of them
> > > introduce a new, unvalidated value.
> > > 
> > > Using WARN_ON_ONCE as the backstop for that assumption isn't worth
> > > it given how many deployed systems run with panic-on-warn enabled;
> > > the existing graceful return BLK_STS_INVAL already handles it on
> > > its own.
> > 
> > That's not a very good reason to remove a WARN_ON. You've left the check
> > in for a condition that should never happen, so when it does happen,
> > it'll be impossible to debug without the WARN.
> > 
> > And the WARN also annotates the branch as unlikely, which is desirable
> > for this case.
> 
> But, if it ever does happen, a WARN_ON will reboot the box, given that
> billions of Linux systems have panic-on-warn enabled.  

So WARN_ON is the new BUG_ON now? If the condition happens we need to
know how we got here and make it obvious something is wrong. So I guess
we'd have to replace every one of these:

	if (WARN_ON_ONCE(condition)) ...

With an open-coded version like:

	if (unlikely(condition)) {
		do_once(dump_stack());
		...
	}

?

That doesn't seem right, so if that is the suggestion, then I think we
need a new macro to provide the result that the WARN_ON usage expected.

> So if this can ever happen, 

But it can't ever happen. This patch's commit message reasoned that as
justification to remove the warn, but we need to know how we got here
when it does happen because it means somebody broke contract.

> just properly handle it and recover and don't loose user
> data.

We can't save the data from this specific condition: the data from the
request is unwritable and lost. We've also learned that EINVAL errors
are not handled for many DM stacking drivers in very bad ways, so again,
we need to know how we got here when something breaks the API contract
otherwise it'll be a difficult problem to debug without that visibility.