| Message ID | 20240813120712.2592310-2-shikemeng@huaweicloud.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes and cleanups to ext4 ialloc | expand |
> 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
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; }
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(-)