Date:   Thu, 21 Nov 2019 16:09:33 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     "Theodore Ts'o" <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 2/2] ext4: simulate various I/O and checksum errors when
 reading metadata

On Thu, Nov 21, 2019 at 01:30:36PM -0500, Theodore Ts'o wrote:
> This allows us to test various error handling code paths
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> ---
>  fs/ext4/balloc.c |  4 +++-
>  fs/ext4/ext4.h   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  fs/ext4/ialloc.c |  4 +++-
>  fs/ext4/inode.c  |  6 +++++-
>  fs/ext4/namei.c  | 11 ++++++++---
>  fs/ext4/sysfs.c  | 23 +++++++++++++++++++++++
>  6 files changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 102c38527a10..5f993a411251 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -371,7 +371,8 @@ static int ext4_validate_block_bitmap(struct super_block *sb,
>  	if (buffer_verified(bh))
>  		goto verified;
>  	if (unlikely(!ext4_block_bitmap_csum_verify(sb, block_group,
> -			desc, bh))) {
> +						    desc, bh) ||
> +		     ext4_simulate_fail(sb, EXT4_SIM_BBITMAP_CRC))) {
>  		ext4_unlock_group(sb, block_group);
>  		ext4_error(sb, "bg %u: bad block bitmap checksum", block_group);
>  		ext4_mark_group_bitmap_corrupted(sb, block_group,
> @@ -505,6 +506,7 @@ int ext4_wait_block_bitmap(struct super_block *sb, ext4_group_t block_group,
>  	if (!desc)
>  		return -EFSCORRUPTED;
>  	wait_on_buffer(bh);
> +	ext4_simulate_fail_bh(sb, bh, EXT4_SIM_BBITMAP_EIO);
>  	if (!buffer_uptodate(bh)) {
>  		ext4_set_errno(sb, EIO);
>  		ext4_error(sb, "Cannot read block bitmap - "
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 1c9ac0fc8715..e6798db4634c 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1557,6 +1557,9 @@ struct ext4_sb_info {
>  	/* Barrier between changing inodes' journal flags and writepages ops. */
>  	struct percpu_rw_semaphore s_journal_flag_rwsem;
>  	struct dax_device *s_daxdev;
> +#ifdef CONFIG_EXT4_DEBUG
> +	unsigned long s_simulate_fail;
> +#endif
>  };
>  
>  static inline struct ext4_sb_info *EXT4_SB(struct super_block *sb)
> @@ -1575,6 +1578,45 @@ static inline int ext4_valid_inum(struct super_block *sb, unsigned long ino)
>  		 ino <= le32_to_cpu(EXT4_SB(sb)->s_es->s_inodes_count));
>  }
>  
> +static inline int ext4_simulate_fail(struct super_block *sb,
> +				     unsigned long flag)

Nit: bool?

> +{
> +#ifdef CONFIG_EXT4_DEBUG
> +	unsigned long old, new;
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +
> +	do {
> +		old = READ_ONCE(sbi->s_simulate_fail);
> +		if (likely((old & flag) == 0))
> +			return 0;
> +		new = old & ~flag;
> +	} while (unlikely(cmpxchg(&sbi->s_simulate_fail, old, new) != old));

If I'm reading this correctly, this means that userspace sets a
s_simulate_fail bit via sysfs knob, and the next time the filesystem
calls ext4_simulate_fail with the same bit set in @flag we'll return
true to say "simulate the failure" and clear the bit in s_simulate_fail?

IOWs, the simulated failures have to be re-armed every time?

Seems reasonable, but consider the possibility that in the future it
might be useful if you could set up periodic failures (e.g. directory
lookups fail 10% of the time) so that you can see how something like
fsstress reacts to less-predictable failures?

Of course that also increases the amount of fugly sysfs boilerplate so
that each knob can have its own sysfs file... that alone is half of a
reason not to do that. :(

--D

> +	return 1;
> +#else
> +	return 0;
> +#endif
> +}
> +
> +static inline void ext4_simulate_fail_bh(struct super_block *sb,
> +					 struct buffer_head *bh,
> +					 unsigned long flag)
> +{
> +	if (!IS_ERR(bh) && ext4_simulate_fail(sb, flag))
> +		clear_buffer_uptodate(bh);
> +}
> +
> +/*
> + * Simulate_fail flags
> + */
> +#define EXT4_SIM_BBITMAP_EIO	0x00000001
> +#define EXT4_SIM_BBITMAP_CRC	0x00000002
> +#define EXT4_SIM_IBITMAP_EIO	0x00000004
> +#define EXT4_SIM_IBITMAP_CRC	0x00000008
> +#define EXT4_SIM_INODE_EIO	0x00000010
> +#define EXT4_SIM_INODE_CRC	0x00000020
> +#define EXT4_SIM_DIRBLOCK_EIO	0x00000040
> +#define EXT4_SIM_DIRBLOCK_CRC	0x00000080
> +
>  /*
>   * Error number codes for s_{first,last}_error_errno
>   *
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 31a5fd6f5e6a..f7033594f4a0 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -94,7 +94,8 @@ static int ext4_validate_inode_bitmap(struct super_block *sb,
>  		goto verified;
>  	blk = ext4_inode_bitmap(sb, desc);
>  	if (!ext4_inode_bitmap_csum_verify(sb, block_group, desc, bh,
> -					   EXT4_INODES_PER_GROUP(sb) / 8)) {
> +					   EXT4_INODES_PER_GROUP(sb) / 8) ||
> +	    ext4_simulate_fail(sb, EXT4_SIM_IBITMAP_CRC)) {
>  		ext4_unlock_group(sb, block_group);
>  		ext4_error(sb, "Corrupt inode bitmap - block_group = %u, "
>  			   "inode_bitmap = %llu", block_group, blk);
> @@ -192,6 +193,7 @@ ext4_read_inode_bitmap(struct super_block *sb, ext4_group_t block_group)
>  	get_bh(bh);
>  	submit_bh(REQ_OP_READ, REQ_META | REQ_PRIO, bh);
>  	wait_on_buffer(bh);
> +	ext4_simulate_fail_bh(sb, bh, EXT4_SIM_IBITMAP_EIO);
>  	if (!buffer_uptodate(bh)) {
>  		put_bh(bh);
>  		ext4_set_errno(sb, EIO);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b67ffa24ae0a..564f88040de9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4243,6 +4243,8 @@ static int __ext4_get_inode_loc(struct inode *inode,
>  	bh = sb_getblk(sb, block);
>  	if (unlikely(!bh))
>  		return -ENOMEM;
> +	if (ext4_simulate_fail(sb, EXT4_SIM_INODE_EIO))
> +		goto simulate_eio;
>  	if (!buffer_uptodate(bh)) {
>  		lock_buffer(bh);
>  
> @@ -4341,6 +4343,7 @@ static int __ext4_get_inode_loc(struct inode *inode,
>  		blk_finish_plug(&plug);
>  		wait_on_buffer(bh);
>  		if (!buffer_uptodate(bh)) {
> +		simulate_eio:
>  			ext4_set_errno(inode->i_sb, EIO);
>  			EXT4_ERROR_INODE_BLOCK(inode, block,
>  					       "unable to read itable block");
> @@ -4555,7 +4558,8 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  					      sizeof(gen));
>  	}
>  
> -	if (!ext4_inode_csum_verify(inode, raw_inode, ei)) {
> +	if (!ext4_inode_csum_verify(inode, raw_inode, ei) ||
> +	    ext4_simulate_fail(sb, EXT4_SIM_INODE_CRC)) {
>  		ext4_set_errno(inode->i_sb, EFSBADCRC);
>  		ext4_error_inode(inode, function, line, 0,
>  				 "iget: checksum invalid");
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index cee7c28e070d..be14e33e5103 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -109,7 +109,10 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
>  	struct ext4_dir_entry *dirent;
>  	int is_dx_block = 0;
>  
> -	bh = ext4_bread(NULL, inode, block, 0);
> +	if (ext4_simulate_fail(inode->i_sb, EXT4_SIM_DIRBLOCK_EIO))
> +		bh = ERR_PTR(-EIO);
> +	else
> +		bh = ext4_bread(NULL, inode, block, 0);
>  	if (IS_ERR(bh)) {
>  		__ext4_warning(inode->i_sb, func, line,
>  			       "inode #%lu: lblock %lu: comm %s: "
> @@ -153,7 +156,8 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
>  	 * caller is sure it should be an index block.
>  	 */
>  	if (is_dx_block && type == INDEX) {
> -		if (ext4_dx_csum_verify(inode, dirent))
> +		if (ext4_dx_csum_verify(inode, dirent) &&
> +		    !ext4_simulate_fail(inode->i_sb, EXT4_SIM_DIRBLOCK_CRC))
>  			set_buffer_verified(bh);
>  		else {
>  			ext4_set_errno(inode->i_sb, EFSBADCRC);
> @@ -164,7 +168,8 @@ static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
>  		}
>  	}
>  	if (!is_dx_block) {
> -		if (ext4_dirblock_csum_verify(inode, bh))
> +		if (ext4_dirblock_csum_verify(inode, bh) &&
> +		    !ext4_simulate_fail(inode->i_sb, EXT4_SIM_DIRBLOCK_CRC))
>  			set_buffer_verified(bh);
>  		else {
>  			ext4_set_errno(inode->i_sb, EFSBADCRC);
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index eb1efad0e20a..a990d28d191b 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -29,6 +29,7 @@ typedef enum {
>  	attr_last_error_time,
>  	attr_feature,
>  	attr_pointer_ui,
> +	attr_pointer_ul,
>  	attr_pointer_atomic,
>  	attr_journal_task,
>  } attr_id_t;
> @@ -160,6 +161,9 @@ static struct ext4_attr ext4_attr_##_name = {			\
>  #define EXT4_RW_ATTR_SBI_UI(_name,_elname)	\
>  	EXT4_ATTR_OFFSET(_name, 0644, pointer_ui, ext4_sb_info, _elname)
>  
> +#define EXT4_RW_ATTR_SBI_UL(_name,_elname)	\
> +	EXT4_ATTR_OFFSET(_name, 0644, pointer_ul, ext4_sb_info, _elname)
> +
>  #define EXT4_ATTR_PTR(_name,_mode,_id,_ptr) \
>  static struct ext4_attr ext4_attr_##_name = {			\
>  	.attr = {.name = __stringify(_name), .mode = _mode },	\
> @@ -194,6 +198,9 @@ EXT4_RW_ATTR_SBI_UI(warning_ratelimit_interval_ms, s_warning_ratelimit_state.int
>  EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst);
>  EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval);
>  EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst);
> +#ifdef CONFIG_EXT4_DEBUG
> +EXT4_RW_ATTR_SBI_UL(simulate_fail, s_simulate_fail);
> +#endif
>  EXT4_RO_ATTR_ES_UI(errors_count, s_error_count);
>  EXT4_ATTR(first_error_time, 0444, first_error_time);
>  EXT4_ATTR(last_error_time, 0444, last_error_time);
> @@ -228,6 +235,9 @@ static struct attribute *ext4_attrs[] = {
>  	ATTR_LIST(first_error_time),
>  	ATTR_LIST(last_error_time),
>  	ATTR_LIST(journal_task),
> +#ifdef CONFIG_EXT4_DEBUG
> +	ATTR_LIST(simulate_fail),
> +#endif
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(ext4);
> @@ -318,6 +328,11 @@ static ssize_t ext4_attr_show(struct kobject *kobj,
>  		else
>  			return snprintf(buf, PAGE_SIZE, "%u\n",
>  					*((unsigned int *) ptr));
> +	case attr_pointer_ul:
> +		if (!ptr)
> +			return 0;
> +		return snprintf(buf, PAGE_SIZE, "%lu\n",
> +				*((unsigned long *) ptr));
>  	case attr_pointer_atomic:
>  		if (!ptr)
>  			return 0;
> @@ -361,6 +376,14 @@ static ssize_t ext4_attr_store(struct kobject *kobj,
>  		else
>  			*((unsigned int *) ptr) = t;
>  		return len;
> +	case attr_pointer_ul:
> +		if (!ptr)
> +			return 0;
> +		ret = kstrtoul(skip_spaces(buf), 0, &t);
> +		if (ret)
> +			return ret;
> +		*((unsigned long *) ptr) = t;
> +		return len;
>  	case attr_inode_readahead:
>  		return inode_readahead_blks_store(sbi, buf, len);
>  	case attr_trigger_test_error:
> -- 
> 2.23.0
> 

Powered by blists - more mailing lists