Messages in this thread Patch in this message | | | Date | Wed, 5 Jan 2022 18:24:36 -0500 | From | "Theodore Ts'o" <> | Subject | Re: [PATCH v2] ext4: Remove unnecessary assignments |
| |
On Thu, Nov 04, 2021 at 06:36:37AM +0000, cgel.zte@gmail.com wrote: > From: luo penghao <luo.penghao@zte.com.cn> > > The assignment at the end of the function is not necessary > > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c > index 089c958..f1258a7 100644 > --- a/fs/ext4/mballoc.c > +++ b/fs/ext4/mballoc.c > @@ -3886,7 +3886,7 @@ void ext4_mb_mark_bb(struct super_block *sb, ext4_fsblk_t block, > if (err) > goto out_err; > sync_dirty_buffer(bitmap_bh); > - err = ext4_handle_dirty_metadata(NULL, NULL, gdp_bh); > + ext4_handle_dirty_metadata(NULL, NULL, gdp_bh); > sync_dirty_buffer(gdp_bh); > > out_err:
There's actually a bigger issue here than the Clang analyzer complaining about the unnecessasary assignment --- and that is we *should* be propagating the error up to ext4_mb_mark_bb's callers, and those callers should be logging errors and potentially aborting the fast_commit replay.
There might be some errors that can be ignored, if an idempotent operation doesn't need to be redone. However, in cases like ext4_handle_dirty_metadata(), or ext4_read_block_bitmap(), any failures are probably due to something fatal happening --- either an ENOMEM, or an I/O error, etc., and simply silently aborting the current function without logging any kind of problem is going to make it much harder to root cause a potential fast commit replay failure.
So what we should probably do is make ext4_mb_mark_bb return an error, and then we'll need to look at all of the callers of ext4_mb_mark_bb, and fix them up as necessary.
- Ted
| |