From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f41.google.com (mail-wr1-f41.google.com [209.85.221.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5A3CF1A0BF8 for ; Wed, 8 Jan 2025 10:50:54 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736333456; cv=none; b=Mwody32+1eNtfdSX7JFE/bpQzyX9muNEQfeuhmU+PMLqYFzK8mOYgcjsvHzBbcd9E2r0KfwuOJ/Njo5HgZXwYmSBLp6r4axKFsPmeb6U3gsyzeEmSCQGKCenuurdBedZcQr8Amh9lcE1OcCo/sSYH3wKhznwIMzHzT74ArtYlNw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736333456; c=relaxed/simple; bh=CnoF580ZZ4PAK1kkMnr7eHWee3X1iBtMG4hLhvClm7c=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Odxe1jY71G8PN/ONTzpcvbHthhVwDuja5hbU2zx1vt0F3G4iPY6ldhYMRdZHnM3M/MggDYE1XE7v40Gr80BdeEm+s/oNUcHvj0SDrxnEDxBoPhrZW6pk7u0K7y8/fp+HQwhx3G/IsgDrWtv6zSR+GGiZYDuUs6K5wCXnohQLCJc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=grimberg.me; spf=pass smtp.mailfrom=gmail.com; arc=none smtp.client-ip=209.85.221.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=grimberg.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f41.google.com with SMTP id ffacd0b85a97d-388cae9eb9fso8727067f8f.3 for ; Wed, 08 Jan 2025 02:50:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736333453; x=1736938253; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=odzJOE89mvVjuAymTIDl4mqtqkpFi0QvREDYkft5dtY=; b=FVKSoCQt/SFA6r8mWLWTsHW8A0CXKPVmg11ajfbrD28yyuL8B+67iHIMrqdw3YG4xK LAvZNSpPKLHTh82gQEhfg1Pqb3O2xMPU3PxpSWGlfglC9XGgapAnIwmvVs/zsUZIFsih RKTOXXLBM9kukwCZljBj+rE8xc0qcEyqD8IUsibJJJKpkeoL45/Xqo5OFOyKeOmMFyAl WIjXPDfzRIGW0IvwD6Iw+Ed0mr1pxbhkCYsCtUQ66oBGrShcN+UnOyjhmV9V+mOcfc1a T+E5TplhJvsISdEwZ2FE/ut7c2VEXdfv0eaT5f7XooZWF9W8y42pPdAqVgqrUInuwJtZ jLsw== X-Forwarded-Encrypted: i=1; AJvYcCWVqRwsDgcBgCt5MaFvhGiTbhiA006GfLfZA64kPDFoxkD6GyI5MF5ox5kOiCwk+xMEMevSd0WRhEQzaM4=@vger.kernel.org X-Gm-Message-State: AOJu0Ywl0hDwUcT9Grsba7XEuoxciF6+qKZLlL8SXPeS8SZD3guHX9fZ Mzy5+mYSxkIQrHm7tZzVcMMZGskHH1/w8WleUJzrYaQdlGLIOBdz X-Gm-Gg: ASbGnctjAsFAVhOO5N6WmynGVE1sm2TVbvbFPGE4aEqI2t+inH2nz0XJgH0tdYqy+iZ ubMk2/TZ9ZYqePIzyww+owDvy1F5gkgaBWe9Bi5Fy/TqjWlzzfi8kCI2Ccw0Zmat8MjmYfi8QaK qI06T20vs8i1fEBRmMo2kHmYWkdkf7tTGNuwShQFUCe0Tp+rrjfJtmisnmQXOxYD3GR2F8SmKx3 NQYuQ/2aWyNlKWh11fPanZIyKmvC/HlWNsPZCBZpAJ3vA/RPB/csVBmAjg6lGRklhHYExAxZXPU DZAVSXmsfcJoDI6JgFI5EJ4= X-Google-Smtp-Source: AGHT+IGyJIn1+FUTSTjaqFHuvNbDlz2LfdKbLFnG0S/LcdjitsAv0ZLQ/c6wEokgaGCbwAr2f6Fagg== X-Received: by 2002:a5d:5f4b:0:b0:385:ed16:c91 with SMTP id ffacd0b85a97d-38a872e8ab2mr1439214f8f.24.1736333452447; Wed, 08 Jan 2025 02:50:52 -0800 (PST) Received: from [10.50.4.206] (bzq-84-110-32-226.static-ip.bezeqint.net. [84.110.32.226]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-436e2de8edcsm16567425e9.23.2025.01.08.02.50.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 08 Jan 2025 02:50:51 -0800 (PST) Message-ID: Date: Wed, 8 Jan 2025 12:50:50 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 2/3] nvme: trigger reset when keep alive fails To: Daniel Wagner Cc: Daniel Wagner , James Smart , Keith Busch , Christoph Hellwig , Hannes Reinecke , Paul Ely , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org References: <20241129-nvme-fc-handle-com-lost-v3-0-d8967b3cae54@kernel.org> <20241129-nvme-fc-handle-com-lost-v3-2-d8967b3cae54@kernel.org> <693187ac-9fe2-4ba3-8fcf-e34204fe7247@flourine.local> Content-Language: en-US From: Sagi Grimberg In-Reply-To: <693187ac-9fe2-4ba3-8fcf-e34204fe7247@flourine.local> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 07/01/2025 16:38, Daniel Wagner wrote: > On Tue, Dec 24, 2024 at 12:31:35PM +0200, Sagi Grimberg wrote: >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>> index bfd71511c85f8b1a9508c6ea062475ff51bf27fe..2a07c2c540b26c8cbe886711abaf6f0afbe6c4df 100644 >>> --- a/drivers/nvme/host/core.c >>> +++ b/drivers/nvme/host/core.c >>> @@ -1320,6 +1320,12 @@ static enum rq_end_io_ret nvme_keep_alive_end_io(struct request *rq, >>> dev_err(ctrl->device, >>> "failed nvme_keep_alive_end_io error=%d\n", >>> status); >>> + /* >>> + * The driver reports that we lost the connection, >>> + * trigger a recovery. >>> + */ >>> + if (status == BLK_STS_TRANSPORT) >>> + nvme_reset_ctrl(ctrl); >>> return RQ_END_IO_NONE; >>> } >>> >> A lengthy explanation that results in nvme core behavior that assumes a very >> specific driver behavior. > I tried to explain exactly what's going on, so we can discuss possible > solutions without communicating past each other. > > In the meantime I started on a patch set for the TP4129 related changes > in the spec (KATO Corrections and Clarifications). These changes would > also depend on the kato timeout handler triggering a reset. > > I am fine with dropping this change for now and discuss it in the light > of TP4129 if this is what you prefer? > >> Isn't the root of the problem that FC is willing to live >> peacefully with a controller >> without any queues/connectivity to it without periodically reconnecting? > The root problem is that the connect lost event gets ignored in the > CONNECTING state for the first connection attempt. All will work fine > for RECONNECTING state. > > Maybe something like this instead? (untested) > > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c > index c4cbe3ce81f7..1f1d1d62a978 100644 > --- a/drivers/nvme/host/fc.c > +++ b/drivers/nvme/host/fc.c > @@ -148,6 +148,7 @@ struct nvme_fc_rport { > #define ASSOC_ACTIVE 0 > #define ASSOC_FAILED 1 > #define FCCTRL_TERMIO 2 > +#define CONNECTIVITY_LOST 3 > > struct nvme_fc_ctrl { > spinlock_t lock; > @@ -785,6 +786,8 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl) > "NVME-FC{%d}: controller connectivity lost. Awaiting " > "Reconnect", ctrl->cnum); > > + set_bit(CONNECTIVITY_LOST, &ctrl->flags); > + > switch (nvme_ctrl_state(&ctrl->ctrl)) { > case NVME_CTRL_NEW: > case NVME_CTRL_LIVE: > @@ -3071,6 +3074,8 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl) > if (nvme_fc_ctlr_active_on_rport(ctrl)) > return -ENOTUNIQ; > > + clear_bit(CONNECTIVITY_LOST, &ctrl->flags); > + > dev_info(ctrl->ctrl.device, > "NVME-FC{%d}: create association : host wwpn 0x%016llx " > " rport wwpn 0x%016llx: NQN \"%s\"\n", > @@ -3174,6 +3179,11 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl) > > changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE); > > + if (test_bit(CONNECTIVITY_LOST, &ctrl->flags)) { > + ret = -EIO; > + goto out_term_aeo_ops; > + } > + > ctrl->ctrl.nr_reconnects = 0; > > if (changed) This looks a lot better to me.