On Thu, Nov 07, 2024 at 04:53:42PM +0100, Jan Kara wrote:
> On Thu 24-10-24 10:19:09, leo.lilong@xxxxxxxxxxxxxxx wrote:
> > From: Long Li <leo.lilong@xxxxxxxxxx>
> >
> > When I enabled ext4 debug for fault injection testing, I encountered the
> > following warning:
> >
> > EXT4-fs error (device sda): ext4_read_inode_bitmap:201: comm fsstress:
> > Cannot read inode bitmap - block_group = 8, inode_bitmap = 1051
> > WARNING: CPU: 0 PID: 511 at fs/buffer.c:1181 mark_buffer_dirty+0x1b3/0x1d0
> >
> > The root cause of the issue lies in the improper implementation of ext4's
> > buffer_head read fault injection. The actual completion of buffer_head
> > read and the buffer_head fault injection are not atomic, which can lead
> > to the uptodate flag being cleared on normally used buffer_heads in race
> > conditions.
> >
> > [CPU0] [CPU1] [CPU2]
> > ext4_read_inode_bitmap
> > ext4_read_bh()
> > <bh read complete>
> > ext4_read_inode_bitmap
> > if (buffer_uptodate(bh))
> > return bh
> > jbd2_journal_commit_transaction
> > __jbd2_journal_refile_buffer
> > __jbd2_journal_unfile_buffer
> > __jbd2_journal_temp_unlink_buffer
> > ext4_simulate_fail_bh()
> > clear_buffer_uptodate
> > mark_buffer_dirty
> > <report warning>
> > WARN_ON_ONCE(!buffer_uptodate(bh))
> >
> > The best approach would be to perform fault injection in the IO completion
> > callback function, rather than after IO completion. However, the IO
> > completion callback function cannot get the fault injection code in sb.
> >
> > Fix it by passing the result of fault injection into the bh read function,
> > we simulate faults within the bh read function itself. This requires adding
> > an extra parameter to the bh read functions that need fault injection.
> >
> > Fixes: 46f870d690fe ("ext4: simulate various I/O and checksum errors when reading metadata")
> > Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
>
> Thanks for the fix! One suggestion below:
>
> > @@ -3100,9 +3092,9 @@ extern struct buffer_head *ext4_sb_bread(struct super_block *sb,
> > extern struct buffer_head *ext4_sb_bread_unmovable(struct super_block *sb,
> > sector_t block);
> > extern void ext4_read_bh_nowait(struct buffer_head *bh, blk_opf_t op_flags,
> > - bh_end_io_t *end_io);
> > + bh_end_io_t *end_io, bool simu_fail);
> > extern int ext4_read_bh(struct buffer_head *bh, blk_opf_t op_flags,
> > - bh_end_io_t *end_io);
> > + bh_end_io_t *end_io, bool simu_fail);
>
> Instead of adding a bool argument whether we should simulate a failure, I'd
> pass the 'code' into ext4_read_bh_nowait() and handle the check in there.
> That reduces the boilerplate code a bit and looks somewhat cleaner.
>
> Honza
Hi Honza,
Thanks for your reply, your solution does appear more cleaner, but it seems
we cannot directly get sbi->s_simulate_fail in ext4_read_bh_nowait(), nor
can we get it in the IO completion callback function.
Thanks,
Long Li
