[v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

[PATCH v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

Posted by Pankaj Raghav 3 months, 2 weeks ago

All filesystems will already check the max and min value of their block
size during their initialization. __getblk_slow() is a very low-level
function to have these checks. Remove them and only check for logical
block size alignment.

As this check with logical block size alignment might never trigger, add
WARN_ON_ONCE() to the check. As WARN_ON_ONCE() will already print the
stack, remove the call to dump_stack().

Suggested-by: Matthew Wilcox <willy@infradead.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
Changes since v3:
- Use WARN_ON_ONCE on the logical block size check and remove the call
  to dump_stack.
- Use IS_ALIGNED() to check for aligned instead of open coding the
  check.

 fs/buffer.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index d61073143127..565fe88773c2 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1122,14 +1122,9 @@ __getblk_slow(struct block_device *bdev, sector_t block,
 {
 	bool blocking = gfpflags_allow_blocking(gfp);
 
-	if (unlikely(size & (bdev_logical_block_size(bdev) - 1) ||
-		     (size < 512 || size > PAGE_SIZE))) {
-		printk(KERN_ERR "getblk(): invalid block size %d requested\n",
-					size);
-		printk(KERN_ERR "logical block size: %d\n",
-					bdev_logical_block_size(bdev));
-
-		dump_stack();
+	if (WARN_ON_ONCE(!IS_ALIGNED(size, bdev_logical_block_size(bdev)))) {
+		printk(KERN_ERR "getblk(): block size %d not aligned to logical block size %d\n",
+		       size, bdev_logical_block_size(bdev));
 		return NULL;
 	}
 

base-commit: b39f7d75dc41b5f5d028192cd5d66cff71179f35
-- 
2.49.0

Re: [PATCH v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

Posted by Christian Brauner 3 months, 1 week ago

On Thu, 26 Jun 2025 13:32:23 +0200, Pankaj Raghav wrote:
> All filesystems will already check the max and min value of their block
> size during their initialization. __getblk_slow() is a very low-level
> function to have these checks. Remove them and only check for logical
> block size alignment.
> 
> As this check with logical block size alignment might never trigger, add
> WARN_ON_ONCE() to the check. As WARN_ON_ONCE() will already print the
> stack, remove the call to dump_stack().
> 
> [...]

Applied to the vfs-6.17.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.17.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.17.misc

[1/1] fs/buffer: remove the min and max limit checks in __getblk_slow()
      https://git.kernel.org/vfs/vfs/c/36996c013faf

Re: [PATCH v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

Posted by Baokun Li 3 months, 1 week ago

On 2025/6/26 19:32, Pankaj Raghav wrote:
> All filesystems will already check the max and min value of their block
> size during their initialization. __getblk_slow() is a very low-level
> function to have these checks. Remove them and only check for logical
> block size alignment.
>
> As this check with logical block size alignment might never trigger, add
> WARN_ON_ONCE() to the check. As WARN_ON_ONCE() will already print the
> stack, remove the call to dump_stack().
>
> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Makes sense. Feel free to add:

Reviewed-by: Baokun Li <libaokun1@huawei.com>

> ---
> Changes since v3:
> - Use WARN_ON_ONCE on the logical block size check and remove the call
>    to dump_stack.
> - Use IS_ALIGNED() to check for aligned instead of open coding the
>    check.
>
>   fs/buffer.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index d61073143127..565fe88773c2 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1122,14 +1122,9 @@ __getblk_slow(struct block_device *bdev, sector_t block,
>   {
>   	bool blocking = gfpflags_allow_blocking(gfp);
>   
> -	if (unlikely(size & (bdev_logical_block_size(bdev) - 1) ||
> -		     (size < 512 || size > PAGE_SIZE))) {
> -		printk(KERN_ERR "getblk(): invalid block size %d requested\n",
> -					size);
> -		printk(KERN_ERR "logical block size: %d\n",
> -					bdev_logical_block_size(bdev));
> -
> -		dump_stack();
> +	if (WARN_ON_ONCE(!IS_ALIGNED(size, bdev_logical_block_size(bdev)))) {
> +		printk(KERN_ERR "getblk(): block size %d not aligned to logical block size %d\n",
> +		       size, bdev_logical_block_size(bdev));
>   		return NULL;
>   	}
>   
>
> base-commit: b39f7d75dc41b5f5d028192cd5d66cff71179f35

Re: [PATCH v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

Posted by David Laight 3 months, 1 week ago

On Fri, 27 Jun 2025 10:02:30 +0800
Baokun Li <libaokun1@huawei.com> wrote:

> On 2025/6/26 19:32, Pankaj Raghav wrote:
> > All filesystems will already check the max and min value of their block
> > size during their initialization. __getblk_slow() is a very low-level
> > function to have these checks. Remove them and only check for logical
> > block size alignment.
> >
> > As this check with logical block size alignment might never trigger, add
> > WARN_ON_ONCE() to the check. As WARN_ON_ONCE() will already print the
> > stack, remove the call to dump_stack().
> >
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>  
> 
> Makes sense. Feel free to add:
> 
> Reviewed-by: Baokun Li <libaokun1@huawei.com>
> 
> > ---
> > Changes since v3:
> > - Use WARN_ON_ONCE on the logical block size check and remove the call
> >    to dump_stack.
> > - Use IS_ALIGNED() to check for aligned instead of open coding the
> >    check.
> >
> >   fs/buffer.c | 11 +++--------
> >   1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index d61073143127..565fe88773c2 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1122,14 +1122,9 @@ __getblk_slow(struct block_device *bdev, sector_t block,
> >   {
> >   	bool blocking = gfpflags_allow_blocking(gfp);
> >   
> > -	if (unlikely(size & (bdev_logical_block_size(bdev) - 1) ||
> > -		     (size < 512 || size > PAGE_SIZE))) {
> > -		printk(KERN_ERR "getblk(): invalid block size %d requested\n",
> > -					size);
> > -		printk(KERN_ERR "logical block size: %d\n",
> > -					bdev_logical_block_size(bdev));
> > -
> > -		dump_stack();
> > +	if (WARN_ON_ONCE(!IS_ALIGNED(size, bdev_logical_block_size(bdev)))) {
> > +		printk(KERN_ERR "getblk(): block size %d not aligned to logical block size %d\n",
> > +		       size, bdev_logical_block_size(bdev));
> >   		return NULL;

Shouldn't that use WARN_ONCE(condition, fmt, ...)

	David
 
> >   	}
> >   
> >
> > base-commit: b39f7d75dc41b5f5d028192cd5d66cff71179f35  
> 
> 
>

Re: [PATCH v4] fs/buffer: remove the min and max limit checks in __getblk_slow()

Posted by Pankaj Raghav (Samsung) 3 months, 1 week ago

On Sun, Jun 29, 2025 at 11:15:06AM +0100, David Laight wrote:
> On Fri, 27 Jun 2025 10:02:30 +0800
> Baokun Li <libaokun1@huawei.com> wrote:
> 
> > On 2025/6/26 19:32, Pankaj Raghav wrote:
> > > All filesystems will already check the max and min value of their block
> > > size during their initialization. __getblk_slow() is a very low-level
> > > function to have these checks. Remove them and only check for logical
> > > block size alignment.
> > >
> > > As this check with logical block size alignment might never trigger, add
> > > WARN_ON_ONCE() to the check. As WARN_ON_ONCE() will already print the
> > > stack, remove the call to dump_stack().
> > >
> > > Suggested-by: Matthew Wilcox <willy@infradead.org>
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>  
> > 
> > Makes sense. Feel free to add:
> > 
> > Reviewed-by: Baokun Li <libaokun1@huawei.com>
> > 
> > > ---
> > > Changes since v3:
> > > - Use WARN_ON_ONCE on the logical block size check and remove the call
> > >    to dump_stack.
> > > - Use IS_ALIGNED() to check for aligned instead of open coding the
> > >    check.
> > >
> > >   fs/buffer.c | 11 +++--------
> > >   1 file changed, 3 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/buffer.c b/fs/buffer.c
> > > index d61073143127..565fe88773c2 100644
> > > --- a/fs/buffer.c
> > > +++ b/fs/buffer.c
> > > @@ -1122,14 +1122,9 @@ __getblk_slow(struct block_device *bdev, sector_t block,
> > >   {
> > >   	bool blocking = gfpflags_allow_blocking(gfp);
> > >   
> > > -	if (unlikely(size & (bdev_logical_block_size(bdev) - 1) ||
> > > -		     (size < 512 || size > PAGE_SIZE))) {
> > > -		printk(KERN_ERR "getblk(): invalid block size %d requested\n",
> > > -					size);
> > > -		printk(KERN_ERR "logical block size: %d\n",
> > > -					bdev_logical_block_size(bdev));
> > > -
> > > -		dump_stack();
> > > +	if (WARN_ON_ONCE(!IS_ALIGNED(size, bdev_logical_block_size(bdev)))) {
> > > +		printk(KERN_ERR "getblk(): block size %d not aligned to logical block size %d\n",
> > > +		       size, bdev_logical_block_size(bdev));
> > >   		return NULL;
> 
> Shouldn't that use WARN_ONCE(condition, fmt, ...)

We need to return NULL if the check fails. So having the condition and
the format as an `if` condition does not look nice. Plus the formatting
will look weird.

--
Pankaj