Re: [PATCH 2/2] nvme: use blk-mq polling for uring commands

[Keith Busch <kbusch@kernel.org>]

On Mon, Mar 27, 2023 at 07:28:10PM +0530, Kanchan Joshi wrote:
> > -	}
> > +	if (blk_rq_is_poll(req))
> > +		WRITE_ONCE(ioucmd->cookie, req);
> 
> blk_rq_is_poll(req) warns for null "req->bio" and returns false if that
> is the case. That defeats one of the purpose of the series i.e. poll on
> no-payload commands such as flush/write-zeroes.

Sorry, I'm sending out various patches piecemeal. This patch here depends on
this one sent out earlier:

  https://lore.kernel.org/linux-block/3f670ca7-908d-db55-3da1-4090f116005d@nvidia.com/T/#mbc6174ce3f9dbae38ae2ca646518be4bf105f6e4

> > 	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's safe to check the cookie like this and rely on its contents. The queue's
hctx's can't change within an rcu section, and the cookie is cleared in the
completion path prior to the request being free'd. In the worst case, we're
racing another polling thread completing our request while simultaneously
trying to renumber the hctx's, but the request and the current hctx it points
are reliable if we see non-NULL.

> If driver is loaded without the poll-queues, we will not be able
> to set req into ioucmd->cookie during the submission (in
> nvme_uring_cmd_io). Therefore, the original code checked for QUEUE_FLAG_POLL
> before treating ioucmd->cookie as bio here.

You don't need to check the queue's FLAG_POLL after the request is allocated.
The user can't change this directly, and this flag can't be changed with
requests in flight, so checking blk_rq_is_poll() is the only thing we need to
rely on.

> This should handle it (on top of your patch):

This doesn't work with multipath.