Re: [PATCH 2/2] selftests: ublk: test that teardown after incomplete recovery completes

[Ming Lei <ming.lei@redhat.com>]

On Fri, Apr 03, 2026 at 09:23:56PM -0600, Uday Shankar wrote:
> Before the fix, teardown of a ublk server that was attempting to recover
> a device, but died when it had submitted a nonempty proper subset of the
> fetch commands to any queue would loop forever. Add a test to verify
> that, after the fix, teardown completes. This is done by:
> 
> - Adding a new argument to the fault_inject target that causes it die
>   after fetching a nonempty proper subset of the IOs to a queue
> - Using that argument in a new test while trying to recover an
>   already-created device
> - Attempting to delete the ublk device at the end of the test; this
>   hangs forever if teardown from the fault-injected ublk server never
>   completed.
> 
> It was manually verified that the test passes with the fix and hangs
> without it.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
>  tools/testing/selftests/ublk/Makefile           |  1 +
>  tools/testing/selftests/ublk/fault_inject.c     | 51 +++++++++++++++++++++++--
>  tools/testing/selftests/ublk/kublk.c            |  4 ++
>  tools/testing/selftests/ublk/kublk.h            |  3 ++
>  tools/testing/selftests/ublk/test_generic_17.sh | 35 +++++++++++++++++
>  5 files changed, 91 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/ublk/Makefile b/tools/testing/selftests/ublk/Makefile
> index 8ac2d4a682a1768fb1eb9d2dd2a5d01294a67a03..d338668c5a5fbd73f6d70165455a3551ab13e894 100644
> --- a/tools/testing/selftests/ublk/Makefile
> +++ b/tools/testing/selftests/ublk/Makefile
> @@ -18,6 +18,7 @@ TEST_PROGS += test_generic_10.sh
>  TEST_PROGS += test_generic_12.sh
>  TEST_PROGS += test_generic_13.sh
>  TEST_PROGS += test_generic_16.sh
> +TEST_PROGS += test_generic_17.sh
>  
>  TEST_PROGS += test_batch_01.sh
>  TEST_PROGS += test_batch_02.sh
> diff --git a/tools/testing/selftests/ublk/fault_inject.c b/tools/testing/selftests/ublk/fault_inject.c
> index 3b897f69c014cc73b4b469d816e80284dd21b577..228a9605053409c84baaf255f97c4abc271a8bfd 100644
> --- a/tools/testing/selftests/ublk/fault_inject.c
> +++ b/tools/testing/selftests/ublk/fault_inject.c
> @@ -10,11 +10,17 @@
>  
>  #include "kublk.h"
>  
> +struct fi_opts {
> +	long long delay_ns;
> +	bool die_during_fetch;
> +};
> +
>  static int ublk_fault_inject_tgt_init(const struct dev_ctx *ctx,
>  				      struct ublk_dev *dev)
>  {
>  	const struct ublksrv_ctrl_dev_info *info = &dev->dev_info;
>  	unsigned long dev_size = 250UL << 30;
> +	struct fi_opts *opts = NULL;
>  
>  	if (ctx->auto_zc_fallback) {
>  		ublk_err("%s: not support auto_zc_fallback\n", __func__);
> @@ -35,17 +41,51 @@ static int ublk_fault_inject_tgt_init(const struct dev_ctx *ctx,
>  	};
>  	ublk_set_integrity_params(ctx, &dev->tgt.params);
>  
> -	dev->private_data = (void *)(unsigned long)(ctx->fault_inject.delay_us * 1000);
> +	opts = calloc(1, sizeof(*opts));
> +	if (!opts) {
> +		ublk_err("%s: couldn't allocate memory for opts\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	opts->delay_ns = ctx->fault_inject.delay_us * 1000;
> +	opts->die_during_fetch = ctx->fault_inject.die_during_fetch;
> +	dev->private_data = opts;
> +
>  	return 0;
>  }
>  
> +static void ublk_fault_inject_pre_fetch_io(struct ublk_thread *t,
> +					   struct ublk_queue *q, int tag)
> +{
> +	struct fi_opts *opts = q->dev->private_data;
> +
> +	if (!opts->die_during_fetch)
> +		return;
> +
> +	/*
> +	 * Each queue fetches its IOs in increasing order of tags, so
> +	 * dying just before we're about to fetch tag 1 (regardless of
> +	 * what queue we're on) guarantees that we've fetched a nonempty
> +	 * proper subset of the tags on that queue.
> +	 */
> +	if (tag == 1) {
> +		/*
> +		 * Ensure our commands are actually live in the kernel
> +		 * before we die.
> +		 */
> +		io_uring_submit(&t->ring);
> +		raise(SIGKILL);
> +	}
> +}
> +
>  static int ublk_fault_inject_queue_io(struct ublk_thread *t,
>  				      struct ublk_queue *q, int tag)
>  {
>  	const struct ublksrv_io_desc *iod = ublk_get_iod(q, tag);
>  	struct io_uring_sqe *sqe;
> +	struct fi_opts *opts = q->dev->private_data;
>  	struct __kernel_timespec ts = {
> -		.tv_nsec = (long long)q->dev->private_data,
> +		.tv_nsec = opts->delay_ns,
>  	};
>  
>  	ublk_io_alloc_sqes(t, &sqe, 1);
> @@ -77,29 +117,34 @@ static void ublk_fault_inject_cmd_line(struct dev_ctx *ctx, int argc, char *argv
>  {
>  	static const struct option longopts[] = {
>  		{ "delay_us", 	1,	NULL,  0  },
> +		{ "die_during_fetch", 1, NULL, 0  },
>  		{ 0, 0, 0, 0 }
>  	};
>  	int option_idx, opt;
>  
>  	ctx->fault_inject.delay_us = 0;
> +	ctx->fault_inject.die_during_fetch = false;
>  	while ((opt = getopt_long(argc, argv, "",
>  				  longopts, &option_idx)) != -1) {
>  		switch (opt) {
>  		case 0:
>  			if (!strcmp(longopts[option_idx].name, "delay_us"))
>  				ctx->fault_inject.delay_us = strtoll(optarg, NULL, 10);
> +			if (!strcmp(longopts[option_idx].name, "die_during_fetch"))
> +				ctx->fault_inject.die_during_fetch = strtoll(optarg, NULL, 10);
>  		}
>  	}
>  }
>  
>  static void ublk_fault_inject_usage(const struct ublk_tgt_ops *ops)
>  {
> -	printf("\tfault_inject: [--delay_us us (default 0)]\n");
> +	printf("\tfault_inject: [--delay_us us (default 0)] [--die_during_fetch 1]\n");
>  }
>  
>  const struct ublk_tgt_ops fault_inject_tgt_ops = {
>  	.name = "fault_inject",
>  	.init_tgt = ublk_fault_inject_tgt_init,
> +	.pre_fetch_io = ublk_fault_inject_pre_fetch_io,
>  	.queue_io = ublk_fault_inject_queue_io,
>  	.tgt_io_done = ublk_fault_inject_tgt_io_done,
>  	.parse_cmd_line = ublk_fault_inject_cmd_line,
> diff --git a/tools/testing/selftests/ublk/kublk.c b/tools/testing/selftests/ublk/kublk.c
> index e1c3b3c55e565c8cad6b6fe9b9b764cd244818c0..8260c96a39c05584065f41a52f3d9050614454c6 100644
> --- a/tools/testing/selftests/ublk/kublk.c
> +++ b/tools/testing/selftests/ublk/kublk.c
> @@ -796,6 +796,8 @@ static void ublk_submit_fetch_commands(struct ublk_thread *t)
>  			q = &t->dev->q[q_id];
>  			io = &q->ios[tag];
>  			io->buf_index = j++;
> +			if (q->tgt_ops->pre_fetch_io)
> +				q->tgt_ops->pre_fetch_io(t, q, tag);
>  			ublk_queue_io_cmd(t, io);
>  		}
>  	} else {
> @@ -807,6 +809,8 @@ static void ublk_submit_fetch_commands(struct ublk_thread *t)
>  		for (i = 0; i < q->q_depth; i++) {
>  			io = &q->ios[i];
>  			io->buf_index = i;
> +			if (q->tgt_ops->pre_fetch_io)
> +				q->tgt_ops->pre_fetch_io(t, q, i);
>  			ublk_queue_io_cmd(t, io);
>  		}
>  	}

The callback needs to be called in ublk_batch_setup_queues() for F_BATCH
too.

Otherwise, this patch looks good.


Thanks,
Ming