From nobody Tue Oct 10 22:26:19 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4S4r8q2fZYz4wQMs; Tue, 10 Oct 2023 22:26:19 +0000 (UTC) (envelope-from git@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 "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4S4r8q1VDGz4H1H; Tue, 10 Oct 2023 22:26:19 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1696976779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=zvHn6p2qMnmpwqqzi8vLphxe2u80LBhP8PcyqcIeC3w=; b=UV+P8c4Se7mxt5lUXhgMyerpQsQFam/lsGw0pW5FEbGL01cahNuxM+oa43YfMvtotZhdAY Z2j58OXwrutnOgIBFIOdAFGCBil6f4MpWcX4nmb9N4uxwNynZlGz9CpYvBYYzSWYMhK82b iHTygIHuR4aczmF0vsr6yfbQVjAokmtkZs+ZIBjgMHz/9e5OJWjyU10RWlAcBkugw1gMFz gnQqXXEn/V3scyuA+v69PCsNFriq7xo9ZlTLd8JL5xkDjsl3ctepMIvSi6TERr4JssMptG nuIs5ica2+fOQy9q55SaB2zdBfjQSSajoGBtIo+GC8rfd4MIdoOy5JGL+DJRDA== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1696976779; a=rsa-sha256; cv=none; b=n/FB9UhkbsPBOu3uxISK2UBnfHuhBS5IbdfgrDUPhdZ/DUx0EgVGR7XxxnjXgbd2ce/nNt eXSnU8LxFvgurTb+qtFJ2SHP7gD8MhpivmhSIx6w+AqzId6R+3AbNMHKxLvrUd7Jnn6e/P /ietDVCgWMjot1hPqWFCsITS7IvaA83jn3xbHvETl4Pnja/89fSbeVSC+BSTVsMVitauZS Np7nqoguONcyqxlHvv2+FcDRVK0Mni/Sczp6gMG6ImOU/o8B+R3DDMT0ngrrm+e1h49tLw OKEhwBhpNcq/LqjHGthtf7qKEMG4eTKYePk1yoKLIWauDWNKjs0LKJkri8MC5w== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1696976779; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=zvHn6p2qMnmpwqqzi8vLphxe2u80LBhP8PcyqcIeC3w=; b=LRdjV6pMYOWSzBRZh4bBATKH7YJVG/1Gm6vSkAtWxHwgSMZ30eEZNuz0hIa+iYg7RxHQTn VIPg+M8Q9UYvvwS60d7mMA3UJc7S6q47b4n60FV7tnt+vBo5X7nKL+Fv8pSsjTa6j0brXQ txfODZbSWdcyQdoyEGGLQ+wGQ5zsO704Dle7PnzbVocPf7db9gqCSoq/WaZ60kPPHxXsvC coC0ZSCDLAx2XtCKZQ3E6cpsbrRCJjv+BUslutgKxjfznoUt7S24Ut4703+iOMi2vNp1Tv UgCv6zCWRt8kGG4iSIrHuqqmMAjJ1FqU7ePdUf4wh112zJbIxyH8nc++HkQ6Pw== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (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 did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4S4r8q0YTczwPR; Tue, 10 Oct 2023 22:26:19 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.17.1/8.17.1) with ESMTP id 39AMQJRP085498; Tue, 10 Oct 2023 22:26:19 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 39AMQJfl085495; Tue, 10 Oct 2023 22:26:19 GMT (envelope-from git) Date: Tue, 10 Oct 2023 22:26:19 GMT Message-Id: <202310102226.39AMQJfl085495@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Warner Losh Subject: git: 9cd7b624732c - main - nvme: Eliminate RECOVERY_FAILED state List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: imp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 9cd7b624732c3b675178b02b7447272f67a3203d Auto-Submitted: auto-generated The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=9cd7b624732c3b675178b02b7447272f67a3203d commit 9cd7b624732c3b675178b02b7447272f67a3203d Author: Warner Losh AuthorDate: 2023-10-10 17:13:16 +0000 Commit: Warner Losh CommitDate: 2023-10-10 22:13:57 +0000 nvme: Eliminate RECOVERY_FAILED state While it seemed like a good idea to have this state, we can do everything we wanted with the state by checking ctrlr->is_failed since that's set before we start failing the qpairs. Add some comments about racing when we're failing the controller, though in practice I'm not sure that kind of race could even be lost. Sponsored by: Netflix Reviewed by: chuck, gallatin, jhb Differential Revision: https://reviews.freebsd.org/D42051 --- sys/dev/nvme/nvme_private.h | 1 - sys/dev/nvme/nvme_qpair.c | 43 ++++++++++++++++++++++++++++--------------- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/sys/dev/nvme/nvme_private.h b/sys/dev/nvme/nvme_private.h index c573fbfd572f..f6ad51795adb 100644 --- a/sys/dev/nvme/nvme_private.h +++ b/sys/dev/nvme/nvme_private.h @@ -150,7 +150,6 @@ struct nvme_tracker { enum nvme_recovery { RECOVERY_NONE = 0, /* Normal operations */ RECOVERY_WAITING, /* waiting for the reset to complete */ - RECOVERY_FAILED, /* We have failed, no more I/O */ }; struct nvme_qpair { struct nvme_controller *ctrlr; diff --git a/sys/dev/nvme/nvme_qpair.c b/sys/dev/nvme/nvme_qpair.c index cd0057f444b8..6d9d337e76a5 100644 --- a/sys/dev/nvme/nvme_qpair.c +++ b/sys/dev/nvme/nvme_qpair.c @@ -1027,6 +1027,18 @@ nvme_qpair_timeout(void *arg) mtx_assert(&qpair->recovery, MA_OWNED); + /* + * If the controller is failed, then stop polling. This ensures that any + * failure processing that races with the qpair timeout will fail + * safely. + */ + if (qpair->ctrlr->is_failed) { + nvme_printf(qpair->ctrlr, + "Failed controller, stopping watchdog timeout.\n"); + qpair->timer_armed = false; + return; + } + switch (qpair->recovery_state) { case RECOVERY_NONE: /* @@ -1120,11 +1132,6 @@ nvme_qpair_timeout(void *arg) nvme_printf(ctrlr, "Waiting for reset to complete\n"); idle = false; /* We want to keep polling */ break; - case RECOVERY_FAILED: - KASSERT(qpair->ctrlr->is_failed, - ("Recovery state failed w/o failed controller\n")); - idle = true; /* nothing to monitor */ - break; } /* @@ -1244,11 +1251,21 @@ _nvme_qpair_submit_request(struct nvme_qpair *qpair, struct nvme_request *req) if (tr == NULL || qpair->recovery_state != RECOVERY_NONE) { /* * No tracker is available, or the qpair is disabled due to an - * in-progress controller-level reset or controller failure. If - * we lose the race with recovery_state, then we may add an - * extra request to the queue which will be resubmitted later. - * We only set recovery_state to NONE with qpair->lock also - * held. + * in-progress controller-level reset. If we lose the race with + * recovery_state, then we may add an extra request to the queue + * which will be resubmitted later. We only set recovery_state + * to NONE with qpair->lock also held, so if we observe that the + * state is not NONE, we know it can't transition to NONE below + * when we've submitted the request to hardware. + * + * Also, as part of the failure process, we set recovery_state + * to RECOVERY_WAITING, so we check here to see if we've failed + * the controller. We set it before we call the qpair_fail + * functions, which take out the lock lock before messing with + * queued_req. Since we hold that lock, we know it's safe to + * either fail directly, or queue the failure should is_failed + * be stale. If we lose the race reading is_failed, then + * nvme_qpair_fail will fail the queued request. */ if (qpair->ctrlr->is_failed) { @@ -1314,7 +1331,7 @@ nvme_qpair_enable(struct nvme_qpair *qpair) mtx_assert(&qpair->recovery, MA_OWNED); if (mtx_initialized(&qpair->lock)) mtx_assert(&qpair->lock, MA_OWNED); - KASSERT(qpair->recovery_state != RECOVERY_FAILED, + KASSERT(!qpair->ctrlr->is_failed, ("Enabling a failed qpair\n")); qpair->recovery_state = RECOVERY_NONE; @@ -1471,10 +1488,6 @@ nvme_qpair_fail(struct nvme_qpair *qpair) if (!mtx_initialized(&qpair->lock)) return; - mtx_lock(&qpair->recovery); - qpair->recovery_state = RECOVERY_FAILED; - mtx_unlock(&qpair->recovery); - mtx_lock(&qpair->lock); if (!STAILQ_EMPTY(&qpair->queued_req)) {