diff mbox series

[RFC,6/8] block/bdev: lift block size restrictions and use common definition

Message ID 20241113094727.1497722-7-mcgrof@kernel.org (mailing list archive)
State New
Headers show
Series enable bs > ps for block devices | expand

Commit Message

Luis Chamberlain Nov. 13, 2024, 9:47 a.m. UTC
We now can support blocksizes larger than PAGE_SIZE, so lift
the restriction up to the max supported page cache order and
just bake this into a common helper used by the block layer.

We bound ourselves to 64k, because beyond that we need more testing.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/bdev.c           | 5 ++---
 include/linux/blkdev.h | 6 +++++-
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Hannes Reinecke Nov. 13, 2024, 9:57 a.m. UTC | #1
On 11/13/24 10:47, Luis Chamberlain wrote:
> We now can support blocksizes larger than PAGE_SIZE, so lift
> the restriction up to the max supported page cache order and
> just bake this into a common helper used by the block layer.
> 
> We bound ourselves to 64k, because beyond that we need more testing.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   block/bdev.c           | 5 ++---
>   include/linux/blkdev.h | 6 +++++-
>   2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index 167d82b46781..3a5fd65f6c8e 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -157,8 +157,7 @@ int set_blocksize(struct file *file, int size)
>   	struct inode *inode = file->f_mapping->host;
>   	struct block_device *bdev = I_BDEV(inode);
>   
> -	/* Size must be a power of two, and between 512 and PAGE_SIZE */
> -	if (size > PAGE_SIZE || size < 512 || !is_power_of_2(size))
> +	if (blk_validate_block_size(size))
>   		return -EINVAL;
>   
>   	/* Size cannot be smaller than the size supported by the device */
> @@ -185,7 +184,7 @@ int sb_set_blocksize(struct super_block *sb, int size)
>   	if (set_blocksize(sb->s_bdev_file, size))
>   		return 0;
>   	/* If we get here, we know size is power of two
> -	 * and it's value is between 512 and PAGE_SIZE */
> +	 * and it's value is larger than 512 */
>   	sb->s_blocksize = size;
>   	sb->s_blocksize_bits = blksize_bits(size);
>   	return sb->s_blocksize;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 50c3b959da28..cc9fca1fceaa 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -25,6 +25,7 @@
>   #include <linux/uuid.h>
>   #include <linux/xarray.h>
>   #include <linux/file.h>
> +#include <linux/pagemap.h>
>   
>   struct module;
>   struct request_queue;
> @@ -268,10 +269,13 @@ static inline dev_t disk_devt(struct gendisk *disk)
>   	return MKDEV(disk->major, disk->first_minor);
>   }
>   
> +/* We should strive for 1 << (PAGE_SHIFT + MAX_PAGECACHE_ORDER) */
> +#define BLK_MAX_BLOCK_SIZE      (SZ_64K)
> +

Please make the comment a bit more descriptive, indicating that beyond 
64k more testing is required, hence it's not enabled for now.

We _could_ add a config option to make this conditional...

>   /* blk_validate_limits() validates bsize, so drivers don't usually need to */
>   static inline int blk_validate_block_size(unsigned long bsize)
>   {
> -	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
> +	if (bsize < 512 || bsize > BLK_MAX_BLOCK_SIZE || !is_power_of_2(bsize))
>   		return -EINVAL;
>   
>   	return 0;
Cheers,

Hannes
Matthew Wilcox Nov. 13, 2024, 2:14 p.m. UTC | #2
On Wed, Nov 13, 2024 at 01:47:25AM -0800, Luis Chamberlain wrote:
> @@ -185,7 +184,7 @@ int sb_set_blocksize(struct super_block *sb, int size)
>  	if (set_blocksize(sb->s_bdev_file, size))
>  		return 0;
>  	/* If we get here, we know size is power of two
> -	 * and it's value is between 512 and PAGE_SIZE */
> +	 * and it's value is larger than 512 */

If you're changing this line, please delete the incorrect apostrophe.

>  	sb->s_blocksize = size;
>  	sb->s_blocksize_bits = blksize_bits(size);
>  	return sb->s_blocksize;
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 50c3b959da28..cc9fca1fceaa 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -25,6 +25,7 @@
>  #include <linux/uuid.h>
>  #include <linux/xarray.h>
>  #include <linux/file.h>
> +#include <linux/pagemap.h>

Why do we need to add this include?

> @@ -268,10 +269,13 @@ static inline dev_t disk_devt(struct gendisk *disk)
>  	return MKDEV(disk->major, disk->first_minor);
>  }
>  
> +/* We should strive for 1 << (PAGE_SHIFT + MAX_PAGECACHE_ORDER) */
> +#define BLK_MAX_BLOCK_SIZE      (SZ_64K)

