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

> 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

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
> 

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
> > 
> 
> 

>>>> 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

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