[PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Julian Sun]

The __GFP_NOFAIL flag ensures that allocation will not fail.
So remove the unnecessary checks.

Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
---
 fs/ext4/extents.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index a07a98a4b97a..95debd5d6506 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2940,10 +2940,6 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
 	} else {
 		path = kcalloc(depth + 1, sizeof(struct ext4_ext_path),
 			       GFP_NOFS | __GFP_NOFAIL);
-		if (path == NULL) {
-			ext4_journal_stop(handle);
-			return -ENOMEM;
-		}
 		path[0].p_maxdepth = path[0].p_depth = depth;
 		path[0].p_hdr = ext_inode_hdr(inode);
 		i = 0;
-- 
2.39.5

Re: [PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Baokun Li]

On 2025/2/28 16:11, Julian Sun wrote:
> The __GFP_NOFAIL flag ensures that allocation will not fail.
> So remove the unnecessary checks.
Actually, even with __GFP_NOFAIL set, kcalloc() can still return NULL,
such as when the input parameters overflow.


Baokun
>
> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
> ---
>   fs/ext4/extents.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a07a98a4b97a..95debd5d6506 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2940,10 +2940,6 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>   	} else {
>   		path = kcalloc(depth + 1, sizeof(struct ext4_ext_path),
>   			       GFP_NOFS | __GFP_NOFAIL);
> -		if (path == NULL) {
> -			ext4_journal_stop(handle);
> -			return -ENOMEM;
> -		}
>   		path[0].p_maxdepth = path[0].p_depth = depth;
>   		path[0].p_hdr = ext_inode_hdr(inode);
>   		i = 0;

Re: [PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Julian Sun]

Hi, Baokun.
Thanks for your review and comments.

On Fri, Feb 28, 2025 at 4:44 PM Baokun Li <libaokun1@huawei.com> wrote:
>
> On 2025/2/28 16:11, Julian Sun wrote:
> > The __GFP_NOFAIL flag ensures that allocation will not fail.
> > So remove the unnecessary checks.
> Actually, even with __GFP_NOFAIL set, kcalloc() can still return NULL,
> such as when the input parameters overflow.
>
Yeah, agreed. But IMO an overflow shouldn’t happen in this situation.

If there's something I'm missing, please let me know.
>
> Baokun
> >
> > Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>
> > ---
> >   fs/ext4/extents.c | 4 ----
> >   1 file changed, 4 deletions(-)
> >
> > diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> > index a07a98a4b97a..95debd5d6506 100644
> > --- a/fs/ext4/extents.c
> > +++ b/fs/ext4/extents.c
> > @@ -2940,10 +2940,6 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
> >       } else {
> >               path = kcalloc(depth + 1, sizeof(struct ext4_ext_path),
> >                              GFP_NOFS | __GFP_NOFAIL);
> > -             if (path == NULL) {
> > -                     ext4_journal_stop(handle);
> > -                     return -ENOMEM;
> > -             }
> >               path[0].p_maxdepth = path[0].p_depth = depth;
> >               path[0].p_hdr = ext_inode_hdr(inode);
> >               i = 0;
>
>

Thanks,
-- 
Julian Sun <sunjunchao2870@gmail.com>

Re: [PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Zhang Yi]

On 2025/2/28 16:11, Julian Sun wrote:
> The __GFP_NOFAIL flag ensures that allocation will not fail.
> So remove the unnecessary checks.
> 
> Signed-off-by: Julian Sun <sunjunchao2870@gmail.com>

Thanks for the cleanup, looks good to me.

Reviewed-by: Zhang Yi <yi.zhang@huawei.com>

> ---
>  fs/ext4/extents.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index a07a98a4b97a..95debd5d6506 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -2940,10 +2940,6 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
>  	} else {
>  		path = kcalloc(depth + 1, sizeof(struct ext4_ext_path),
>  			       GFP_NOFS | __GFP_NOFAIL);
> -		if (path == NULL) {
> -			ext4_journal_stop(handle);
> -			return -ENOMEM;
> -		}
>  		path[0].p_maxdepth = path[0].p_depth = depth;
>  		path[0].p_hdr = ext_inode_hdr(inode);
>  		i = 0;

Re: [PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Theodore Ts'o]

On Fri, Feb 28, 2025 at 05:30:06PM +0800, Julian Sun wrote:
> > Actually, even with __GFP_NOFAIL set, kcalloc() can still return NULL,
> > such as when the input parameters overflow.
> >
> Yeah, agreed. But IMO an overflow shouldn’t happen in this situation.
> 
> If there's something I'm missing, please let me know.

It's not a matter of missing something; or even Right vs Wrong.
Different maintainers have different tastes about this sort of thing.

The mm folks have changed the meaning of __GFP_NOFAIL in the past
(TL;DR: they *hate* that concept, and I wouldn't be surprised if they
try to change its behavior in the future) and especially in large code
bases such as the Linux Kernel, I'm a big believer in defensive
programming.

As Linus has said in a different thread, when a compiler adds warnings
because of what it thinks are "unnecessary" range checks, that's a bad
warning.  Adding extra range checks is never a bad thing, and compiler
behaviour that whine about that sort of thing are.... unfortunate.
Similarly, I'd much rather keep the extra check.

(Also, there exist static program checkers, such as Coverity, that
don't know about the semantics of the GFP_* flags, and so removing the
check would actually cause those tools to complain.)

Cheers,

					- Ted

Re: [PATCH] ext4: remove unnecessary checks for __GFP_NOFAIL allocation.

[Julian Sun]

Hi, Ted.

On Fri, Feb 28, 2025 at 9:34 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Fri, Feb 28, 2025 at 05:30:06PM +0800, Julian Sun wrote:
> > > Actually, even with __GFP_NOFAIL set, kcalloc() can still return NULL,
> > > such as when the input parameters overflow.
> > >
> > Yeah, agreed. But IMO an overflow shouldn’t happen in this situation.
> >
> > If there's something I'm missing, please let me know.
>
> It's not a matter of missing something; or even Right vs Wrong.
> Different maintainers have different tastes about this sort of thing.
>
> The mm folks have changed the meaning of __GFP_NOFAIL in the past
> (TL;DR: they *hate* that concept, and I wouldn't be surprised if they
> try to change its behavior in the future) and especially in large code
> bases such as the Linux Kernel, I'm a big believer in defensive
> programming.
>
> As Linus has said in a different thread, when a compiler adds warnings
> because of what it thinks are "unnecessary" range checks, that's a bad
> warning.  Adding extra range checks is never a bad thing, and compiler
> behaviour that whine about that sort of thing are.... unfortunate.
> Similarly, I'd much rather keep the extra check.
>
> (Also, there exist static program checkers, such as Coverity, that
> don't know about the semantics of the GFP_* flags, and so removing the
> check would actually cause those tools to complain.)
>
Got it. Thanks for your detailed explanation, it makes sense to me.
By the way, I know you're busy, and I’m not trying to rush you, but
when you have some time, could you please take a look at these
patches? Thank you!

https://lore.kernel.org/linux-ext4/20250107044702.1836852-1-sunjunchao2870@gmail.com/

> Cheers,
>
>                                         - Ted


Best Regards,
-- 
Julian Sun <sunjunchao2870@gmail.com>