| Message ID | 20181211233652.9705-5-sagi@grimberg.me (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | restore polling to nvme-rdma | expand |
[adding Jens] > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1547,6 +1547,8 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id) > if (ns->head->disk) { > nvme_update_disk_info(ns->head->disk, ns, id); > blk_queue_stack_limits(ns->head->disk->queue, ns->queue); > + /* XXX: multipath device does not support polling for now... */ > + blk_queue_flag_clear(QUEUE_FLAG_POLL, ns->queue); I'd drop the XXX. But I think we actually have a block layer problem here. Currently stacking devices will just pass through REQ_HIPRI, despite none of them supporting any polling for it. So we need to make sure in the block layer or I/O submitter that REQ_HIPRI is only set if QUEUE_FLAG_POLL is supported. I think it would also help if we rename it to REQ_POLL to make this more obvious.
> [adding Jens] > >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -1547,6 +1547,8 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id) >> if (ns->head->disk) { >> nvme_update_disk_info(ns->head->disk, ns, id); >> blk_queue_stack_limits(ns->head->disk->queue, ns->queue); >> + /* XXX: multipath device does not support polling for now... */ >> + blk_queue_flag_clear(QUEUE_FLAG_POLL, ns->queue); > > I'd drop the XXX. But I think we actually have a block layer problem > here. Currently stacking devices will just pass through REQ_HIPRI, > despite none of them supporting any polling for it. Yea... forgot there are other stack devices... > So we need to make sure in the block layer or I/O submitter that > REQ_HIPRI is only set if QUEUE_FLAG_POLL is supported. I think it would > also help if we rename it to REQ_POLL to make this more obvious. It used to check for it, but was changed to look at nr_maps instead... So I think this is a regression...
On Tue, Dec 11, 2018 at 11:19:22PM -0800, Sagi Grimberg wrote: >> So we need to make sure in the block layer or I/O submitter that >> REQ_HIPRI is only set if QUEUE_FLAG_POLL is supported. I think it would >> also help if we rename it to REQ_POLL to make this more obvious. > > It used to check for it, but was changed to look at nr_maps instead... > So I think this is a regression... blk_mq_map_queue looks at both QUEUE_FLAG_POLL and nr_maps for the queue selection, but that doesn't help with the problem that we should never pass poll requests through stacking devices.
>>> So we need to make sure in the block layer or I/O submitter that >>> REQ_HIPRI is only set if QUEUE_FLAG_POLL is supported. I think it would >>> also help if we rename it to REQ_POLL to make this more obvious. >> >> It used to check for it, but was changed to look at nr_maps instead... >> So I think this is a regression... > > blk_mq_map_queue looks at both QUEUE_FLAG_POLL and nr_maps for the > queue selection, but that doesn't help with the problem that we should > never pass poll requests through stacking devices. Yea I agree.. Would probably make better sense to forbid it in the core, but I'm not entirely sure how this is possible make_request drivers...
On Tue, Dec 11, 2018 at 11:29:33PM -0800, Sagi Grimberg wrote: > Yea I agree.. Would probably make better sense to forbid it in the core, > but I'm not entirely sure how this is possible make_request drivers... Either we can clear the flag in generic_make_request when QUEUE_FLAG_POLL is not set, or just change the few users to never even set it in that case.
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index f90576862736..511d399a6002 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -1547,6 +1547,8 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id) if (ns->head->disk) { nvme_update_disk_info(ns->head->disk, ns, id); blk_queue_stack_limits(ns->head->disk->queue, ns->queue); + /* XXX: multipath device does not support polling for now... */ + blk_queue_flag_clear(QUEUE_FLAG_POLL, ns->queue); } #endif }
Since the multipath device does not support polling (yet) we cannot pass requests to the polling queue map as those will not generate interrupt so we cannot reap the completion. Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/nvme/host/core.c | 2 ++ 1 file changed, 2 insertions(+)