diff mbox series

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

Message ID 20250408072440.1977943-3-ming.lei@redhat.com (mailing list archive)
State New
Headers show
Series ublk: two fixes | expand

Commit Message

Ming Lei April 8, 2025, 7:24 a.m. UTC 
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.

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()")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Uday Shankar April 8, 2025, 7:11 p.m. UTC | #1 
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
>
Ming Lei April 9, 2025, 1:09 a.m. UTC | #2 
On Tue, Apr 08, 2025 at 01:11:52PM -0600, Uday Shankar wrote:
> 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);
> }

Indeed, will update commit log.

> 
> > 
> > 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()")

Will fix it in V2.

Thanks
Ming
diff mbox series

Patch

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);