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

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

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