Re: [v3 for-next 2/3] block: allow error injection rules to delay bios

[Mohamed Khalfella <mkhalfella@purestorage.com>]

On Tue 2026-09-29 00:16:33 +0200, Md Haris Iqbal wrote:
> Error injection can only fail a bio today. Add a delay_us option so that
> a rule can hold a bio back first, to model a slow device.
> 
> If a matching rule has delay_us set, the bio is held for that long. It is
> then failed if the rule also has a status, or resubmitted otherwise.
> 
> The resubmission is done through submit_bio_noacct_nocheck(), which is
> where the injection hook sits. To avoid the same delay rule being applied
> to it again, mark the bio with the BIO_ERROR_INJECTED flag before queueing
> the work in blk_error_inject_delay(). This also prevents bios that are
> split later and resubmitted from being evaluated for delay (or error
> injection) again. __bio_clone() does not propagate the flag, so a clone a
> stacking driver aims at a lower device is still evaluated.
> 
> The bio is submitted from a workqueue rather than from the timer, because
> submitting a bio can sleep. Bios with REQ_NOWAIT are never delayed, and
> values above 600 seconds are rejected.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Md Haris Iqbal <haris.iqbal@linux.dev>
> ---
>  block/error-injection.c   | 160 ++++++++++++++++++++++++++++++++++----
>  block/error-injection.h   |   1 +
>  include/linux/blk_types.h |   1 +
>  3 files changed, 147 insertions(+), 15 deletions(-)
> 
> diff --git a/block/error-injection.c b/block/error-injection.c
> index db543fe27630..a35fe41a43e4 100644
> --- a/block/error-injection.c
> +++ b/block/error-injection.c
> @@ -6,9 +6,17 @@
>  #include <linux/blkdev.h>
>  #include <linux/parser.h>
>  #include <linux/seq_file.h>
> +#include <linux/workqueue.h>
>  #include "blk.h"
>  #include "error-injection.h"
>  
> +/*
> + * Cap the delay so that a typo can't wedge a device for good.  This is still
> + * well beyond the default hung task timeout, which is one of the things a
> + * delay is useful for triggering.
> + */
> +#define BLK_ERROR_INJECT_MAX_DELAY_US	(600 * USEC_PER_SEC)

DEFAULT_HUNG_TASK_TIMEOUT is 120 seconds (set in lib/Kconfig.debug)