I think we need CONFIG_TRANSPARENT_HUGEPAGE to go over PAGE_SIZE.
John Garry Nov. 18, 2024, 9:18 a.m. UTC | #3
On 13/11/2024 09:47, Luis Chamberlain wrote:
>   #include <linux/uuid.h>
>   #include <linux/xarray.h>
>   #include <linux/file.h>
> +#include <linux/pagemap.h>
>   
>   struct module;
>   struct request_queue;
> @@ -268,10 +269,13 @@ static inline dev_t disk_devt(struct gendisk *disk)
>   	return MKDEV(disk->major, disk->first_minor);
>   }
>   
> +/* We should strive for 1 << (PAGE_SHIFT + MAX_PAGECACHE_ORDER) */

I fell that this comment can be reworked.

I think that what we want to say is that hard limit is 1 << (PAGE_SHIFT 
+ MAX_PAGECACHE_ORDER), but we set at sensible size of 64K.

> +#define BLK_MAX_BLOCK_SIZE      (SZ_64K)
> +
diff mbox series

Patch

diff --git a/block/bdev.c b/block/bdev.c
index 167d82b46781..3a5fd65f6c8e 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -157,8 +157,7 @@  int set_blocksize(struct file *file, int size)
 	struct inode *inode = file->f_mapping->host;
 	struct block_device *bdev = I_BDEV(inode);
 
-	/* Size must be a power of two, and between 512 and PAGE_SIZE */
-	if (size > PAGE_SIZE || size < 512 || !is_power_of_2(size))
+	if (blk_validate_block_size(size))
 		return -EINVAL;
 
 	/* Size cannot be smaller than the size supported by the device */
@@ -185,7 +184,7 @@  int sb_set_blocksize(struct super_block *sb, int size)
 	if (set_blocksize(sb->s_bdev_file, size))
 		return 0;
 	/* If we get here, we know size is power of two
-	 * and it's value is between 512 and PAGE_SIZE */
+	 * and it's value is larger than 512 */
 	sb->s_blocksize = size;
 	sb->s_blocksize_bits = blksize_bits(size);
 	return sb->s_blocksize;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 50c3b959da28..cc9fca1fceaa 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -25,6 +25,7 @@ 
 #include <linux/uuid.h>
 #include <linux/xarray.h>
 #include <linux/file.h>
+#include <linux/pagemap.h>
 
 struct module;
 struct request_queue;
@@ -268,10 +269,13 @@  static inline dev_t disk_devt(struct gendisk *disk)
 	return MKDEV(disk->major, disk->first_minor);
 }
 
+/* We should strive for 1 << (PAGE_SHIFT + MAX_PAGECACHE_ORDER) */
+#define BLK_MAX_BLOCK_SIZE      (SZ_64K)
+
 /* blk_validate_limits() validates bsize, so drivers don't usually need to */
 static inline int blk_validate_block_size(unsigned long bsize)
 {
-	if (bsize < 512 || bsize > PAGE_SIZE || !is_power_of_2(bsize))
+	if (bsize < 512 || bsize > BLK_MAX_BLOCK_SIZE || !is_power_of_2(bsize))
 		return -EINVAL;
 
 	return 0;