[PATCH 0/7] Fixes and cleanups to ext4 ialloc

[PATCH 0/7] Fixes and cleanups to ext4 ialloc

[Kemeng Shi]

This series contains some random fixes and cleanups to ext4 ialloc. More
details can be found in respective patches. Thanks!

Kemeng Shi (7):
  ext4: avoid buffer_head leak in ext4_mark_inode_used
  ext4: avoid potential buffer_head leak in __ext4_new_inode
  ext4: avoid negative min_clusters in find_group_orlov
  ext4: remove dead check in __ext4_new_inode
  ext4: move checksum length calculation of inode bitmap into
    ext4_inode_bitmap_csum_[verify/set] functions
  ext4: remove unneeded NULL check of buffer_head in
    ext4_mark_inode_used
  ext4: check buffer_verified in advance to avoid unneeded
    ext4_get_group_info

 fs/ext4/bitmap.c |  8 ++++++--
 fs/ext4/ext4.h   |  4 ++--
 fs/ext4/ialloc.c | 38 +++++++++++++++++++-------------------
 fs/ext4/resize.c |  3 +--
 4 files changed, 28 insertions(+), 25 deletions(-)

-- 
2.30.0

[PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Kemeng Shi]

Release inode_bitmap_bh from ext4_read_inode_bitmap in
ext4_mark_inode_used to avoid buffer_head leak.
By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
is NULL.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 9dfd768ed9f8..ad7f13976dc6 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -755,10 +755,10 @@ int ext4_mark_inode_used(struct super_block *sb, int ino)
 	struct ext4_group_desc *gdp;
 	ext4_group_t group;
 	int bit;
-	int err = -EFSCORRUPTED;
+	int err;
 
 	if (ino < EXT4_FIRST_INO(sb) || ino > max_ino)
-		goto out;
+		return -EFSCORRUPTED;
 
 	group = (ino - 1) / EXT4_INODES_PER_GROUP(sb);
 	bit = (ino - 1) % EXT4_INODES_PER_GROUP(sb);
@@ -860,6 +860,7 @@ int ext4_mark_inode_used(struct super_block *sb, int ino)
 	err = ext4_handle_dirty_metadata(NULL, NULL, group_desc_bh);
 	sync_dirty_buffer(group_desc_bh);
 out:
+	brelse(inode_bitmap_bh);
 	return err;
 }
 
-- 
2.30.0

[PATCH 2/7] ext4: avoid potential buffer_head leak in __ext4_new_inode

[Kemeng Shi]

If a group is marked EXT4_GROUP_INFO_IBITMAP_CORRUPT after it's inode
bitmap buffer_head was successfully verified, then __ext4_new_inode
will get a valid inode_bitmap_bh of a corrupted group from
ext4_read_inode_bitmap in which case inode_bitmap_bh misses a release.
Hnadle "IS_ERR(inode_bitmap_bh)" and group corruption separately like
how ext4_free_inode does to avoid buffer_head leak.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index ad7f13976dc6..f24a238b6b09 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1054,9 +1054,13 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		brelse(inode_bitmap_bh);
 		inode_bitmap_bh = ext4_read_inode_bitmap(sb, group);
 		/* Skip groups with suspicious inode tables */
