block: delay support for error injection

Error injection can only fail a bio today. This adds a delay_us option so that a rule can hold a bio back first, to model a slow device.

The delay happens above the driver, so it is invisible to the I/O statistics and never reaches the blk-mq timeout handler or SCSI error handling. What it does exercise is the code waiting above the block
layer: io_uring cancellation, hung task detection, and filesystem or userspace timeouts.

Two things are worth a look. A delayed bio is marked with a new BIO_ERROR_INJECTED bio flag, so the rules are not applied to it again and it can never pick up a status from another rule. Resubmitting it below the injection hook is not enough on its own, because a bio is split below the hook and the remainder is resubmitted above it. And holding a bio back reorders it against bios submitted later, which breaks sequential write ordering on zoned devices. Both are documented in patch 3.

Patch 1 is a prep cleanup. It moves the rejection of an unknown status tag into the parser, because patch 2 makes a rule without a status valid.

v1: https://lore.kernel.org/linux-block/[email protected]/

Changes since v1:

- Add the BIO_ERROR_INJECTED bio flag. v1 only resubmitted a delayed bio below the injection hook, which left the remainder of a split going through the hook and matching the same rule again, so a bio was held once per split instead of once.

- Patch 1: tag_to_blk_status() returns an error and passes the status back through a pointer, instead of returning BLK_STS_OK for both the "OK" tag and an unknown one. A delay-only rule has no status, so the two have to be told apart.

- Documentation: a delayed bio is held once rather than once per split.

Tested in a VM.

Md Haris Iqbal (3):
block: reject unknown status tags in error injection rules
block: allow error injection rules to delay bios
Documentation: block: document error injection delays

 Documentation/block/error-injection.rst |  58 ++++++++-
 block/blk-core.c                        |  27 ++--
 block/blk.h                             |   3 +-
 block/error-injection.c                 | 165 +++++++++++++++++++++---
 block/error-injection.h                 |   1 +
 include/linux/blk_types.h               |   1 +
 6 files changed, 222 insertions(+), 33 deletions(-)

2.53.0

tag_to_blk_status() returns BLK_STS_OK both for the "OK" tag and for a tag it does not recognise, so a caller cannot tell the two apart. match_status() leaves *status at BLK_STS_OK for an unknown tag and relies on error_inject_add() rejecting BLK_STS_OK.

That holds only while a rule without a status is meaningless. The delay option added next makes such a rule valid, and "OK" is what blk_error_injection_show() prints for one, so the two cases have to be told apart. Return the status through a pointer and report an unknown tag as -EINVAL. There is no spare blk_status_t to encode "not found" in:
every value in the table has a tag that can be typed, and anything outside the table trips the WARN_ON_ONCE() in blk_status_to_str(), blk_status_to_tag() and blk_status_to_errno().

For a single status= this does not change behaviour: an unknown tag still fails the write with -EINVAL. A repeated status= where an invalid tag comes first is now rejected instead of being overridden by the later one.

---
 block/blk-core.c        | 14 ++++++--------
 block/blk.h             |  2 +-
 block/error-injection.c |  7 ++++---
 3 files changed, 11 insertions(+), 12 deletions(-)

index 196bccf27f58..29c86addb8f2 100644
@@ -225,21 +225,19 @@ const char *blk_status_to_tag(blk_status_t status)
 	return blk_errors[idx].tag;
 }
 
-blk_status_t tag_to_blk_status(const char *tag)
+int tag_to_blk_status(const char *tag, blk_status_t *status)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(blk_errors); i++) {
 		if (blk_errors[i].tag &&
-		    !strcmp(blk_errors[i].tag, tag))
-			return (__force blk_status_t)i;
+		    !strcmp(blk_errors[i].tag, tag)) {
+			*status = (__force blk_status_t)i;
+			return 0;
+		}
 	}
 
