Re: [PATCH 3/4] ublk: cancel commits arriving after the quiesce scan

[Ming Lei <tom.leiming@gmail.com>]

On Wed, Nov 11, 2026 at 10:58:01PM +0800, shao.mingyin@zte.com.cn wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> QUIESCE_DEV marks the queues canceling and cancels pending commands.
> A server can still be processing an I/O when that scan passes its slot.
> If it submits COMMIT_AND_FETCH_REQ afterwards, the newly published
> command can remain queued without a future request to complete it.
> The server waits for that completion and the device remains LIVE.

The issue is real, and it can be reproduced without any test hook:

        kublk add -t fault_inject -q 1 -r 1 --delay_us 3000000
        dd if=/dev/zero of=/dev/ublkbN oflag=direct bs=4k count=1 &
        sleep 1
        kublk quiesce -n N

Without this patch the device stays LIVE after the delayed commit
lands; with it the device reaches QUIESCED. Please add this as a new
recovery selftest in v2, so the fix is covered.

> 
> Share the cancellation claim with the command handler and use it after
> processing a commit when the queue is canceling. The cancel lock and
> CANCELED flag ensure that the control path and command handler cannot
> complete the same command twice.

The commit log needs to cover a few more things:

1) ublk_cancel_cmd() refuses to cancel while the request of this tag is
started, and the new call site skips that check. With zero copy or auto
buffer registration the committed request may still be in flight when
the command is aborted here. I think it is safe, because ->canceling is
already set, so any new request on this tag is aborted in
ublk_queue_rq() and never dispatched to this slot, but please explain
that in the commit log.

2) If ublk_wait_for_idle_io() fails with -EBUSY or -EINTR, ->canceling
is left set and ublk_cancel_dev() isn't called. With this patch the
server gets UBLK_IO_RES_ABORT on its next commit even though QUIESCE_DEV
returned an error. That may be fine, but it is a behavior change and
should be documented.

3) The new check fires whenever ->canceling is set, not only for
QUIESCE_DEV, so the subject and commit log are narrower than the change.

> 
> Fixes: b465ae7b2524 ("ublk: add feature UBLK_F_QUIESCE")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> Tested on next-20261105 with CONFIG_BLK_DEV_UBLK=y in a local QEMU/KVM
> VM (4 vCPUs, 4 GiB), using real ublk devices and fio. A test-only server
> hook held one COMMIT across a successful quiesce, then released it.
> In two runs per version, the original kernel left the device LIVE until
> a second quiesce; the fixed kernel reached QUIESCED without that retry.
> The paired runs used the same userspace probe and no kernel diagnostics.
> 
> The four original recovery scripts also passed twice with this change
> and a separate batch SQE initialization fix, without C or driver
> diagnostic instrumentation or reduced fio load.
> 
>  drivers/block/ublk_drv.c |   40 ++++++++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 3d322fa1b..082f105f1 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -2792,14 +2792,33 @@ static void ublk_start_cancel(struct ublk_device *ub)
>  	ublk_put_disk(disk);
>  }
>  
> +static void ublk_complete_cancelled_cmd(struct ublk_queue *ubq,
> +					struct ublk_io *io,
> +					unsigned int issue_flags)


ublk_drv.c uses "canceled"/"canceling" everywhere, so please name it
ublk_complete_canceled_cmd().


> +{
> +	struct io_uring_cmd *cmd = NULL;
> +	bool done;
> +
> +	spin_lock(&ubq->cancel_lock);
> +	done = !!(io->flags & UBLK_IO_FLAG_CANCELED);
> +	if (!done) {
> +		io->flags |= UBLK_IO_FLAG_CANCELED;
> +		/* dependency ordered against smp_wmb() in ublk_prep_cancel() */
> +		cmd = READ_ONCE(io->cmd);
> +		io->cmd = NULL;
> +	}
> +	spin_unlock(&ubq->cancel_lock);
> +
> +	if (!done && cmd)
> +		io_uring_cmd_done(cmd, UBLK_IO_RES_ABORT, issue_flags);
> +}
> +
>  static void ublk_cancel_cmd(struct ublk_queue *ubq, u16 tag,
>  		unsigned int issue_flags)
>  {
>  	struct ublk_io *io = &ubq->ios[tag];
>  	struct ublk_device *ub = ubq->dev;
> -	struct io_uring_cmd *cmd = NULL;
>  	struct request *req;
> -	bool done;
>  
>  	if (!(io->flags & UBLK_IO_FLAG_ACTIVE))
>  		return;
> @@ -2818,18 +2837,7 @@ static void ublk_cancel_cmd(struct ublk_queue *ubq, u16 tag,
>  	if (req && blk_mq_request_started(req) && req->tag == tag)
>  		return;
>  
> -	spin_lock(&ubq->cancel_lock);
> -	done = !!(io->flags & UBLK_IO_FLAG_CANCELED);
> -	if (!done) {
> -		io->flags |= UBLK_IO_FLAG_CANCELED;
> -		/* dependency ordered against smp_wmb() in ublk_prep_cancel() */
> -		cmd = READ_ONCE(io->cmd);
> -		io->cmd = NULL;
> -	}
> -	spin_unlock(&ubq->cancel_lock);
> -
> -	if (!done && cmd)
> -		io_uring_cmd_done(cmd, UBLK_IO_RES_ABORT, issue_flags);
> +	ublk_complete_cancelled_cmd(ubq, io, issue_flags);
>  }
>  
>  /*
> @@ -3560,6 +3568,10 @@ static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
>  			req->__sector = addr;
>  		if (compl)
>  			__ublk_complete_rq(req, io, ublk_dev_need_map_io(ub), NULL);
> +
> +		/* Quiesce may have scanned this slot before this command arrived. */
> +		if (unlikely(READ_ONCE(ubq->canceling)))
> +			ublk_complete_cancelled_cmd(ubq, io, issue_flags);
>  		break;

io->cmd and UBLK_IO_FLAG_ACTIVE are stored here before ->canceling is
read, with no barrier in between, while the cancel scan tests
UBLK_IO_FLAG_ACTIVE locklessly after setting ->canceling. Either side
must see the other's store. That holds in practice because ->canceling
is set under blk_mq_quiesce_queue(), but please say so in the comment
instead of leaving it implicit.

>  	}
>  	case UBLK_IO_NEED_GET_DATA:

UBLK_IO_NEED_GET_DATA publishes io->cmd in the same way. If                                                                                                                                     
ublk_get_data() fails and falls back to requeue, the command is kept
active, and with ->canceling set the requeued request is aborted in
ublk_queue_rq(), so this command looks stranded just like the commit
case. I haven't reproduced that one; can you check, and cover it in v2
if so?

Thanks,
Ming