-		if (((!(sbi->s_mount_state & EXT4_FC_REPLAY))
-		     && EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) ||
-		    IS_ERR(inode_bitmap_bh)) {
+		if (IS_ERR(inode_bitmap_bh)) {
+			inode_bitmap_bh = NULL;
+			goto next_group;
+		}
+		if (!(sbi->s_mount_state & EXT4_FC_REPLAY) &&
+		    EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
+			brelse(inode_bitmap_bh);
 			inode_bitmap_bh = NULL;
 			goto next_group;
 		}
-- 
2.30.0

[PATCH 3/7] ext4: avoid negative min_clusters in find_group_orlov

[Kemeng Shi]

min_clusters is signed integer and will be converted to unsigned
integer when compared with unsigned number stats.free_clusters.
If min_clusters is negative, it will be converted to a huge unsigned
value in which case all groups may not meet the actual desired free
clusters.
Set negative min_clusters to 0 to avoid unexpected behavior.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index f24a238b6b09..df41aa079cb5 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -514,6 +514,8 @@ static int find_group_orlov(struct super_block *sb, struct inode *parent,
 	if (min_inodes < 1)
 		min_inodes = 1;
 	min_clusters = avefreec - EXT4_CLUSTERS_PER_GROUP(sb)*flex_size / 4;
+	if (min_clusters < 0)
+		min_clusters = 0;
 
 	/*
 	 * Start looking in the flex group where we last allocated an
-- 
2.30.0

[PATCH 4/7] ext4: remove dead check in __ext4_new_inode

[Kemeng Shi]

If we can't grab any inode, the prvious find_inode_bit will set ino
to be >= EXT4_INODES_PER_GROUP(sb). So the check of need to repeat
in the same group is not needed.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index df41aa079cb5..f446588af368 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -1067,7 +1067,6 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 			goto next_group;
 		}
 
-repeat_in_this_group:
 		ret2 = find_inode_bit(sb, group, inode_bitmap_bh, &ino);
 		if (!ret2)
 			goto next_group;
@@ -1117,8 +1116,6 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		if (!ret2)
 			goto got; /* we grabbed the inode! */
 
-		if (ino < EXT4_INODES_PER_GROUP(sb))
-			goto repeat_in_this_group;
 next_group:
 		if (++group == ngroups)
 			group = 0;
-- 
2.30.0

[PATCH 5/7] ext4: move checksum length calculation of inode bitmap into ext4_inode_bitmap_csum_[verify/set] functions

[Kemeng Shi]

There are some little improve:
1. remove repeat code to calculate checksum length of inode bitmap
2. remove unnecessary checksum length calculation if checksum is not
enabled.
3. use more efficient bit shift operation instead of div opreation.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/bitmap.c |  8 ++++++--
 fs/ext4/ext4.h   |  4 ++--
 fs/ext4/ialloc.c | 12 ++++--------
 fs/ext4/resize.c |  3 +--
 4 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/fs/ext4/bitmap.c b/fs/ext4/bitmap.c
index cd725bebe69e..2a135075468d 100644
--- a/fs/ext4/bitmap.c
+++ b/fs/ext4/bitmap.c
@@ -18,15 +18,17 @@ unsigned int ext4_count_free(char *bitmap, unsigned int numchars)
 
 int ext4_inode_bitmap_csum_verify(struct super_block *sb,
 				  struct ext4_group_desc *gdp,
-				  struct buffer_head *bh, int sz)
+				  struct buffer_head *bh)
 {
 	__u32 hi;
 	__u32 provided, calculated;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int sz;
 
 	if (!ext4_has_metadata_csum(sb))
 		return 1;
 
+	sz = EXT4_INODES_PER_GROUP(sb) >> 3;
 	provided = le16_to_cpu(gdp->bg_inode_bitmap_csum_lo);
 	calculated = ext4_chksum(sbi, sbi->s_csum_seed, (__u8 *)bh->b_data, sz);
 	if (sbi->s_desc_size >= EXT4_BG_INODE_BITMAP_CSUM_HI_END) {
@@ -40,14 +42,16 @@ int ext4_inode_bitmap_csum_verify(struct super_block *sb,
 
 void ext4_inode_bitmap_csum_set(struct super_block *sb,
 				struct ext4_group_desc *gdp,
-				struct buffer_head *bh, int sz)
+				struct buffer_head *bh)
 {
 	__u32 csum;
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int sz;
 
 	if (!ext4_has_metadata_csum(sb))
 		return;
 
+	sz = EXT4_INODES_PER_GROUP(sb) >> 3;
 	csum = ext4_chksum(sbi, sbi->s_csum_seed, (__u8 *)bh->b_data, sz);
 	gdp->bg_inode_bitmap_csum_lo = cpu_to_le16(csum & 0xFFFF);
 	if (sbi->s_desc_size >= EXT4_BG_INODE_BITMAP_CSUM_HI_END)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index efed7f09876d..7494b2abb794 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2693,10 +2693,10 @@ struct mmpd_data {
 extern unsigned int ext4_count_free(char *bitmap, unsigned numchars);
 void ext4_inode_bitmap_csum_set(struct super_block *sb,
 				struct ext4_group_desc *gdp,
-				struct buffer_head *bh, int sz);
+				struct buffer_head *bh);
 int ext4_inode_bitmap_csum_verify(struct super_block *sb,
 				  struct ext4_group_desc *gdp,
-				  struct buffer_head *bh, int sz);
+				  struct buffer_head *bh);
 void ext4_block_bitmap_csum_set(struct super_block *sb,
 				struct ext4_group_desc *gdp,
 				struct buffer_head *bh);
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index f446588af368..39fcffe4c529 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -98,8 +98,7 @@ static int ext4_validate_inode_bitmap(struct super_block *sb,
 	if (buffer_verified(bh))
 		goto verified;
 	blk = ext4_inode_bitmap(sb, desc);
-	if (!ext4_inode_bitmap_csum_verify(sb, desc, bh,
-					   EXT4_INODES_PER_GROUP(sb) / 8) ||
+	if (!ext4_inode_bitmap_csum_verify(sb, desc, bh) ||
 	    ext4_simulate_fail(sb, EXT4_SIM_IBITMAP_CRC)) {
 		ext4_unlock_group(sb, block_group);
 		ext4_error(sb, "Corrupt inode bitmap - block_group = %u, "
@@ -327,8 +326,7 @@ void ext4_free_inode(handle_t *handle, struct inode *inode)
 		if (percpu_counter_initialized(&sbi->s_dirs_counter))
 			percpu_counter_dec(&sbi->s_dirs_counter);
 	}
-	ext4_inode_bitmap_csum_set(sb, gdp, bitmap_bh,
-				   EXT4_INODES_PER_GROUP(sb) / 8);
+	ext4_inode_bitmap_csum_set(sb, gdp, bitmap_bh);
 	ext4_group_desc_csum_set(sb, block_group, gdp);
 	ext4_unlock_group(sb, block_group);
 
@@ -853,8 +851,7 @@ int ext4_mark_inode_used(struct super_block *sb, int ino)
 
 	ext4_free_inodes_set(sb, gdp, ext4_free_inodes_count(sb, gdp) - 1);
 	if (ext4_has_group_desc_csum(sb)) {
-		ext4_inode_bitmap_csum_set(sb, gdp, inode_bitmap_bh,
-					   EXT4_INODES_PER_GROUP(sb) / 8);
+		ext4_inode_bitmap_csum_set(sb, gdp, inode_bitmap_bh);
 		ext4_group_desc_csum_set(sb, group, gdp);
 	}
 
@@ -1228,8 +1225,7 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
 		}
 	}
 	if (ext4_has_group_desc_csum(sb)) {
-		ext4_inode_bitmap_csum_set(sb, gdp, inode_bitmap_bh,
-					   EXT4_INODES_PER_GROUP(sb) / 8);
+		ext4_inode_bitmap_csum_set(sb, gdp, inode_bitmap_bh);
 		ext4_group_desc_csum_set(sb, group, gdp);
 	}
 	ext4_unlock_group(sb, group);
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
index 0ba9837d65ca..e04eb08b9060 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -1319,8 +1319,7 @@ static int ext4_set_bitmap_checksums(struct super_block *sb,
 	bh = ext4_get_bitmap(sb, group_data->inode_bitmap);
 	if (!bh)
 		return -EIO;
-	ext4_inode_bitmap_csum_set(sb, gdp, bh,
-				   EXT4_INODES_PER_GROUP(sb) / 8);
+	ext4_inode_bitmap_csum_set(sb, gdp, bh);
 	brelse(bh);
 
 	bh = ext4_get_bitmap(sb, group_data->block_bitmap);
-- 
2.30.0

[PATCH 6/7] ext4: remove unneeded NULL check of buffer_head in ext4_mark_inode_used

[Kemeng Shi]

If gdp from ext4_get_group_desc is not NULL, then returned group_desc_bh
won't be NULL either. Remove check of group_desc_bh and only check
returned gdp from ext4_get_group_desc like how other callers do.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 39fcffe4c529..0cb109037384 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -772,7 +772,7 @@ int ext4_mark_inode_used(struct super_block *sb, int ino)
 	}
 
 	gdp = ext4_get_group_desc(sb, group, &group_desc_bh);
-	if (!gdp || !group_desc_bh) {
+	if (!gdp) {
 		err = -EINVAL;
 		goto out;
 	}
-- 
2.30.0

[PATCH 7/7] ext4: check buffer_verified in advance to avoid unneeded ext4_get_group_info

[Kemeng Shi]

Check buffer_verified in advance to avoid unneeded ext4_get_group_info.
This could be a simple cleanup as complier may handle this.

Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
---
 fs/ext4/ialloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 0cb109037384..e700236303cd 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -87,10 +87,10 @@ static int ext4_validate_inode_bitmap(struct super_block *sb,
 	if (EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY)
 		return 0;
 
-	grp = ext4_get_group_info(sb, block_group);
-
 	if (buffer_verified(bh))
 		return 0;
+
+	grp = ext4_get_group_info(sb, block_group);
 	if (!grp || EXT4_MB_GRP_IBITMAP_CORRUPT(grp))
 		return -EFSCORRUPTED;
 
-- 
2.30.0

Re: [PATCH 2/7] ext4: avoid potential buffer_head leak in __ext4_new_inode

[Zhang Yi]

On 2024/8/13 20:07, Kemeng Shi wrote:
> If a group is marked EXT4_GROUP_INFO_IBITMAP_CORRUPT after it's inode
> bitmap buffer_head was successfully verified, then __ext4_new_inode
> will get a valid inode_bitmap_bh of a corrupted group from
> ext4_read_inode_bitmap in which case inode_bitmap_bh misses a release.
> Hnadle "IS_ERR(inode_bitmap_bh)" and group corruption separately like
> how ext4_free_inode does to avoid buffer_head leak.
> 
> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
> ---
>  fs/ext4/ialloc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index ad7f13976dc6..f24a238b6b09 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1054,9 +1054,13 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>  		brelse(inode_bitmap_bh);
>  		inode_bitmap_bh = ext4_read_inode_bitmap(sb, group);
>  		/* Skip groups with suspicious inode tables */
> -		if (((!(sbi->s_mount_state & EXT4_FC_REPLAY))
> -		     && EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) ||
> -		    IS_ERR(inode_bitmap_bh)) {
> +		if (IS_ERR(inode_bitmap_bh)) {
> +			inode_bitmap_bh = NULL;
> +			goto next_group;
> +		}
> +		if (!(sbi->s_mount_state & EXT4_FC_REPLAY) &&
> +		    EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
> +			brelse(inode_bitmap_bh);
>  			inode_bitmap_bh = NULL;

Wouldn't the inode_bitmap_bh be brelsed in the next loop or the end of this
function? why not just goto next_group?

Thanks,
Yi.

>  			goto next_group;
>  		}
> 

Re: [PATCH 2/7] ext4: avoid potential buffer_head leak in __ext4_new_inode

[Kemeng Shi]

on 8/14/2024 4:48 PM, Zhang Yi wrote:
> On 2024/8/13 20:07, Kemeng Shi wrote:
>> If a group is marked EXT4_GROUP_INFO_IBITMAP_CORRUPT after it's inode
>> bitmap buffer_head was successfully verified, then __ext4_new_inode
>> will get a valid inode_bitmap_bh of a corrupted group from
>> ext4_read_inode_bitmap in which case inode_bitmap_bh misses a release.
>> Hnadle "IS_ERR(inode_bitmap_bh)" and group corruption separately like
>> how ext4_free_inode does to avoid buffer_head leak.
>>
>> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com>
>> ---
>>  fs/ext4/ialloc.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
>> index ad7f13976dc6..f24a238b6b09 100644
>> --- a/fs/ext4/ialloc.c
>> +++ b/fs/ext4/ialloc.c
>> @@ -1054,9 +1054,13 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
>>  		brelse(inode_bitmap_bh);
>>  		inode_bitmap_bh = ext4_read_inode_bitmap(sb, group);
>>  		/* Skip groups with suspicious inode tables */
>> -		if (((!(sbi->s_mount_state & EXT4_FC_REPLAY))
>> -		     && EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) ||
>> -		    IS_ERR(inode_bitmap_bh)) {
>> +		if (IS_ERR(inode_bitmap_bh)) {
>> +			inode_bitmap_bh = NULL;
>> +			goto next_group;
>> +		}
>> +		if (!(sbi->s_mount_state & EXT4_FC_REPLAY) &&
>> +		    EXT4_MB_GRP_IBITMAP_CORRUPT(grp)) {
>> +			brelse(inode_bitmap_bh);
>>  			inode_bitmap_bh = NULL;
> 
> Wouldn't the inode_bitmap_bh be brelsed in the next loop or the end of this
> function? why not just goto next_group?
Sure, goto next_group directly will be better. Will do it in next version.

Thanks,
Kemeng
> 
> Thanks,
> Yi.
> 
>>  			goto next_group;
>>  		}
>>
> 

Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Markus Elfring]

> Release inode_bitmap_bh from ext4_read_inode_bitmap in
> ext4_mark_inode_used to avoid buffer_head leak.
> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
> is NULL.

1. I suggest to split such changes into separate update steps.
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81

2. How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

3. Would you like to append parentheses to any function names?

Regards,
Markus

Re: [PATCH 7/7] ext4: check buffer_verified in advance to avoid unneeded ext4_get_group_info

[Markus Elfring]

…
> This could be a simple cleanup as complier may handle this.

                                    compiler?


Would you like to append parentheses to the mentioned function name?

Regards,
Markus

Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Kemeng Shi]

on 8/15/2024 5:55 PM, Markus Elfring wrote:
>> Release inode_bitmap_bh from ext4_read_inode_bitmap in
>> ext4_mark_inode_used to avoid buffer_head leak.
>> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
>> is NULL.
> 
> 1. I suggest to split such changes into separate update steps.
>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81
It's acceptable to me, but I'm not sure if it worth separate patches
to others. I will do separate in next version if no person is against
this.
> 
> 2. How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> 
> 3. Would you like to append parentheses to any function names?
Thanks for remind me of these. I will improve the series in next
version.

Thanks,
Kemeng
> 
> Regards,
> Markus
> 

Re: [PATCH 7/7] ext4: check buffer_verified in advance to avoid unneeded ext4_get_group_info

[Kemeng Shi]

on 8/15/2024 7:13 PM, Markus Elfring wrote:
> …
>> This could be a simple cleanup as complier may handle this.
> 
>                                     compiler?
Right.
> 
> 
> Would you like to append parentheses to the mentioned function name?
Sure

Will improve in next version.

Thanks,
Kemeng
> 
> Regards,
> Markus
> 

Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Darrick J. Wong]

On Thu, Aug 15, 2024 at 09:17:10PM +0800, Kemeng Shi wrote:
> 
> 
> on 8/15/2024 5:55 PM, Markus Elfring wrote:
> >> Release inode_bitmap_bh from ext4_read_inode_bitmap in
> >> ext4_mark_inode_used to avoid buffer_head leak.
> >> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
> >> is NULL.
> > 
> > 1. I suggest to split such changes into separate update steps.
> >    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81
> It's acceptable to me, but I'm not sure if it worth separate patches
> to others. I will do separate in next version if no person is against
> this.

No, that suggestion is stupid.  There's no reason to generate even more
patches for a three line fix, it's very obvious that you're fixing a
missing resource release and rearranging the first error out
accordingly.

--D

> > 2. How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
> > 
> > 3. Would you like to append parentheses to any function names?
> Thanks for remind me of these. I will improve the series in next
> version.
> 
> Thanks,
> Kemeng
> > 
> > Regards,
> > Markus
> > 
> 
> 

Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Markus Elfring]

>>>> Release inode_bitmap_bh from ext4_read_inode_bitmap in
>>>> ext4_mark_inode_used to avoid buffer_head leak.
>>>> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
>>>> is NULL.
>>>
>>> 1. I suggest to split such changes into separate update steps.
>>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81
>> It's acceptable to me, but I'm not sure if it worth separate patches
>> to others. I will do separate in next version if no person is against
>> this.
>
> No, that suggestion is stupid.

Please reconsider such a view a bit more.



>                                 There's no reason to generate even more
> patches for a three line fix, it's very obvious that you're fixing a
> missing resource release and rearranging the first error out
> accordingly.

You would probably like to distinguish the severity for two changes,
wouldn't you?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n168

Under which circumstances can you accept the separation of development concerns better?

Regards,
Markus

Re: [PATCH 1/7] ext4: avoid buffer_head leak in ext4_mark_inode_used

[Theodore Ts'o]

On Fri, Aug 16, 2024 at 08:56:45AM +0200, Markus Elfring wrote:
> >>>> Release inode_bitmap_bh from ext4_read_inode_bitmap in
> >>>> ext4_mark_inode_used to avoid buffer_head leak.
> >>>> By the way, remove unneeded goto for invalid ino when inode_bitmap_bh
> >>>> is NULL.
> >>>
> >>> 1. I suggest to split such changes into separate update steps.
> >>>    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n81
> >> It's acceptable to me, but I'm not sure if it worth separate patches
> >> to others. I will do separate in next version if no person is against
> >> this.
> >
> > No, that suggestion is stupid.
> 
> Please reconsider such a view a bit more.

Darrick is absolutely correct; that suggestion is.... ill-considered.

> >                                 There's no reason to generate even more
> > patches for a three line fix, it's very obvious that you're fixing a
> > missing resource release and rearranging the first error out
> > accordingly.
> 
> You would probably like to distinguish the severity for two changes,
> wouldn't you?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc3#n168
> 
> Under which circumstances can you accept the separation of development concerns better?

We will sometimes do minor cleanups in the course of fixing a bug.  In
this particular case, the cleanup is so minor, that if someone
suggested it as a stand-alone cleanup patch, I'd reject it as adding
noise, and not being worth the extra commit.

Blindly following rules is a bad idea; that's because software
programming is an engineering discpline, which means we are often
trading off multiple goals, each of which are good in and of
themselves.  For example, extra patch noise, such as fixing
whitespace, or changing a goto errout to a return, makes zero
difference to the generated code, only a very tiny margial improvement
in the readability in the code base; and also increases the chance
that some future bug fix won't backport cleanly to older LTS kernels.

I expect ext4 developers to use their good judgement, and not just
blindly follow rules, even good rules which may make sense 80% or even
95% of the time in the submitting-patches.rst file.

Markus, perhaps you could good "blindly following rules" and read some
of the eassays found from that web search, if you need more
explorations of that topic.

Best regards, 

						- Ted