Re: [PATCH 17/20] nvme: query namespae identifiers before adding the namespace

From: Niklas Cassel <Niklas.Cassel@wdc.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
	Damien Le Moal <Damien.LeMoal@wdc.com>,
	Sagi Grimberg <sagi@grimberg.me>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Subject: Re: [PATCH 17/20] nvme: query namespae identifiers before adding the namespace
Date: Thu, 1 Oct 2020 17:43:23 +0000	[thread overview]
Message-ID: <20201001174321.GA343057@localhost.localdomain> (raw)
In-Reply-To: <20201001171457.GA29475@lst.de>

On Thu, Oct 01, 2020 at 07:14:57PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 30, 2020 at 10:04:54AM +0000, Niklas Cassel wrote:
> > > For the subject of this patch:
> > > s/namespae/namespace
> 
> Fixed and force pushed.
> 
> > > nvme: remove the namespace identifier verification in __nvme_validate_ns
> > > 
> > > None of the identifiers (including the new CSI) can cange over the life
> > > time of a namespace, so don't bother with the extra query here.
> > > "
> > > 
> > > which now seems to have been squashed with this patch.
> > > 
> > > Squashing it is fine, but perhaps you could add that information in this
> > > commit message?
> > 
> > Hm..
> > 
> > I now see that nvme_validate_or_alloc_ns() in v2 of this series does a
> > nvme_identify_ns_descs(), while nvme_validate_or_alloc_ns() in v1 doesnt.
> > 
> > I don't see a change log, but considering this, my suggestion that you
> > the above sentence from the squashed v1 commit no longer makes sense.
> > 
> > What was the reason for this change?
> 
> Where does this this still show up in a change log?

The sentence only exists in the V1 patch series,
not in the V2 patch series. So that is all good.

My questions is rather:

In V1 nvme_validate_or_alloc_ns():
http://git.infradead.org/users/hch/misc.git/blob/refs/heads/nvme-scanning-cleanup:/drivers/nvme/host/core.c#l3975

You will only call nvme_identify_ns_descs() once, and once only per NSID,
in the nvme_alloc_ns() path.
Subsequent calls that goes through the nvme_validate_ns() path will not
call nvme_identify_ns_descs().

In V2 nvme_validate_or_alloc_ns():
http://git.infradead.org/users/hch/misc.git/blob/refs/heads/nvme-scanning-cleanup.2:/drivers/nvme/host/core.c#l3971

You will call nvme_identify_ns_descs() both for the initial nvme_alloc_ns()
path, but then also in each subsequent call, which goes through the
nvme_validate_ns() path.

V1 seems more optimized.
Since we know that none of the identifiers can change during the lifetime
of a namespace, why not do like V1, and only call it in the nvme_alloc_ns()
path?

Something like this:

static void nvme_validate_or_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
{
	...

	ns = nvme_find_get_ns(ctrl, nsid);
	if (ns) {
		nvme_validate_ns(ns, &ids);
		nvme_put_ns(ns);
		return;
	}

	/* only fetch the ns id desc list if we didn't find a ns */
	if (nvme_identify_ns_descs(ctrl, nsid, &ids))
		return;

	switch (ids.csi) {
	...
	}
	...
}

Is there something I'm missing, or why did you change this between V1 and V2?

Kind regards,
Niklas
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme