[PATCH 01/10] nvmet: zeroout id-ns buffer for invalid nsid

[Christoph Hellwig]

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.