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

[Logan Gunthorpe <logang@deltatee.com>]

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme