From: Keith Busch <kbusch@kernel.org> To: Sagi Grimberg <sagi@grimberg.me> Cc: "Niklas Cassel" <Niklas.Cassel@wdc.com>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "hch@lst.de" <hch@lst.de>, "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "axboe@kernel.dk" <axboe@kernel.dk>, "Javier González" <javier.gonz@samsung.com>, "Martin K . Petersen" <martin.petersen@oracle.com>, "Johannes Thumshirn" <Johannes.Thumshirn@wdc.com>, "Matias Bjorling" <Matias.Bjorling@wdc.com>, "Daniel Wagner" <dwagner@suse.de> Subject: Re: [PATCHv3 3/5] nvme: implement I/O Command Sets Command Set support Date: Wed, 24 Jun 2020 10:25:09 -0700 [thread overview] Message-ID: <20200624172509.GD1291930@dhcp-10-100-145-180.wdl.wdc.com> (raw) In-Reply-To: <b5b7f6bc-22d4-0601-1749-2a631fb7d055@grimberg.me> On Tue, Jun 23, 2020 at 04:17:30PM -0700, Sagi Grimberg wrote: > > > > > > - len = nvme_process_ns_desc(ctrl, ids, cur); > > > > > + len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen); > > > > > if (len < 0) > > > > > goto free_data; > > > > > len += sizeof(*cur); > > > > > } > > > > > free_data: > > > > > + if (!status && nvme_multi_css(ctrl) && !csi_seen) { > > > > > > > > We will clear the status if we detect a path error, that is to > > > > avoid needlessly removing the ns for path failures, so you should > > > > check at the goto site. > > > > > > The problem is that this check has to be done after checking all the ns descs, > > > so this check to be done as the final thing, at least after processing all the > > > ns descs. No matter if nvme_process_ns_desc() returned an error, or if > > > simply NVME_NIDT_CSI wasn't part of the ns desc list, so the loop reached the > > > end without error. > > > > > > Even if the nvme command failed and the status was cleared: > > > > > > if (status > 0 && !(status & NVME_SC_DNR)) > > > status = 0; > > > > This check is so weird. What has DNR got to do with whether or not we > > want to continue with this namespace? The commit that adds this says > > it's to check for a host failed IO, but a controller can just as validly > > set DNR in its error status, in which case we'd still want clear the > > status. > > The reason is that if this error is not a DNR error, it means that we > should retry and succeed, we don't want to remove the namespace. And what if it is a DNR error? For example, the controller simply doesn't support this CNS value. While the controller should support it, we definitely don't need it for NVM command set namespaces. > The problem that this solves is the fact that we have various error > recovery conditions (interconnect issues, failures, resets) and the > async works are designed to continue to run, issue I/O and fail. The > scan work, will revalidate namespaces and remove them if it fails to > do so, which is inherently wrong if these are simply inaccessible by > the host at this time. Relying on a specific bit in the status code seems a bit indirect for this kind of condition.
WARNING: multiple messages have this Message-ID (diff)
From: Keith Busch <kbusch@kernel.org> To: Sagi Grimberg <sagi@grimberg.me> Cc: "axboe@kernel.dk" <axboe@kernel.dk>, "Niklas Cassel" <Niklas.Cassel@wdc.com>, "Daniel Wagner" <dwagner@suse.de>, "Martin K . Petersen" <martin.petersen@oracle.com>, "Johannes Thumshirn" <Johannes.Thumshirn@wdc.com>, "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>, "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "Javier González" <javier.gonz@samsung.com>, "hch@lst.de" <hch@lst.de>, "Matias Bjorling" <Matias.Bjorling@wdc.com> Subject: Re: [PATCHv3 3/5] nvme: implement I/O Command Sets Command Set support Date: Wed, 24 Jun 2020 10:25:09 -0700 [thread overview] Message-ID: <20200624172509.GD1291930@dhcp-10-100-145-180.wdl.wdc.com> (raw) In-Reply-To: <b5b7f6bc-22d4-0601-1749-2a631fb7d055@grimberg.me> On Tue, Jun 23, 2020 at 04:17:30PM -0700, Sagi Grimberg wrote: > > > > > > - len = nvme_process_ns_desc(ctrl, ids, cur); > > > > > + len = nvme_process_ns_desc(ctrl, ids, cur, &csi_seen); > > > > > if (len < 0) > > > > > goto free_data; > > > > > len += sizeof(*cur); > > > > > } > > > > > free_data: > > > > > + if (!status && nvme_multi_css(ctrl) && !csi_seen) { > > > > > > > > We will clear the status if we detect a path error, that is to > > > > avoid needlessly removing the ns for path failures, so you should > > > > check at the goto site. > > > > > > The problem is that this check has to be done after checking all the ns descs, > > > so this check to be done as the final thing, at least after processing all the > > > ns descs. No matter if nvme_process_ns_desc() returned an error, or if > > > simply NVME_NIDT_CSI wasn't part of the ns desc list, so the loop reached the > > > end without error. > > > > > > Even if the nvme command failed and the status was cleared: > > > > > > if (status > 0 && !(status & NVME_SC_DNR)) > > > status = 0; > > > > This check is so weird. What has DNR got to do with whether or not we > > want to continue with this namespace? The commit that adds this says > > it's to check for a host failed IO, but a controller can just as validly > > set DNR in its error status, in which case we'd still want clear the > > status. > > The reason is that if this error is not a DNR error, it means that we > should retry and succeed, we don't want to remove the namespace. And what if it is a DNR error? For example, the controller simply doesn't support this CNS value. While the controller should support it, we definitely don't need it for NVM command set namespaces. > The problem that this solves is the fact that we have various error > recovery conditions (interconnect issues, failures, resets) and the > async works are designed to continue to run, issue I/O and fail. The > scan work, will revalidate namespaces and remove them if it fails to > do so, which is inherently wrong if these are simply inaccessible by > the host at this time. Relying on a specific bit in the status code seems a bit indirect for this kind of condition. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-06-24 17:25 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-22 16:25 [PATCHv3 0/5] nvme support for zoned namespace command set Keith Busch 2020-06-22 16:25 ` [PATCHv3 1/5] block: add capacity field to zone descriptors Keith Busch 2020-06-23 6:15 ` Hannes Reinecke 2020-06-23 8:44 ` Sagi Grimberg 2020-06-26 12:17 ` Jens Axboe 2020-06-26 12:17 ` Jens Axboe 2020-06-22 16:25 ` [PATCHv3 2/5] null_blk: introduce zone capacity for zoned device Keith Busch 2020-06-23 6:16 ` Hannes Reinecke 2020-06-23 8:45 ` Sagi Grimberg 2020-06-22 16:25 ` [PATCHv3 3/5] nvme: implement I/O Command Sets Command Set support Keith Busch 2020-06-23 6:20 ` Hannes Reinecke 2020-06-23 9:20 ` Niklas Cassel 2020-06-23 14:25 ` Keith Busch 2020-06-23 8:53 ` Sagi Grimberg 2020-06-23 11:25 ` Niklas Cassel 2020-06-23 14:59 ` Keith Busch 2020-06-23 14:59 ` Keith Busch 2020-06-23 22:10 ` Keith Busch 2020-06-23 22:10 ` Keith Busch 2020-06-23 23:17 ` Sagi Grimberg 2020-06-23 23:17 ` Sagi Grimberg 2020-06-24 17:25 ` Keith Busch [this message] 2020-06-24 17:25 ` Keith Busch 2020-06-24 17:46 ` Sagi Grimberg 2020-06-24 17:46 ` Sagi Grimberg 2020-06-24 18:03 ` Keith Busch 2020-06-24 18:03 ` Keith Busch 2020-06-24 18:28 ` Sagi Grimberg 2020-06-24 18:28 ` Sagi Grimberg 2020-06-24 18:33 ` Sagi Grimberg 2020-06-24 18:33 ` Sagi Grimberg 2020-06-24 18:40 ` Keith Busch 2020-06-24 18:40 ` Keith Busch 2020-06-24 19:03 ` Sagi Grimberg 2020-06-24 19:03 ` Sagi Grimberg 2020-06-24 21:49 ` Keith Busch 2020-06-24 21:49 ` Keith Busch 2020-06-24 22:54 ` Sagi Grimberg 2020-06-24 22:54 ` Sagi Grimberg 2020-06-24 23:54 ` Keith Busch 2020-06-24 23:54 ` Keith Busch 2020-06-23 23:20 ` Sagi Grimberg 2020-06-23 23:20 ` Sagi Grimberg 2020-06-26 8:54 ` Christoph Hellwig 2020-06-26 8:54 ` Christoph Hellwig 2020-06-22 16:25 ` [PATCHv3 4/5] nvme: support for multi-command set effects Keith Busch 2020-06-23 6:21 ` Hannes Reinecke 2020-06-23 17:43 ` Sagi Grimberg 2020-06-23 17:43 ` Sagi Grimberg 2020-06-22 16:25 ` [PATCHv3 5/5] nvme: support for zoned namespaces Keith Busch 2020-06-22 16:48 ` Johannes Thumshirn 2020-06-23 6:23 ` Hannes Reinecke 2020-06-23 17:45 ` Sagi Grimberg 2020-06-23 17:45 ` Sagi Grimberg 2020-06-24 9:11 ` Javier González 2020-06-24 9:11 ` Javier González 2020-06-29 13:53 ` Johannes Thumshirn 2020-06-29 13:53 ` Johannes Thumshirn
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200624172509.GD1291930@dhcp-10-100-145-180.wdl.wdc.com \ --to=kbusch@kernel.org \ --cc=Johannes.Thumshirn@wdc.com \ --cc=Matias.Bjorling@wdc.com \ --cc=Niklas.Cassel@wdc.com \ --cc=axboe@kernel.dk \ --cc=dwagner@suse.de \ --cc=hch@lst.de \ --cc=javier.gonz@samsung.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=martin.petersen@oracle.com \ --cc=sagi@grimberg.me \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.