[PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Luis Chamberlain]

The commit titled "block/bdev: lift block size restrictions to 64k"
lifted the block layer's max supported block size to 64k inside the
helper blk_validate_block_size() now that we support large folios on
the block layer. However, block drivers have relied on the call for
queue_limits_commit_update() to validate and ensure the logical block
size < PAGE_SIZE.

We should take time to validate each block driver before enabling
support for larger logical block sizes, so that those that didn't
have support stay that way and don't need modifications.

Li Wang reported this as a regression on LTP via:

testcases/kernel/syscalls/ioctl/ioctl_loop06

Which uses the loopback driver to enable larger logical block sizes
first with LOOP_CONFIGURE and then LOOP_SET_BLOCK_SIZE. While
I see no reason why the loopback block driver can't support
larger logical block sizes than PAGE_SIZE, leave this validation
step as a secondary effort for each block driver.

Reported-by: Li Wang <liwang@redhat.com>
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202503101538.84c33cd4-lkp@intel.com
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/blk-settings.c   | 4 +++-
 include/linux/blkdev.h | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/blk-settings.c b/block/blk-settings.c
index c44dadc35e1e..5cdd0d7d2af2 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -254,7 +254,9 @@ int blk_validate_limits(struct queue_limits *lim)
 	 */
 	if (!lim->logical_block_size)
 		lim->logical_block_size = SECTOR_SIZE;
-	else if (blk_validate_block_size(lim->logical_block_size)) {
+	else if (blk_validate_block_size(lim->logical_block_size) ||
+		 (lim->logical_block_size > PAGE_SIZE &&
+		   !(lim->features & BLK_FEAT_LBS))) {
 		pr_warn("Invalid logical block size (%d)\n", lim->logical_block_size);
 		return -EINVAL;
 	}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a97428e8bbbe..cdab3731a646 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -341,6 +341,9 @@ typedef unsigned int __bitwise blk_features_t;
 #define BLK_FEAT_ATOMIC_WRITES \
 	((__force blk_features_t)(1u << 16))
 
+/* Supports sector sizes > PAGE_SIZE */
+#define BLK_FEAT_LBS		((__force blk_features_t)(1u << 17))
+
 /*
  * Flags automatically inherited when stacking limits.
  */
-- 
2.47.2

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Christoph Hellwig]

On Tue, Mar 11, 2025 at 10:00:28PM -0700, Luis Chamberlain wrote:
> We should take time to validate each block driver before enabling
> support for larger logical block sizes, so that those that didn't
> have support stay that way and don't need modifications.
> 
> Li Wang reported this as a regression on LTP via:
> 
> testcases/kernel/syscalls/ioctl/ioctl_loop06
> 
> Which uses the loopback driver to enable larger logical block sizes
> first with LOOP_CONFIGURE and then LOOP_SET_BLOCK_SIZE. While
> I see no reason why the loopback block driver can't support
> larger logical block sizes than PAGE_SIZE, leave this validation
> step as a secondary effort for each block driver.

This doesn't really make sense.  We don't want a flag that caps driver
controlled values at a arbitrary value (and then not used it at all in
the patch).

If you need extra per-driver validatation, do it in the driver.

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Luis Chamberlain]

On Wed, Mar 12, 2025 at 06:21:55AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 11, 2025 at 10:00:28PM -0700, Luis Chamberlain wrote:
> > We should take time to validate each block driver before enabling
> > support for larger logical block sizes, so that those that didn't
> > have support stay that way and don't need modifications.
> > 
> > Li Wang reported this as a regression on LTP via:
> > 
> > testcases/kernel/syscalls/ioctl/ioctl_loop06
> > 
> > Which uses the loopback driver to enable larger logical block sizes
> > first with LOOP_CONFIGURE and then LOOP_SET_BLOCK_SIZE. While
> > I see no reason why the loopback block driver can't support
> > larger logical block sizes than PAGE_SIZE, leave this validation
> > step as a secondary effort for each block driver.
> 
> This doesn't really make sense.  We don't want a flag that caps driver
> controlled values at a arbitrary value (and then not used it at all in
> the patch).
> 
> If you need extra per-driver validatation, do it in the driver.

