From owner-svn-src-stable-12@freebsd.org Tue Jun 9 02:03:31 2020 Return-Path: Delivered-To: svn-src-stable-12@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 6D737343732; Tue, 9 Jun 2020 02:03:31 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 49gth32Jd0z4D9l; Tue, 9 Jun 2020 02:03:31 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3084DE1D9; Tue, 9 Jun 2020 02:03:31 +0000 (UTC) (envelope-from mav@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 05923VE0062164; Tue, 9 Jun 2020 02:03:31 GMT (envelope-from mav@FreeBSD.org) Received: (from mav@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 05923VSK062163; Tue, 9 Jun 2020 02:03:31 GMT (envelope-from mav@FreeBSD.org) Message-Id: <202006090203.05923VSK062163@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: mav set sender to mav@FreeBSD.org using -f From: Alexander Motin Date: Tue, 9 Jun 2020 02:03:31 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r361953 - stable/12/sys/cam/ctl X-SVN-Group: stable-12 X-SVN-Commit-Author: mav X-SVN-Commit-Paths: stable/12/sys/cam/ctl X-SVN-Commit-Revision: 361953 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable-12@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: SVN commit messages for only the 12-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jun 2020 02:03:31 -0000 Author: mav Date: Tue Jun 9 02:03:30 2020 New Revision: 361953 URL: https://svnweb.freebsd.org/changeset/base/361953 Log: MFC r361630: Remove session locking from cfiscsi_pdu_update_cmdsn(). cs_cmdsn can be incremented with single atomic. expcmdsn/maxcmdsn set in cfiscsi_pdu_prepare() based on cs_cmdsn are not required to be updated synchronously, only monotonically, that is achieved with lock there. Modified: stable/12/sys/cam/ctl/ctl_frontend_iscsi.c Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/cam/ctl/ctl_frontend_iscsi.c ============================================================================== --- stable/12/sys/cam/ctl/ctl_frontend_iscsi.c Tue Jun 9 02:01:39 2020 (r361952) +++ stable/12/sys/cam/ctl/ctl_frontend_iscsi.c Tue Jun 9 02:03:30 2020 (r361953) @@ -211,7 +211,7 @@ cfiscsi_pdu_update_cmdsn(const struct icl_pdu *request { const struct iscsi_bhs_scsi_command *bhssc; struct cfiscsi_session *cs; - uint32_t cmdsn, expstatsn; + uint32_t cmdsn, curcmdsn; cs = PDU_SESSION(request); @@ -220,16 +220,20 @@ cfiscsi_pdu_update_cmdsn(const struct icl_pdu *request * The purpose of the timeout is to reset the connection when it stalls; * we don't want this to happen when NOP-In or NOP-Out ends up delayed * in some queue. - * - * XXX: Locking? */ cs->cs_timeout = 0; /* + * Immediate commands carry cmdsn, but it is neither incremented nor + * verified. + */ + if (request->ip_bhs->bhs_opcode & ISCSI_BHS_OPCODE_IMMEDIATE) + return (false); + + /* * Data-Out PDUs don't contain CmdSN. */ - if ((request->ip_bhs->bhs_opcode & ~ISCSI_BHS_OPCODE_IMMEDIATE) == - ISCSI_BHS_OPCODE_SCSI_DATA_OUT) + if (request->ip_bhs->bhs_opcode == ISCSI_BHS_OPCODE_SCSI_DATA_OUT) return (false); /* @@ -237,50 +241,37 @@ cfiscsi_pdu_update_cmdsn(const struct icl_pdu *request * (initiator -> target) PDUs. */ bhssc = (const struct iscsi_bhs_scsi_command *)request->ip_bhs; - cmdsn = ntohl(bhssc->bhssc_cmdsn); - expstatsn = ntohl(bhssc->bhssc_expstatsn); + curcmdsn = cmdsn = ntohl(bhssc->bhssc_cmdsn); - CFISCSI_SESSION_LOCK(cs); -#if 0 - if (expstatsn != cs->cs_statsn) { - CFISCSI_SESSION_DEBUG(cs, "received PDU with ExpStatSN %d, " - "while current StatSN is %d", expstatsn, - cs->cs_statsn); - } -#endif + /* + * Increment session cmdsn and exit if we received the expected value. + */ + do { + if (atomic_fcmpset_32(&cs->cs_cmdsn, &curcmdsn, cmdsn + 1)) + return (false); + } while (curcmdsn == cmdsn); - if ((request->ip_bhs->bhs_opcode & ISCSI_BHS_OPCODE_IMMEDIATE) == 0) { - /* - * The target MUST silently ignore any non-immediate command - * outside of this range. - */ - if (ISCSI_SNLT(cmdsn, cs->cs_cmdsn) || - ISCSI_SNGT(cmdsn, cs->cs_cmdsn - 1 + maxtags)) { - CFISCSI_SESSION_UNLOCK(cs); - CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " - "while expected %u", cmdsn, cs->cs_cmdsn); - return (true); - } - - /* - * We don't support multiple connections now, so any - * discontinuity in CmdSN means lost PDUs. Since we don't - * support PDU retransmission -- terminate the connection. - */ - if (cmdsn != cs->cs_cmdsn) { - CFISCSI_SESSION_UNLOCK(cs); - CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " - "while expected %u; dropping connection", - cmdsn, cs->cs_cmdsn); - cfiscsi_session_terminate(cs); - return (true); - } - cs->cs_cmdsn++; + /* + * The target MUST silently ignore any non-immediate command outside + * of this range. + */ + if (ISCSI_SNLT(cmdsn, curcmdsn) || + ISCSI_SNGT(cmdsn, curcmdsn - 1 + maxtags)) { + CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " + "while expected %u", cmdsn, curcmdsn); + return (true); } - CFISCSI_SESSION_UNLOCK(cs); - - return (false); + /* + * We don't support multiple connections now, so any discontinuity in + * CmdSN means lost PDUs. Since we don't support PDU retransmission -- + * terminate the connection. + */ + CFISCSI_SESSION_WARN(cs, "received PDU with CmdSN %u, " + "while expected %u; dropping connection", + cmdsn, curcmdsn); + cfiscsi_session_terminate(cs); + return (true); } static void @@ -367,6 +358,7 @@ cfiscsi_pdu_prepare(struct icl_pdu *response) struct cfiscsi_session *cs; struct iscsi_bhs_scsi_response *bhssr; bool advance_statsn = true; + uint32_t cmdsn; cs = PDU_SESSION(response); @@ -408,8 +400,9 @@ cfiscsi_pdu_prepare(struct icl_pdu *response) if (bhssr->bhssr_opcode != ISCSI_BHS_OPCODE_SCSI_DATA_IN || (bhssr->bhssr_flags & BHSDI_FLAGS_S)) bhssr->bhssr_statsn = htonl(cs->cs_statsn); - bhssr->bhssr_expcmdsn = htonl(cs->cs_cmdsn); - bhssr->bhssr_maxcmdsn = htonl(cs->cs_cmdsn - 1 + + cmdsn = cs->cs_cmdsn; + bhssr->bhssr_expcmdsn = htonl(cmdsn); + bhssr->bhssr_maxcmdsn = htonl(cmdsn - 1 + imax(0, maxtags - cs->cs_outstanding_ctl_pdus)); if (advance_statsn) @@ -2461,19 +2454,14 @@ cfiscsi_datamove_in(union ctl_io *io) buffer_offset = io->scsiio.kern_rel_offset; /* - * This is the transfer length expected by the initiator. In theory, - * it could be different from the correct amount of data from the SCSI - * point of view, even if that doesn't make any sense. + * This is the transfer length expected by the initiator. It can be + * different from the amount of data from the SCSI point of view. */ expected_len = ntohl(bhssc->bhssc_expected_data_transfer_length); -#if 0 - if (expected_len != io->scsiio.kern_total_len) { - CFISCSI_SESSION_DEBUG(cs, "expected transfer length %zd, " - "actual length %zd", expected_len, - (size_t)io->scsiio.kern_total_len); - } -#endif + /* + * If the transfer is outside of expected length -- we are done. + */ if (buffer_offset >= expected_len) { #if 0 CFISCSI_SESSION_DEBUG(cs, "buffer_offset = %zd, "