Re: [RFC PATCH 3/3] nvme: Introduce nvme_execute_passthru_rq_nowait()

[Logan Gunthorpe]

On 2019-10-25 2:41 p.m., Sagi Grimberg wrote:
>> +#ifdef CONFIG_NVME_TARGET_PASSTHRU
>> +static void nvme_execute_passthru_rq_work(struct work_struct *w)
>> +{
>> +ÂÂÂ struct nvme_request *req = container_of(w, struct nvme_request,
>> work);
>> +ÂÂÂ struct request *rq = blk_mq_rq_from_pdu(req);
>> +ÂÂÂ rq_end_io_fn *done = rq->end_io;
>> +ÂÂÂ void *end_io_data = rq->end_io_data;
> 
> Why is end_io_data stored to a local variable here? where is it set?

blk_execute_rq() (which is called by nvme_execute_rq()) will overwrite
rq->endio and rq->end_io_data. We store them here so we can call the
callback appropriately after the request completes. It would be set by
the caller in the same way they set it if they were calling
blk_execute_rq_nowait().

>> +
>> +ÂÂÂ nvme_execute_passthru_rq(rq);
>> +
>> +ÂÂÂ if (done) {
>> +ÂÂÂÂÂÂÂ rq->end_io_data = end_io_data;
>> +ÂÂÂÂÂÂÂ done(rq, 0);
>> +ÂÂÂ }
>> +}
>> +
>> +void nvme_execute_passthru_rq_nowait(struct request *rq, rq_end_io_fn
>> *done)
>> +{
>> +ÂÂÂ struct nvme_command *cmd = nvme_req(rq)->cmd;
>> +ÂÂÂ struct nvme_ctrl *ctrl = nvme_req(rq)->ctrl;
>> +ÂÂÂ struct nvme_ns *ns = rq->q->queuedata;
>> +ÂÂÂ struct gendisk *disk = ns ? ns->disk : NULL;
>> +ÂÂÂ u32 effects;
>> +
>> +ÂÂÂ /*
>> +ÂÂÂÂ * This function may be called in interrupt context, so we cannot
>> sleep
>> +ÂÂÂÂ * but nvme_passthru_[start|end]() may sleep so we need to execute
>> +ÂÂÂÂ * the command in a work queue.
>> +ÂÂÂÂ */
>> +ÂÂÂ effects = nvme_command_effects(ctrl, ns, cmd->common.opcode);
>> +ÂÂÂ if (effects) {
>> +ÂÂÂÂÂÂÂ rq->end_io = done;
>> +ÂÂÂÂÂÂÂ INIT_WORK(&nvme_req(rq)->work, nvme_execute_passthru_rq_work);
>> +ÂÂÂÂÂÂÂ queue_work(nvme_wq, &nvme_req(rq)->work);
> 
> This work will need to be flushed when in nvme_stop_ctrl. That is
> assuming that it will fail-fast and not hang (which it should given
> that its a passthru command that is allocated via nvme_alloc_request).

Hmm, that's going to be a bit tricky. Seeing the work_struct belongs
potentially to a number of different requests, we can't just flush the
individual work items. I think we'd have to create a work-queue per ctrl
and flush that. Any objections to that?

Logan