Are you suggesting we just move back the PAGE_SIZE check, or to keep
the checks for the block driver limits into each driver?

  Luis

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Christoph Hellwig]

On Tue, Mar 11, 2025 at 10:37:27PM -0700, Luis Chamberlain wrote:
> > If you need extra per-driver validatation, do it in the driver.
> 
> Are you suggesting we just move back the PAGE_SIZE check,

PAGE_SIZE now is a consumer (i.e. file system) limitation.  Having
a flag in the provider (driver) does not make sense.

> or to keep
> the checks for the block driver limits into each driver?

Most drivers probably don't have a limit other than than that implicit
by the field width used for reporting.  So in general the driver should
not need any checks.  The only exceptions might be for virtual drivers
where the value comes from userspace, but even then it is a bit doubtful.

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Luis Chamberlain]

On Wed, Mar 12, 2025 at 06:40:53AM +0100, Christoph Hellwig wrote:
> On Tue, Mar 11, 2025 at 10:37:27PM -0700, Luis Chamberlain wrote:
> > > If you need extra per-driver validatation, do it in the driver.
> > 
> > Are you suggesting we just move back the PAGE_SIZE check,
> 
> PAGE_SIZE now is a consumer (i.e. file system) limitation.  Having
> a flag in the provider (driver) does not make sense.
> 
> > or to keep
> > the checks for the block driver limits into each driver?
> 
> Most drivers probably don't have a limit other than than that implicit
> by the field width used for reporting.  So in general the driver should
> not need any checks.  The only exceptions might be for virtual drivers
> where the value comes from userspace, but even then it is a bit doubtful.

Alrighty, so silly tests just need to be updated. If a hang is reported,
we can look into it, or just add block driver checks / limitations.

  Luis

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Christoph Hellwig]

On Wed, Mar 12, 2025 at 05:19:36PM +0800, Li Wang wrote:
> Well, does that patch for ioctl_loop06 still make sense?
> Or any other workaround?
> https://lists.linux.it/pipermail/ltp/2025-March/042599.html

The real question is what block sizes we want to support for the
loop driver.  Because if it is larger than the physical block size
it can lead to torn writes.  But I guess no one cared about those
on loop so far, so why care about this now..

But if we don't want any limit on the block size that patch looks
right.

Re: [PATCH] block: add BLK_FEAT_LBS to check for PAGE_SIZE limit

[Hannes Reinecke]

On 3/13/25 03:54, Li Wang wrote:
> 
> 
> On Wed, Mar 12, 2025 at 9:59 PM Christoph Hellwig <hch@lst.de 
> <mailto:hch@lst.de>> wrote:
> 
>     On Wed, Mar 12, 2025 at 05:19:36PM +0800, Li Wang wrote:
>      > Well, does that patch for ioctl_loop06 still make sense?
>      > Or any other workaround?
>      > https://lists.linux.it/pipermail/ltp/2025-March/042599.html
>     <https://lists.linux.it/pipermail/ltp/2025-March/042599.html>
> 
>     The real question is what block sizes we want to support for the
>     loop driver.  Because if it is larger than the physical block size
>     it can lead to torn writes.  But I guess no one cared about those
>     on loop so far, so why care about this now..
> 
> 
> That's because the kernel test-robot reports a LTP/ioctl_loop06 test
> fail in kernel commit:
>    47dd67532303803  ("block/bdev: lift block size restrictions to 64k")
> 
> The ioctl_loop06 is a boundary testing and always fail with
> LOOP_SET_BLOCK_SIZE set a value larger than PAGE_SIZE.
> But now it's set successfully unexpectedly.
> 
> If you all believe the boundary test for loopback driver is redundant,
> I can help remove that from LTP code.
> 
I would remove it.
Yes, we might incur torn writes, but previously we hadn't cared about 
that. And if we cared we should have a dedicated test for that; there's
no guarantee that we cannot have torn writes even with 4k blocks.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich