[PATCH -next] ext4: Remove redundant null pointer check

[PATCH -next] ext4: Remove redundant null pointer check

[Li Zetao]

Since the ext4_find_extent() does not return a null pointer, the check for
the null pointer here is redundant. Drop null pointer check for clean
code.

No functional change intended.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
 fs/ext4/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e067f2dd0335..12f0771d57d2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -6112,7 +6112,7 @@ int ext4_ext_clear_bb(struct inode *inode)
 			break;
 		if (ret > 0) {
 			path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
-			if (!IS_ERR_OR_NULL(path)) {
+			if (!IS_ERR(path)) {
 				for (j = 0; j < path->p_depth; j++) {
 
 					ext4_mb_mark_bb(inode->i_sb,
-- 
2.34.1

Re: [PATCH -next] ext4: Remove redundant null pointer check

[Baokun Li]

Hi Zetao,

On 2024/8/20 9:32, Li Zetao wrote:
> Since the ext4_find_extent() does not return a null pointer, the check for
> the null pointer here is redundant. Drop null pointer check for clean
> code.
>
> No functional change intended.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
>   fs/ext4/extents.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e067f2dd0335..12f0771d57d2 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -6112,7 +6112,7 @@ int ext4_ext_clear_bb(struct inode *inode)
>   			break;
>   		if (ret > 0) {
>   			path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
> -			if (!IS_ERR_OR_NULL(path)) {
> +			if (!IS_ERR(path)) {
>   				for (j = 0; j < path->p_depth; j++) {
>   
>   					ext4_mb_mark_bb(inode->i_sb,

Thanks for the cleanup patch.

But the change is already included in the patch:

  https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/


Thanks,
Baokun

Re: [PATCH -next] ext4: Remove redundant null pointer check

[Theodore Ts'o]

On Tue, Sep 03, 2024 at 03:52:01PM +0800, Baokun Li wrote:
> Thanks for the cleanup patch.
> 
> But the change is already included in the patch:
> 
>  https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/

Yeah, I noticed.  I had already applied Zetao's patch when I processed
yours, so I just ended up manually handling the patch conflict.

(I haven't set out the patch acks yet, because the current state of
the ext4/dev branch is apparently causing a test regression which I'm
trying to root cause.  They will be in tomorrow's fs-next and
linux-next branch, though unless I end up figuring out the problematic
patch or patch series, and end up dropping them from the ext4 dev
branch today.  Still, feel free to take a look and let me know if I
screwed up anything.)

						- Ted

Re: [PATCH -next] ext4: Remove redundant null pointer check

[Baokun Li]

Hi Theodore,

Unfortunately it is possible that the regression was introduced
precisely by the mishandling of conflicts here.

On 2024/9/3 20:50, Theodore Ts'o wrote:
> On Tue, Sep 03, 2024 at 03:52:01PM +0800, Baokun Li wrote:
>> Thanks for the cleanup patch.
>>
>> But the change is already included in the patch:
>>
>>   https://lore.kernel.org/all/20240710040654.1714672-21-libaokun@huaweicloud.com/
> Yeah, I noticed.  I had already applied Zetao's patch when I processed
> yours, so I just ended up manually handling the patch conflict.
>
> (I haven't set out the patch acks yet, because the current state of
> the ext4/dev branch is apparently causing a test regression which I'm
> trying to root cause.  They will be in tomorrow's fs-next and
> linux-next branch, though unless I end up figuring out the problematic
> patch or patch series, and end up dropping them from the ext4 dev
> branch today.  Still, feel free to take a look and let me know if I
> screwed up anything.)
>
> 						- Ted
>

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=92bbb922166cd7a829bf60d168372ffa1c54e81d

The changes after resolving the conflict in ext4_ext_clear_bb() are
as follows:

---------------------------------------------------------------------------

@@ -6128,12 +6122,9 @@ int ext4_ext_clear_bb(struct inode *inode)
         if (IS_ERR(path))
                 return PTR_ERR(path);
         ex = path[path->p_depth].p_ext;
-       if (!ex) {
-               ext4_free_ext_path(path);
-               return 0;
-       }
+       if (!ex)
+               goto out;
         end = le32_to_cpu(ex->ee_block) + ext4_ext_get_actual_len(ex);
-       ext4_free_ext_path(path);

         cur = 0;
         while (cur < end) {
@@ -6146,7 +6137,6 @@ int ext4_ext_clear_bb(struct inode *inode)
                         path = ext4_find_extent(inode, map.m_lblk, 
NULL, 0);
                         if (!IS_ERR(path)) {
                                 for (j = 0; j < path->p_depth; j++) {
-
ext4_mb_mark_bb(inode->i_sb,
path[j].p_block, 1, false);
ext4_fc_record_regions(inode->i_sb, inode->i_ino,
@@ -6161,5 +6151,7 @@ int ext4_ext_clear_bb(struct inode *inode)
                 cur = cur + map.m_len;
         }

+out:
+       ext4_free_ext_path(path);
         return 0;
  }

---------------------------------------------------------------------------

This can cause path leaks and path double free.

----------------------------------------

     path_A = ext4_find_extent
     while (cur < end) {
             path_B = ext4_find_extent(inode, map.m_lblk, NULL, 0);
             // path_A leak
             ext4_free_ext_path(path_B);
     }
out:
     ext4_free_ext_path(path_B); // path_B double free

----------------------------------------

I think it's best to drop Zetao's patch
   b2e662cb86ca ("ext4: remove redundant null pointer check")
and then reapply the conflicting patch
  92bbb922166c ("ext4: make some fast commit functions reuse extents path")

Or apply the following modifications to conflicting patch in the tree:

---------------------------------------------------------------------------

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 789316f22f97..34e25eee6521 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -6132,7 +6132,7 @@ int ext4_ext_clear_bb(struct inode *inode)
                 if (ret < 0)
                         break;
                 if (ret > 0) {
-                       path = ext4_find_extent(inode, map.m_lblk, NULL, 0);
+                       path = ext4_find_extent(inode, map.m_lblk, path, 0);
                         if (!IS_ERR(path)) {
                                 for (j = 0; j < path->p_depth; j++) {
ext4_mb_mark_bb(inode->i_sb,
@@ -6140,7 +6140,8 @@ int ext4_ext_clear_bb(struct inode *inode)
ext4_fc_record_regions(inode->i_sb, inode->i_ino,
                                                         0, 
path[j].p_block, 1, 1);
                                 }
-                               ext4_free_ext_path(path);
+                       } else {
+                               path = NULL;
                         }
                         ext4_mb_mark_bb(inode->i_sb, map.m_pblk, 
map.m_len, false);
                         ext4_fc_record_regions(inode->i_sb, inode->i_ino,

---------------------------------------------------------------------------


Cheers,
Baokun