> +
>  struct blk_error_inject {
>  	struct list_head		entry;
>  	sector_t			start;
> @@ -18,14 +26,104 @@ struct blk_error_inject {
>  
>  	/* only inject every 1 / chance times */
>  	unsigned int			chance;
> +
> +	/* hold the bio for this long before submitting or failing it */
> +	unsigned int			delay_us;
> +};
> +
> +/*
> + * A bio held by a delay rule.  It does not point back at the rule, so rules
> + * can be removed while delayed bios are outstanding.  The submitter holds the
> + * bdev open until the bio completes, so the disk and queue stay allocated.
> + * A delayed bio holds no queue usage counter reference, so it does not hold
> + * up a queue freeze either.
> + */
> +struct blk_error_inject_delay {
> +	struct delayed_work		dwork;
> +	struct bio			*bio;
> +	blk_status_t			status;
>  };
>  
> +static struct workqueue_struct *blk_error_inject_wq;
> +
>  DEFINE_STATIC_KEY_FALSE(blk_error_injection_enabled);
>  
> +static void blk_error_inject_delay_work(struct work_struct *work)
> +{
> +	struct blk_error_inject_delay *d = container_of(to_delayed_work(work),
> +			struct blk_error_inject_delay, dwork);
> +	struct bio *bio = d->bio;
> +	struct gendisk *disk = bio->bi_bdev->bd_disk;
> +	blk_status_t status = d->status;
> +
> +	kfree(d);
> +
> +	if (status != BLK_STS_OK) {
> +		pr_info_ratelimited("%pg: injecting %s error for %s at sector %llu:%u\n",
> +				disk->part0, blk_status_to_str(status),
> +				blk_op_str(bio_op(bio)), bio->bi_iter.bi_sector,
> +				bio_sectors(bio));
> +		bio->bi_status = status;
> +		bio_endio(bio);
> +	} else {
> +		/*
> +		 * Resubmit.  The BIO_ERROR_INJECTED flag ensures that a bio
> +		 * that was delayed once skips error injection entirely from
> +		 * here on, including any other rule that covers it.
> +		 */
> +		submit_bio_noacct_nocheck(bio, false);
> +	}
> +}
> +
> +/*
> + * Hand the bio to a workqueue that submits or fails it once the delay has
> + * expired.  Both blk_mq_submit_bio() and ->submit_bio can sleep, so this can't
> + * be completed from the timer itself.
> + *
> + * Returns false if the bio can't be delayed, in which case the caller handles
> + * it immediately instead.
> + */
> +static bool blk_error_inject_delay(struct gendisk *disk, struct bio *bio,
> +		blk_status_t status, unsigned int delay_us)
> +{
> +	struct blk_error_inject_delay *d;
> +
> +	/* never block a bio that asked not to be blocked */
> +	if (bio->bi_opf & REQ_NOWAIT)
> +		return false;
> +
> +	d = kmalloc_obj(*d, GFP_NOIO);
> +	if (!d)
> +		return false;
> +
> +	pr_info_ratelimited("%pg: delaying %s at sector %llu:%u by %uus\n",
> +			disk->part0, blk_op_str(bio_op(bio)),
> +			bio->bi_iter.bi_sector, bio_sectors(bio), delay_us);
> +
> +	d->bio = bio;
> +	d->status = status;
> +	INIT_DELAYED_WORK(&d->dwork, blk_error_inject_delay_work);
> +
> +	/*
> +	 * Mark the bio before queueing the work, which can complete it as soon
> +	 * as it is queued.  Splitting happens below the injection hook, but
> +	 * bio_submit_split_bioset() resubmits the remainder through the hook
> +	 * again, and as bio_split() only advances the original bio that
> +	 * remainder still matches the same rule.  Without this a bio would be
> +	 * delayed once per split.
> +	 */
> +	bio_set_flag(bio, BIO_ERROR_INJECTED);
> +	queue_delayed_work(blk_error_inject_wq, &d->dwork,
> +			usecs_to_jiffies(delay_us));

How about comparing the delay against sysctl_hung_task_timeout_secs in
order not to trigger hung task detector?

> +	return true;
> +}
> +
>  bool __blk_error_inject(struct bio *bio)
>  {
>  	struct gendisk *disk = bio->bi_bdev->bd_disk;
>  	struct blk_error_inject *inj;
> +	blk_status_t status = BLK_STS_OK;
> +	unsigned int delay_us = 0;
>  
>  	rcu_read_lock();
>  	list_for_each_entry_rcu(inj, &disk->error_injection_list, entry) {
> @@ -45,29 +143,38 @@ bool __blk_error_inject(struct bio *bio)
>  		if (inj->chance > 1 && (get_random_u32() % inj->chance) != 0)
>  			continue;
>  
> -		pr_info_ratelimited("%pg: injecting %s error for %s at sector %llu:%u\n",
> -				disk->part0, blk_status_to_str(inj->status),
> -				blk_op_str(inj->op), bio->bi_iter.bi_sector,
> -				bio_sectors(bio));
> -		bio->bi_status = inj->status;
> -		rcu_read_unlock();
> -		bio_endio(bio);
> -		return true;
> +		status = inj->status;
> +		delay_us = inj->delay_us;
> +		break;
>  	}
>  	rcu_read_unlock();
> -	return false;
> +
> +	if (delay_us && blk_error_inject_delay(disk, bio, status, delay_us))
> +		return true;
> +	if (status == BLK_STS_OK)
> +		return false;

status == BLK_STS_OK is permitted only if delay_us != 0. It means we are
here because blk_error_inject_delay() failed to inject a delay?

Also, in case delay injection fails, do we want to continue with error
injection? Or inject nothing and return false instead?

> +
> +	pr_info_ratelimited("%pg: injecting %s error for %s at sector %llu:%u\n",
> +			disk->part0, blk_status_to_str(status),
> +			blk_op_str(bio_op(bio)), bio->bi_iter.bi_sector,
> +			bio_sectors(bio));
> +	bio->bi_status = status;
> +	bio_endio(bio);
> +	return true;
>  }
>  
>  static int error_inject_add(struct gendisk *disk, enum req_op op,
>  		sector_t start, u64 nr_sectors, blk_status_t status,
> -		unsigned int chance)
> +		unsigned int chance, unsigned int delay_us)
>  {
>  	struct blk_error_inject *inj;
>  	int error = -EINVAL;
>  
>  	if (op == REQ_OP_LAST)
>  		return -EINVAL;
> -	if (status == BLK_STS_OK)
> +	if (status == BLK_STS_OK && !delay_us)
> +		return -EINVAL;
> +	if (delay_us > BLK_ERROR_INJECT_MAX_DELAY_US)
>  		return -EINVAL;

This could also compare against sysctl_hung_task_timeout_secs and maybe
drop BLK_ERROR_INJECT_MAX_DELAY_US?