From: Christoph Hellwig <hch@lst.de>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>,
"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
"sagi@grimberg.me" <sagi@grimberg.me>
Subject: Re: [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid
Date: Tue, 2 Feb 2021 10:38:31 +0100 [thread overview]
Message-ID: <20210202093831.GG17771@lst.de> (raw)
In-Reply-To: <BYAPR04MB49653AC1A94F78112729EC6686B59@BYAPR04MB4965.namprd04.prod.outlook.com>
On Tue, Feb 02, 2021 at 12:12:33AM +0000, Chaitanya Kulkarni wrote:
> >> + req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> >> + if (!req->ns) {
> >> + status = NVME_SC_INVALID_NS;
> >> + /*
> >> + * According to spec : If the specified namespace is
> >> + * an unallocated NSID then the controller returns a zero filled
> >> + * data structure. Also don't override the error status as invalid
> >> + * namespace takes priority over the failed zeroout buffer case.
> >> + */
> >> + nvmet_zero_sgl(req, 0, sizeof(*id));
> >> + goto out;
> >> + }
> >> +
> >> id = kzalloc(sizeof(*id), GFP_KERNEL);
> >> if (!id) {
> >> status = NVME_SC_INTERNAL;
> >> goto out;
> >> }
> >>
> >> - /* return an all zeroed buffer if we can't find an active namespace */
> >> - req->ns = nvmet_find_namespace(ctrl, req->cmd->identify.nsid);
> >> - if (!req->ns) {
> >> - status = NVME_SC_INVALID_NS;
> >> - goto done;
> > I think all we need to do is to remove the status assignment here.
> > While you're patch avoids the memory allocation for this case, it isn't
> > really the fast path so I'd rather avoid the extra code.
> >
> Sorry, I didn't understand this comment. If we remove the status assignment
> then host will not get the right error. That is the bug we fixed it
> initially
> with bffcd507780e.
Actually, looking at it, bffcd507780e is wrong. Identify Namespace
to a namespace that is not active should return a status of 0 and
a zeroed structure.
> Regarding fast path, if system is under pressure even single memory
> allocation
> can be costly, especially when host tries to do read id-ns, is there any
> reason why we should not consider this scenario ?
No sensible host gets there. I'd rather keep the code simple.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2021-02-02 9:38 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-01 5:41 [PATCH 00/10] nvmet: fixes and some cleanups Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid Chaitanya Kulkarni
2021-02-01 17:24 ` Christoph Hellwig
2021-02-02 0:12 ` Chaitanya Kulkarni
2021-02-02 9:38 ` Christoph Hellwig [this message]
2021-02-03 4:31 ` Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 02/10] nvmet: return uniform error for invalid ns Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 03/10] nvmet: set error in nvmet_find_namespace() Chaitanya Kulkarni
2021-02-01 17:27 ` Christoph Hellwig
2021-02-02 0:13 ` Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 04/10] nvmet: remove nsid param from nvmet_find_namespace() Chaitanya Kulkarni
2021-02-01 17:28 ` Christoph Hellwig
2021-02-02 0:13 ` Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 05/10] nvmet: remove extra variable in is-ns handler Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 06/10] nvmet: add helper to report invalid opcode Chaitanya Kulkarni
2021-02-01 17:29 ` Christoph Hellwig
2021-02-02 0:14 ` Chaitanya Kulkarni
2021-02-02 9:39 ` Christoph Hellwig
2021-02-01 5:41 ` [PATCH 07/10] nvmet: use invalid cmd opcode helper Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 08/10] " Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 09/10] nvmet: use min of device_path and disk len Chaitanya Kulkarni
2021-02-01 5:41 ` [PATCH 10/10] nvme-loop: rename variable to get rid of the warn Chaitanya Kulkarni
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=20210202093831.GG17771@lst.de \
--to=hch@lst.de \
--cc=Chaitanya.Kulkarni@wdc.com \
--cc=linux-nvme@lists.infradead.org \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox