From patchwork Sun Aug 7 17:09:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sagi Grimberg X-Patchwork-Id: 9266487 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DD69E6075A for ; Sun, 7 Aug 2016 17:09:26 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CBE5226E8A for ; Sun, 7 Aug 2016 17:09:26 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BB11427D5E; Sun, 7 Aug 2016 17:09:26 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 16BD726E8A for ; Sun, 7 Aug 2016 17:09:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752037AbcHGRJO (ORCPT ); Sun, 7 Aug 2016 13:09:14 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:35830 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751713AbcHGRJM (ORCPT ); Sun, 7 Aug 2016 13:09:12 -0400 Received: by mail-wm0-f54.google.com with SMTP id f65so85937381wmi.0; Sun, 07 Aug 2016 10:09:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=74ACRFQOtRn+JKxSKeXuxMkco+OC8jh+Z+8w8Lo4+SI=; b=fACGvNCOCfiDbwYELTQjLLU0yF48eVxbKJjEAlJ+H7FR0DES/sG+LbzW9vIbb7+qyP v0ZIMemQ5qTjMmByUGvsKrs5cb8ZRYWHDKyy9/Pv/qt9pAlKmLSQY/bx+od6w3lb59Io roLtyyrMM6xRuYlCCgK5K7Dwp30lOi9A4ji4m1fxhrGClNRwk2kXnBAQV1bFugbp6EUE qAiugrgu4gowM2R7Kf3MywGRk6IiihEyC+1fm1YnRruilC++iMntqbmEkYIxVrkt8x24 BwNjLq8oNuc+8dvB8Rbb2F7FgS5sX04/az3W3/XvKIbXYB3Vdifx5DZ6xVRxMxv6qlCp SfNw== X-Gm-Message-State: AEkoouuuZDZBbkUTPORKs/XqdETI9wbKXZa1SswG6NAbu4lc558x3Knbs1kVcFa/0CUTBg== X-Received: by 10.28.14.68 with SMTP id 65mr11771152wmo.68.1470589750461; Sun, 07 Aug 2016 10:09:10 -0700 (PDT) Received: from [192.168.1.204] (bzq-82-81-101-184.red.bezeqint.net. [82.81.101.184]) by smtp.gmail.com with ESMTPSA id q137sm19316094wmd.19.2016.08.07.10.09.08 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 07 Aug 2016 10:09:09 -0700 (PDT) Subject: Re: IB/isert: Return value of iser target transport handlers ignored by iscsi target To: Potnuri Bharat Teja , target-devel@vger.kernel.org, nab@linux-iscsi.org References: <20160804103506.GA29543@t5fpga-b1.asicdesigners.com> Cc: linux-scsi@vger.kernel.org, swise@opengridcomputing.com, varun@chelsio.com, rajur@chelsio.com From: Sagi Grimberg Message-ID: <8f3ad0ed-a17d-571a-7e1a-eb61caa263c9@grimberg.me> Date: Sun, 7 Aug 2016 20:09:08 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160804103506.GA29543@t5fpga-b1.asicdesigners.com> Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP > Hi, Hi Baharat, > In iSER target during iwarp connection tear-down due to ping timeouts, the rdma > queues are set to error state and subsequent queued iscsi session commands posted > shall fail with corresponding errno returned by ib_post_send/recv. > At this stage iser target handlers (Ex: isert_put_datain()) > return the error to iscsci target, but these errors are > not being handled by the iscsi target handlers(Ex: lio_queue_status()). Indeed looks like lio_queue_data_in() assumes that ->iscsit_queue_data_in() cannot fail. This either mean that isert_put_data_in() should take care of the extra put in this case, or the iscsi should correctly handle a queue_full condition (because we should not be hitting this in the normal IO flow). Does this (untested) patch help? --- iscsit_build_rsp_pdu(cmd, conn, true, hdr); @@ -1848,7 +1849,10 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd) isert_dbg("Posting SCSI Response\n"); - return isert_post_response(isert_conn, isert_cmd); + ret = isert_post_response(isert_conn, isert_cmd); + if (ret) + target_put_sess_cmd(&cmd->se_cmd); + return 0; } static void @@ -2128,7 +2132,7 @@ isert_put_datain(struct iscsi_conn *conn, struct iscsi_cmd *cmd) struct isert_conn *isert_conn = conn->context; struct ib_cqe *cqe = NULL; struct ib_send_wr *chain_wr = NULL; - int rc; + int ret; isert_dbg("Cmd: %p RDMA_WRITE data_length: %u\n", isert_cmd, se_cmd->data_length); @@ -2148,34 +2152,41 @@ isert_put_datain(struct iscsi_conn *conn, struct iscsi_cmd *cmd) isert_init_send_wr(isert_conn, isert_cmd, &isert_cmd->tx_desc.send_wr); - rc = isert_post_recv(isert_conn, isert_cmd->rx_desc); - if (rc) { - isert_err("ib_post_recv failed with %d\n", rc); - return rc; + ret = isert_post_recv(isert_conn, isert_cmd->rx_desc); + if (ret) { + isert_err("ib_post_recv failed with %d\n", ret); + goto out; } chain_wr = &isert_cmd->tx_desc.send_wr; } - isert_rdma_rw_ctx_post(isert_cmd, isert_conn, cqe, chain_wr); + ret = isert_rdma_rw_ctx_post(isert_cmd, isert_conn, cqe, chain_wr); isert_dbg("Cmd: %p posted RDMA_WRITE for iSER Data READ\n", isert_cmd); - return 1; +out: + if (ret) + target_put_sess_cmd(&cmd->se_cmd); + return 0; } static int isert_get_dataout(struct iscsi_conn *conn, struct iscsi_cmd *cmd, bool recovery) { struct isert_cmd *isert_cmd = iscsit_priv_cmd(cmd); + int ret; isert_dbg("Cmd: %p RDMA_READ data_length: %u write_data_done: %u\n", isert_cmd, cmd->se_cmd.data_length, cmd->write_data_done); isert_cmd->tx_desc.tx_cqe.done = isert_rdma_read_done; - isert_rdma_rw_ctx_post(isert_cmd, conn->context, + ret = isert_rdma_rw_ctx_post(isert_cmd, conn->context, &isert_cmd->tx_desc.tx_cqe, NULL); isert_dbg("Cmd: %p posted RDMA_READ memory for ISER Data WRITE\n", isert_cmd);+out: + if (ret) + target_put_sess_cmd(&cmd->se_cmd); return 0; } -- > > -> While closing the session in case of ping timeouts, isert_close_connection()-> > isert_wait_conn()->isert_wait4cmds() checks for the queued session commands > and waits infinitely for command completion 'cmd_wait_comp' in target_wait_for_sess_cmds(). > 'cmd_wait_comp' will be never complete as the kref on the session command is > not derefed, due to which target_release_cmd_kref() is not called by kref_put(). > Due to this, the older session is not cleared causing the next login negotiation to fail > as the older session is still active(Older SID exists). Makes sense... > [Query 1] If the return value of ib_post_send/recv() are handled to deref the > corresponding queued session commands, the wait on cmd_wait_comp will be > complete and clears off the session successfully. Is this the rightway to > do it here? I think that given that ->iscsit_queue_data_in() and ->iscsit_queue_status() are never expected to fail we probably should take care of it in isert... Nic, any input on the iscsit side? > > [Query 2] An extra deref is done in case of transport_status CMD_T_TAS > in target_wait_for_sess_cmds(), can similar > implementation be done for transport state CMD_T_FABRIC_STOP? I think that can work also given that target_sess_cmd_list_set_waiting() is indeed set when we are starting teardown. Not sure how this will affect other transports though... -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c index a990c04208c9..ff5dfc0f7c50 100644 --- a/drivers/infiniband/ulp/isert/ib_isert.c +++ b/drivers/infiniband/ulp/isert/ib_isert.c @@ -1810,6 +1810,7 @@ isert_put_response(struct iscsi_conn *conn, struct iscsi_cmd *cmd) struct ib_send_wr *send_wr = &isert_cmd->tx_desc.send_wr; struct iscsi_scsi_rsp *hdr = (struct iscsi_scsi_rsp *) &isert_cmd->tx_desc.iscsi_header; + int ret; isert_create_send_desc(isert_conn, isert_cmd, &isert_cmd->tx_desc);