Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
From: Sagi Grimberg <sagi@grimberg.me>
To: Keith Busch <kbusch@kernel.org>
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:46:03 -0700
Message-ID: <e59e402b-de74-e670-59d1-a6c51680a31d@grimberg.me> (raw)
In-Reply-To: <20200624172509.GD1291930@dhcp-10-100-145-180.wdl.wdc.com>



On 6/24/20 10:25 AM, Keith Busch wrote:
> 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.

Maybe I mis-undersatnd the comment, but if you see a DNR error, it means
that the controller replied an error and its final, so then you can have
extra checks.

I think the point here is that if we got a non-dnr status, we should not
take any actions on this namespace because we need to retry first
(either because the controller is unavailable or it needs the host to
retry for another reason).

>> 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.

I actually think this approach covers exactly what we are trying to
achieve. But I'll let others comment on this.

  reply index

Thread overview: 38+ 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-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 22:10       ` Keith Busch
2020-06-23 23:17         ` Sagi Grimberg
2020-06-24 17:25           ` Keith Busch
2020-06-24 17:46             ` Sagi Grimberg [this message]
2020-06-24 18:03               ` Keith Busch
2020-06-24 18:28                 ` Sagi Grimberg
2020-06-24 18:33                   ` Sagi Grimberg
2020-06-24 18:40                     ` Keith Busch
2020-06-24 19:03                       ` Sagi Grimberg
2020-06-24 21:49                         ` Keith Busch
2020-06-24 22:54                           ` Sagi Grimberg
2020-06-24 23:54                             ` Keith Busch
2020-06-23 23:20       ` Sagi Grimberg
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-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-24  9:11   ` Javier González
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=e59e402b-de74-e670-59d1-a6c51680a31d@grimberg.me \
    --to=sagi@grimberg.me \
    --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=kbusch@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=martin.petersen@oracle.com \
    /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: link

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git