Re: [PATCH 2/2] ublk: don't fail request for recovery & reissue in case of ubq->canceling

[Uday Shankar <ushankar@purestorage.com>]

On Tue, Apr 08, 2025 at 03:24:38PM +0800, Ming Lei wrote:
> ubq->canceling is set with request queue quiesced when io_uring context is
> exiting. Recovery & reissue(UBLK_F_USER_RECOVERY_REISSUE) requires
> request to be re-queued and re-dispatch after device is recovered.
> 
> However commit d796cea7b9f3 ("ublk: implement ->queue_rqs()") still may
> fail any request in case of ubq->canceling, this way breaks
> UBLK_F_USER_RECOVERY_REISSUE.

This change actually affects UBLK_F_USER_RECOVERY as long as FAIL_IO
isn't set (regardless of RECOVERY_REISSUE). RECOVERY_REISSUE only
changes behavior for requests that are already in the ublk server when
the ublk server dies, but the code change only affects requests that
come in after ublk server death is already detected. At that point, both
plain USER_RECOVERY and USER_RECOVERY|USER_RECOVERY_REISSUE should see
requests queue instead of completing with error. See below code
snippets:

static inline void __ublk_abort_rq(struct ublk_queue *ubq,
		struct request *rq)
{
	/* We cannot process this rq so just requeue it. */
	if (ublk_nosrv_dev_should_queue_io(ubq->dev))
		blk_mq_requeue_request(rq, false);
	else
		blk_mq_end_request(rq, BLK_STS_IOERR);
}

/*
 * Should I/O issued while there is no ublk server queue? If not, I/O
 * issued while there is no ublk server will get errors.
 */
static inline bool ublk_nosrv_dev_should_queue_io(struct ublk_device *ub)
{
	return (ub->dev_info.flags & UBLK_F_USER_RECOVERY) &&
	       !(ub->dev_info.flags & UBLK_F_USER_RECOVERY_FAIL_IO);
}

> 
> Fix it by calling __ublk_abort_rq() in case of ubq->canceling.
> 
> Reported-by: Uday Shankar <ushankar@purestorage.com>
> Closes: https://lore.kernel.org/linux-block/Z%2FQkkTRHfRxtN%2FmB@dev-ushankar.dev.purestorage.com/
> Fixes: commit d796cea7b9f3 ("ublk: implement ->queue_rqs()")

Will this upset anything parsing these tags? The syntax I've usually
seen doesn't have "commit," i.e.

Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")

> Signed-off-by: Ming Lei <ming.lei@redhat.com>

Code change looks good.

Reviewed-by: Uday Shankar <ushankar@purestorage.com>

> ---
>  drivers/block/ublk_drv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 41bed67508f2..d6ca2f1097ad 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1371,7 +1371,8 @@ static enum blk_eh_timer_return ublk_timeout(struct request *rq)
>  	return BLK_EH_RESET_TIMER;
>  }
>  
> -static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
> +static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq,
> +				  bool check_cancel)
>  {
>  	blk_status_t res;
>  
> @@ -1390,7 +1391,7 @@ static blk_status_t ublk_prep_req(struct ublk_queue *ubq, struct request *rq)
>  	if (ublk_nosrv_should_queue_io(ubq) && unlikely(ubq->force_abort))
>  		return BLK_STS_IOERR;
>  
> -	if (unlikely(ubq->canceling))
> +	if (check_cancel && unlikely(ubq->canceling))
>  		return BLK_STS_IOERR;
>  
>  	/* fill iod to slot in io cmd buffer */
> @@ -1409,7 +1410,7 @@ static blk_status_t ublk_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	struct request *rq = bd->rq;
>  	blk_status_t res;
>  
> -	res = ublk_prep_req(ubq, rq);
> +	res = ublk_prep_req(ubq, rq, false);
>  	if (res != BLK_STS_OK)
>  		return res;
>  
> @@ -1441,7 +1442,7 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
>  			ublk_queue_cmd_list(ubq, &submit_list);
>  		ubq = this_q;
>  
> -		if (ublk_prep_req(ubq, req) == BLK_STS_OK)
> +		if (ublk_prep_req(ubq, req, true) == BLK_STS_OK)
>  			rq_list_add_tail(&submit_list, req);
>  		else
>  			rq_list_add_tail(&requeue_list, req);
> -- 
> 2.47.0
>