Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
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 14:49:16 -0700
Message-ID: <20200624214916.GG1291930@dhcp-10-100-145-180.wdl.wdc.com> (raw)
In-Reply-To: <06623bef-7269-f45b-9f43-8c854ddf812d@grimberg.me>

On Wed, Jun 24, 2020 at 12:03:41PM -0700, Sagi Grimberg wrote:
> 
> > > > > If the controller does not support the CNS value, it may return Invalid
> > > > > Field with DNR set. That error currently gets propogated back to
> > > > > nvme_init_ns_head(), which then abandons the namespace. Just as the code
> > > > > coments say, we had been historically been clearing such errors because
> > > > > we have other ways to identify the namespace, but now we're not clearing
> > > > > that error.
> > > > 
> > > > I don't understand what you are saying Keith.
> > > > 
> > > > My comment was for this block:
> > > > -- 
> > > >       if (!status && nvme_multi_css(ctrl) && !csi_seen) {
> > > >           dev_warn(ctrl->device, "Command set not reported for nsid:%d\n",
> > > >                nsid);
> > > >           status = -EINVAL;
> > > >       }
> > > > -- 
> > > > 
> > > > I was saying that !status doesn't necessarily mean success, but it can
> > > > also mean that its an retry-able error status (due to transport or
> > > > controller). If we see a retry-able error we should still clear the
> > > > status so we don't abandon the namespace.
> > > > 
> > > > This for example would achieve that:
> > 
> > We're not talking about the same thing. I am only talking about what
> > introduced the DNR check, from commit 59c7c3caaaf87.
> > 
> > I know you added it because you want to abort comparing identifiers on a
> > rescan when retrieving the identifiers failed. That's fine, but I am
> > saying this fails namespace creation in the first place for some types
> > of devices that used to succeed.
> 
> OK, now I think I understand (thanks for clarifying that the discussion
> is not on patch 3/5, but rather on 59c7c3caaaf87).
> 
> So the original proposal was to check NVME_SC_HOST_PATH_ERROR (and now
> we have NVME_SC_HOST_ABORTED_CMD) but with the review Christoph gave
> it seemed to make more sense that we generalize and check the DNR bit.

Okay, I didn't question this approach when it first went through, so
sorry about this digression, but I really don't get how this DNR check
helps at all.

The code currently returns an error if DNR is set. Based on the commit
messages, it sounds like you need that error to skip comparing
identifiers through nvme's scan_work calling revalidate_disk():
suppressing the error has revalidate_disk() fail with -ENODEV when
comparing identifiers fails.

Since we do return the error when DNR is set, we skip comparing
identifiers and return blk_status_to_errno(nvme_error_status(ret))
instead. How is that errno an improvement?

And then if DNR is not set, we suppress the error and proceed with
comparing identifiers??

  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
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 [this message]
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=20200624214916.GG1291930@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: 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