lkml.org 
[lkml]   [2020]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.7 054/265] nvmet: fail outstanding host posted AEN req
    Date
    From: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>

    [ Upstream commit 819f7b88b48fd2bce932cfe3a38bf8fcefbcabe7 ]

    In function nvmet_async_event_process() we only process AENs iff
    there is an open slot on the ctrl->async_event_cmds[] && aen
    event list posted by the target is not empty. This keeps host
    posted AEN outstanding if target generated AEN list is empty.
    We do cleanup the target generated entries from the aen list in
    nvmet_ctrl_free()-> nvmet_async_events_free() but we don't
    process AEN posted by the host. This leads to following problem :-

    When processing admin sq at the time of nvmet_sq_destroy() holds
    an extra percpu reference(atomic value = 1), so in the following code
    path after switching to atomic rcu, release function (nvmet_sq_free())
    is not getting called which blocks the sq->free_done in
    nvmet_sq_destroy() :-

    nvmet_sq_destroy()
    percpu_ref_kill_and_confirm()
    - __percpu_ref_switch_mode()
    -- __percpu_ref_switch_to_atomic()
    --- call_rcu() -> percpu_ref_switch_to_atomic_rcu()
    ---- /* calls switch callback */
    - percpu_ref_put()
    -- percpu_ref_put_many(ref, 1)
    --- else if (unlikely(atomic_long_sub_and_test(nr, &ref->count)))
    ---- ref->release(ref); <---- Not called.

    This results in indefinite hang:-

    void nvmet_sq_destroy(struct nvmet_sq *sq)
    ...
    if (ctrl && ctrl->sqs && ctrl->sqs[0] == sq) {
    nvmet_async_events_process(ctrl, status);
    percpu_ref_put(&sq->ref);
    }
    percpu_ref_kill_and_confirm(&sq->ref, nvmet_confirm_sq);
    wait_for_completion(&sq->confirm_done);
    wait_for_completion(&sq->free_done); <-- Hang here

    Which breaks the further disconnect sequence. This problem seems to be
    introduced after commit 64f5e9cdd711b ("nvmet: fix memory leak when
    removing namespaces and controllers concurrently").

    This patch processes ctrl->async_event_cmds[] in the admin sq destroy()
    context irrespetive of aen_list. Also we get rid of the controller's
    aen_list processing in the nvmet_sq_destroy() context and just ignore
    ctrl->aen_list.

    This results in nvmet_async_events_process() being called from workqueue
    context so we adjust the code accordingly.

    Fixes: 64f5e9cdd711 ("nvmet: fix memory leak when removing namespaces and controllers concurrently ")
    Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
    Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Jens Axboe <axboe@kernel.dk>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    drivers/nvme/target/core.c | 27 ++++++++++++++++++++-------
    1 file changed, 20 insertions(+), 7 deletions(-)

    diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
    index ac7ae031ce78d..96deaf3484662 100644
    --- a/drivers/nvme/target/core.c
    +++ b/drivers/nvme/target/core.c
    @@ -129,7 +129,22 @@ static u32 nvmet_async_event_result(struct nvmet_async_event *aen)
    return aen->event_type | (aen->event_info << 8) | (aen->log_page << 16);
    }

    -static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
    +static void nvmet_async_events_failall(struct nvmet_ctrl *ctrl)
    +{
    + u16 status = NVME_SC_INTERNAL | NVME_SC_DNR;
    + struct nvmet_req *req;
    +
    + mutex_lock(&ctrl->lock);
    + while (ctrl->nr_async_event_cmds) {
    + req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
    + mutex_unlock(&ctrl->lock);
    + nvmet_req_complete(req, status);
    + mutex_lock(&ctrl->lock);
    + }
    + mutex_unlock(&ctrl->lock);
    +}
    +
    +static void nvmet_async_events_process(struct nvmet_ctrl *ctrl)
    {
    struct nvmet_async_event *aen;
    struct nvmet_req *req;
    @@ -139,14 +154,13 @@ static void nvmet_async_events_process(struct nvmet_ctrl *ctrl, u16 status)
    aen = list_first_entry(&ctrl->async_events,
    struct nvmet_async_event, entry);
    req = ctrl->async_event_cmds[--ctrl->nr_async_event_cmds];
    - if (status == 0)
    - nvmet_set_result(req, nvmet_async_event_result(aen));
    + nvmet_set_result(req, nvmet_async_event_result(aen));

    list_del(&aen->entry);
    kfree(aen);

    mutex_unlock(&ctrl->lock);
    - nvmet_req_complete(req, status);
    + nvmet_req_complete(req, 0);
    mutex_lock(&ctrl->lock);
    }
    mutex_unlock(&ctrl->lock);
    @@ -169,7 +183,7 @@ static void nvmet_async_event_work(struct work_struct *work)
    struct nvmet_ctrl *ctrl =
    container_of(work, struct nvmet_ctrl, async_event_work);

    - nvmet_async_events_process(ctrl, 0);
    + nvmet_async_events_process(ctrl);
    }

    void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
    @@ -752,7 +766,6 @@ static void nvmet_confirm_sq(struct percpu_ref *ref)

    void nvmet_sq_destroy(struct nvmet_sq *sq)
    {
    - u16 status = NVME_SC_INTERNAL | NVME_SC_DNR;
    struct nvmet_ctrl *ctrl = sq->ctrl;

    /*
    @@ -760,7 +773,7 @@ void nvmet_sq_destroy(struct nvmet_sq *sq)
    * queue doesn't have outstanding requests on it.
    */
    if (ctrl && ctrl->sqs && ctrl->sqs[0] == sq)
    - nvmet_async_events_process(ctrl, status);
    + nvmet_async_events_failall(ctrl);
    percpu_ref_kill_and_confirm(&sq->ref, nvmet_confirm_sq);
    wait_for_completion(&sq->confirm_done);
    wait_for_completion(&sq->free_done);
    --
    2.25.1
    \
     
     \ /
      Last update: 2020-06-30 00:10    [W:4.047 / U:0.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site