-	/*
-	 * Return BLK_STS_OK for mismatches as this function is intended to
-	 * parse error status values.
-	 */
-	return BLK_STS_OK;
+	return -EINVAL;
 }
 
 /**

index 50abfd932886..8a8ab961528d 100644
@@ -52,7 +52,7 @@ void blk_free_flush_queue(struct blk_flush_queue *q);
 
 const char *blk_status_to_str(blk_status_t status);
 const char *blk_status_to_tag(blk_status_t status);
-blk_status_t tag_to_blk_status(const char *tag);
+int tag_to_blk_status(const char *tag, blk_status_t *status);
 enum req_op str_to_blk_op(const char *op);
 
 bool __blk_mq_unfreeze_queue(struct request_queue *q, bool force_atomic);

index e14bc4b723ef..41ee8f788bb5 100644
@@ -171,15 +171,16 @@ static int match_op(substring_t *args, enum req_op *op)
 static int match_status(substring_t *args, blk_status_t *status)
 {
 	const char *tag;
+	int ret;
 
 	tag = match_strdup(args);
 	if (!tag)
 		return -ENOMEM;
-	*status = tag_to_blk_status(tag);
-	if (!*status)
+	ret = tag_to_blk_status(tag, status);
+	if (ret)
 		pr_warn("invalid status '%s'\n", tag);
 	kfree(tag);
-	return 0;
+	return ret;
 }
 
 static ssize_t blk_error_injection_parse_options(struct gendisk *disk,

2.53.0

There is no *status yet.

This reads a lot like AI slop. Can you please self-write a short and descriptive commit message?

Ah yes. I'll correct it.

My bad. I'll correct it.

True. I'll change it.

I'll wait for your comments for the other patches before sending a v3.

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 below the injection hook so that the rules are not applied to it again.

Submitting below the hook is not enough on its own. A bio is split below the hook, and bio_submit_split_bioset() resubmits the remainder through submit_bio_noacct_nocheck(), which is above it. bio_split() advances the original bio and returns a clone of the front piece, so the remainder is the same bio on a range the rule still covers and would be delayed once per split. Mark a delayed bio with BIO_ERROR_INJECTED instead and skip the hook for a bio that has it. __bio_clone() does not propagate the flag, so the front pieces and the clones a stacking driver aims at a lower device are 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.

---
 block/blk-core.c          |  13 +++-
 block/blk.h               |   1 +
 block/error-injection.c   | 158 ++++++++++++++++++++++++++++++++++----
 block/error-injection.h   |   1 +
 include/linux/blk_types.h |   1 +
 5 files changed, 155 insertions(+), 19 deletions(-)

index 29c86addb8f2..6ba21fd37b6c 100644
@@ -757,11 +757,8 @@ static void __submit_bio_noacct_mq(struct bio *bio)
 	current->bio_list = NULL;
 }
 
-void submit_bio_noacct_nocheck(struct bio *bio, bool split)
+void __submit_bio_noacct_nocheck(struct bio *bio, bool split)
 {
-	if (unlikely(blk_error_inject(bio)))
-		return;
-
 	blk_cgroup_bio_start(bio);
 
 	if (!bio_flagged(bio, BIO_TRACE_COMPLETION)) {

@@ -791,6 +788,14 @@ void submit_bio_noacct_nocheck(struct bio *bio, bool split)
 	}
 }
 
+void submit_bio_noacct_nocheck(struct bio *bio, bool split)
+{
+	if (unlikely(blk_error_inject(bio)))
+		return;
+
+	__submit_bio_noacct_nocheck(bio, split);
+}
+
 static blk_status_t blk_validate_atomic_write_op_size(struct request_queue *q,
 						 struct bio *bio)
 {

index 8a8ab961528d..389c9a487067 100644
@@ -61,6 +61,7 @@ bool __blk_freeze_queue_start(struct request_queue *q,
 			      struct task_struct *owner);
 int __bio_queue_enter(struct request_queue *q, struct bio *bio);
 void submit_bio_noacct_nocheck(struct bio *bio, bool split);
+void __submit_bio_noacct_nocheck(struct bio *bio, bool split);
 int bio_submit_or_kill(struct bio *bio, unsigned int flags);
 
 static inline bool blk_try_enter_queue(struct request_queue *q, bool pm)

index 9821d773abab..8b3809d85e83 100644
@@ -13,6 +13,7 @@ static inline bool blk_error_inject(struct bio *bio)
 {
 	if (IS_ENABLED(CONFIG_BLK_ERROR_INJECTION) &&
 	    static_branch_unlikely(&blk_error_injection_enabled) &&
+	    !bio_flagged(bio, BIO_ERROR_INJECTED) &&
 	    test_bit(GD_ERROR_INJECT, &bio->bi_bdev->bd_disk->state))
 		return __blk_error_inject(bio);
 	return false;

index 98e21b4cbf32..50e28dc0f1f9 100644
@@ -323,6 +323,7 @@ enum {
 	BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
 	BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
 	BIO_COMPLETE_IN_TASK, /* complete bi_end_io() in task context */
+	BIO_ERROR_INJECTED, /* error injection rules already applied */
 	BIO_FLAG_LAST
 };
 

2.53.0

The delay does look fine, but I'd really like to not use up one of the scare bio flags for it. Can we insist on the delay only working for an error, so that we never have to reinsert?

We can do that, but that's only half of the feature, and the weaker half IMHO. Only delay with a successful completion covers a more useful scenario for testing in the upper layers.

I am thinking about how else we can do this, but did not find a nice approach.

Maybe we can #ifdef the new flag on CONFIG_BLK_ERROR_INJECTION?

I forgot one small point in favour of the flag.

We can remove addition of __submit_bio_noacct_nocheck() function, since with the flag the re-entry after the delay would fail on blk_error_inject(). This would remove confine the change to error_injection files.

At least I'm not worried about the well-predicted flag, but about using up the very limited flag space. But if this is so important we'll somehow have to live with it. I'll take another closer look to see if I can come up with a better idea.

Thanks.

I can send a clean v3, with the flag, the changes to submit_bio_noacct_nocheck() removed, and the corrections according to your comments. I also found the Documentation changes in this version were big and explanatory, which I have shortened for v3.

Document the delay_us option: the delay happens above the driver, a delayed bio is not run through the rules again, and holding a bio back reorders it against bios submitted later.

---
 Documentation/block/error-injection.rst | 58 ++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

index 81f31af82e65..a1f50d41211d 100644
@@ -9,7 +9,8 @@ Overview
 
 Configurable error injection allows injecting specific block layer status codes
 for sector ranges of a block device.  Errors can be injected unconditionally, or
-with a given probability.
+with a given probability.  Instead of, or before, failing a bio it can also be
+held back for a while to model a slow device.
 
 To use configurable error injection, CONFIG_BLK_ERROR_INJECTION must be enabled.
 

@@ -34,15 +35,60 @@ op=<string>		block layer operation this rule applies to.  This uses
 			the XYZ for each REQ_OP_XYZ operation, e.g. READ, WRITE
 			or DISCARD. Mandatory.
 status=<string>		Status to return.  This uses XYZ for each BLK_STS_XYZ
-			code, e.g. IOERR or MEDIUM. Mandatory.
+			code, e.g. IOERR or MEDIUM. Mandatory unless delay_us
+			is given.
 start=<number>		First block layer sector the rule applies to.
 			Optional, defaults to 0.
 nr_sectors=<number>	Number of sectors this rule applies.
 			Optional, defaults to the remainder of the device.
 chance=<number>		Only return a failure with a likelihood of 1/chance.
 			Optional, defaults to 1 (always).
+delay_us=<number>	Hold the bio back for this many microseconds.  Without
+			status the bio is then submitted to the device as
+			usual, with status it is failed once the delay has
+			expired.  Optional, defaults to 0 (no delay).
+			Values above 600 seconds are rejected.
 ===================	=======================================================
 
+Delays
+------
+
+A delayed bio is held before it is submitted, so the device itself never sees a
+slow I/O: the delay is not visible to the driver, to the I/O statistics, or to
+anything else below submission such as writeback throttling.  Throttling by
+blk-throttle happens before a bio can be delayed, so it is not affected either.
+What it does exercise is everything waiting above the block layer, for instance
+io_uring cancellation, hung task detection, and filesystem or userspace
+timeouts.  Because the low level driver is not involved, a delay does not reach
+the blk-mq timeout handler or SCSI error handling.
+
+Once a bio has been delayed no rule is evaluated for it a second time, not when
+its delay expires and it is submitted below the injection hook, and not when
+the block layer splits it and resubmits the remainder above the hook.  A bio
+that matched a delay rule therefore never gets an error from another rule, even
+one covering the same sectors, and is held for the delay once rather than once
+per split.  Put the delay and the status in a single rule to fail a bio after
+holding it back.
+
+A delayed bio is issued after bios submitted while it was held, which reorders
+the I/O stream.  On zoned devices this breaks sequential write ordering: zone
+write plugging happens below the injection hook, so the writes issued while a
+write is held reach the zone out of order and are failed as misaligned.  Only
+delay reads there.
+
+Bios that must not block are never delayed.  A bio with REQ_NOWAIT set is
+submitted, or failed with the rule's status, immediately.
+
+The delay is a lower bound for anything longer than a timer tick, and the timer
+wheel adds further slack as the delay grows.  Values shorter than a tick are of
+little use: they expire on the next tick, which is anywhere between now and one
+tick away.
+
+Removing rules does not release bios that are already being delayed by them;
+those run out on their own.  A delayed bio whose disk is removed in the meantime
+is not submitted until its delay expires, by which point the queue no longer
+accepts I/O, so it fails with EIO.
+
 Example
 -------
 

@@ -54,6 +100,14 @@ Return BLK_STS_MEDIUM for every write to /dev/nvme0n1:
 
 	$ echo 'add,op=WRITE,start=0,status=MEDIUM' > /sys/kernel/debug/block/nvme0n1/error_injection
 
+Delay every read of /dev/nvme0n1 by 10 milliseconds, then issue it normally:
+
+	$ echo 'add,op=READ,delay_us=10000' > /sys/kernel/debug/block/nvme0n1/error_injection
+
+Fail one in 100 writes with BLK_STS_TIMEOUT, but only after 30 seconds:
+
+	$ echo 'add,op=WRITE,status=TIMEOUT,chance=100,delay_us=30000000' > /sys/kernel/debug/block/nvme0n1/error_injection
+
 Remove all rules for /dev/nvme0n1:
 
 	$ echo 'removeall' > /sys/kernel/debug/block/nvme0n1/error_injection

2.53.0