On Mon, Mar 27, 2023 at 06:48:30PM -0600, Keith Busch wrote:
On Mon, Mar 27, 2023 at 10:50:47PM +0530, Kanchan Joshi wrote:On Mon, Mar 27, 2023 at 8:59 PM Keith Busch <kbusch@xxxxxxxxxx> wrote: > > > rcu_read_lock(); > > > - bio = READ_ONCE(ioucmd->cookie); > > > - ns = container_of(file_inode(ioucmd->file)->i_cdev, > > > - struct nvme_ns, cdev); > > > - q = ns->queue; > > > - if (test_bit(QUEUE_FLAG_POLL, &q->queue_flags) && bio && bio->bi_bdev) > > > - ret = bio_poll(bio, iob, poll_flags); > > > + req = READ_ONCE(ioucmd->cookie); > > > + if (req) { > > > > This is risky. We are not sure if the cookie is actually "req" at this > > moment. > > What else could it be? It's either a real request from a polled hctx tag, or > NULL at this point. It can also be a function pointer that gets assigned on irq-driven completion. See the "struct io_uring_cmd" - we are tight on cacheline, so cookie and task_work_cb share the storage. > It's safe to check the cookie like this and rely on its contents. Hence not safe. Please try running this without poll-queues (at nvme level), you'll see failures.Okay, you have a iouring polling instance used with a file that has poll capabilities, but doesn't have any polling hctx's. It would be nice to exclude these from io_uring's polling since they're wasting CPU time, but that doesn't look easily done.
Do you mean having the ring with IOPOLL set, and yet skip the attempt of actively reaping the completion for certain IOs?
This simple patch atop should work though. --- diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c index 369e8519b87a2..e3ff019404816 100644 --- a/drivers/nvme/host/ioctl.c +++ b/drivers/nvme/host/ioctl.c @@ -612,6 +612,8 @@ static int nvme_uring_cmd_io(struct nvme_ctrl *ctrl, struct nvme_ns *ns, if (blk_rq_is_poll(req)) WRITE_ONCE(ioucmd->cookie, req); + else if (issue_flags & IO_URING_F_IOPOLL) + ioucmd->flags |= IORING_URING_CMD_NOPOLL;
If IO_URING_F_IOPOLL would have come here as part of "ioucmd->flags", we could have just cleared that here. That would avoid the need of NOPOLL flag. That said, I don't feel strongly about new flag too. You decide.
