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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
