Re: [PATCH V2] block: ignore RWF_HIPRI hint for sync dio

[Keith Busch <kbusch@kernel.org>]

On Tue, May 24, 2022 at 08:40:46AM +0800, Ming Lei wrote:
> On Mon, May 23, 2022 at 04:36:04PM -0600, Keith Busch wrote:
> > On Wed, Apr 20, 2022 at 10:31:10PM +0800, Ming Lei wrote:
> > > So far bio is marked as REQ_POLLED if RWF_HIPRI/IOCB_HIPRI is passed
> > > from userspace sync io interface, then block layer tries to poll until
> > > the bio is completed. But the current implementation calls
> > > blk_io_schedule() if bio_poll() returns 0, and this way causes io hang or
> > > timeout easily.
> > 
> > Wait a second. The task's current state is TASK_RUNNING when bio_poll() returns
> > zero, so calling blk_io_schedule() isn't supposed to hang.
> 
> void __sched io_schedule(void)
> {
>         int token;
> 
>         token = io_schedule_prepare();
>         schedule();
>         io_schedule_finish(token);
> }
> 
> But who can wakeup this task after scheduling out? There can't be irq
> handler for POLLED request.

No one. If everything was working, the task state would be RUNNING, so it is
immediately available to be scheduled back in.
 
> The hang can be triggered on nvme/qemu reliably:

And clearly it's not working, but for a different reason. The polling thread
sees an invalid cookie and fails to set the task back to RUNNING, so yes, it
will sleep with no waker in the current code.

We usually expect the cookie to be set inline with submit_bio(), but we're not
guaranteed it won't be punted off to .run_work for a variety of reasons, so
the thread writing the cookie may be racing with the reader.

This was fine before the bio polling support since the cookie was always
returned with submit_bio() before that.

And I would like psync to continue working with polling. As great as io_uring
is, it's just not as efficient @qd1.

Here's a bandaid, though I assume it'll break something...

---
diff --git a/block/blk-mq.c b/block/blk-mq.c
index ed1869a305c4..348136dc7ba9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1146,8 +1146,6 @@ void blk_mq_start_request(struct request *rq)
 	if (blk_integrity_rq(rq) && req_op(rq) == REQ_OP_WRITE)
 		q->integrity.profile->prepare_fn(rq);
 #endif
-	if (rq->bio && rq->bio->bi_opf & REQ_POLLED)
-	        WRITE_ONCE(rq->bio->bi_cookie, blk_rq_to_qc(rq));
 }
 EXPORT_SYMBOL(blk_mq_start_request);
 
@@ -2464,6 +2462,9 @@ static void blk_mq_bio_to_request(struct request *rq, struct bio *bio,
 	WARN_ON_ONCE(err);
 
 	blk_account_io_start(rq);
+
+	if (rq->bio->bi_opf & REQ_POLLED)
+	        WRITE_ONCE(rq->bio->bi_cookie, blk_rq_to_qc(rq));
 }
 
 static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx,
--