Linux-NVME Archive on lore.kernel.org
 help / color / Atom feed
From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To: Christoph Hellwig <hch@lst.de>, Matthew Wilcox <willy@infradead.org>
Cc: "kbusch@kernel.org" <kbusch@kernel.org>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>
Subject: Re: [PATCH V2 2/2] nvmet: use xarray for ctrl ns storing
Date: Wed, 1 Jul 2020 18:36:53 +0000
Message-ID: <BYAPR04MB4965DA55152F9ACE3C3AC93A866C0@BYAPR04MB4965.namprd04.prod.outlook.com> (raw)
In-Reply-To: <20200701060820.GA28214@lst.de>

(+Matthew)

On 6/30/20 11:08 PM, Christoph Hellwig wrote:
> Ok, for the target I can see how this makes sense unlike the host.
Host side is for passthru code which is not merged yet.
> Comments below:
> 
>> -	u64 host_reads = 0, host_writes = 0;
>>   	u64 data_units_read = 0, data_units_written = 0;
>> -	struct nvmet_ns *ns;
>> +	u64 host_reads = 0, host_writes = 0;
>>   	struct nvmet_ctrl *ctrl;
>> +	struct nvmet_ns *ns;
>> +	unsigned long idx;
> 
> Please don't randomly reorder the variable declarations.  Same for
> later function.
Okay since I was touching these functions can we cleanup ?
> 
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index cea7c3834021..d14edaa22ad5 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -10,6 +10,9 @@
>>   #include <linux/pci-p2pdma.h>
>>   #include <linux/scatterlist.h>
>>   
>> +#include "../host/nvme.h"
>> +#undef CREATE_TRACE_POINTS
> 
> We really shoud not include the host nvme.h in the target code.  What
> are you trying to get from it?
> 
Yes we this needs to be removed. I've added nvme_xa_insert() wrapper for 
xa_insert() and didn't post that series. I'd really want us to use 
wrapper to minimize the code and help debugging with pr_debug.

If we do the prototype will go in ../host/nvme.h ? instead of new header
nvme-common.h, please confirm.

>>   static unsigned int nvmet_max_nsid(struct nvmet_subsys *subsys)
>>   {
>> -	struct nvmet_ns *ns;
>> +	struct nvmet_ns *cur;
>> +	unsigned long idx;
>> +	unsigned long nsid;
>>   
>> -	if (list_empty(&subsys->namespaces))
>> +	if (xa_empty(&subsys->namespaces))
>>   		return 0;
>>   
>> -	ns = list_last_entry(&subsys->namespaces, struct nvmet_ns, dev_link);
>> -	return ns->nsid;
>> +	xa_for_each(&subsys->namespaces, idx, cur)
>> +		nsid = cur->nsid;
>> +
>> +	return nsid;
> 
> No need for the xa_empty here.  I also forwarded a question to Matthew
> if there is a better way to find the highest index.
> 

I did look into this and I'm still to add possibly in next V3.
Meanwhile if Matthew has any suggestions that will be great.

I'm not an XArray expert but if it is some flavor of search tree then 
largest index will be on the right most leaf we can either cache it at 
each insertion which will add cost for each insert (not ideal) or just
traverse the tree with an API (ideal).

>>   struct nvmet_ns *nvmet_find_namespace(struct nvmet_ctrl *ctrl, __le32 nsid)
>>   {
>> +	XA_STATE(xas, &ctrl->subsys->namespaces, le32_to_cpu(nsid));
>> +	struct nvmet_ns *ns = NULL;
>>   
>>   	rcu_read_lock();
>> +	do {
>> +		ns = xas_load(&xas);
>> +		if (xa_is_zero(ns))
>> +			ns = NULL;
>> +	} while (xas_retry(&xas, ns));
>>   	if (ns)
>>   		percpu_ref_get(&ns->ref);
>>   	rcu_read_unlock();
> 
> Why doesn't this use xa_load?

I tried to keep load code similar to what we have in the host-core
so that we can add wrapper and not duplicate code which also follows the 
pattern of taking ref count under rcu lock.

> 
>> +	ret = xa_insert(&subsys->namespaces, ns->nsid, ns, GFP_KERNEL);
>> +	if (ret) {
>> +		switch (ret) {
>> +		case -ENOMEM:
>> +			pr_err("xa insert memory allocation\n");
>> +			goto out_unlock;
>> +		case -EBUSY:
>> +			pr_err("xa insert entry already present\n");
>> +			goto out_unlock;
>> +		default:
>> +			goto out_unlock;
>>   		}
>>   	}
>> +
> 
> I don't think we need the switch statement, just return any error
> encountered.
okay.
> 


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

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-01  2:25 [PATCH V2 0/2] nvme: replace linked list with xarray for ns tracking Chaitanya Kulkarni
2020-07-01  2:25 ` [PATCH V2 1/2] nvme-core: use xarray for ctrl " Chaitanya Kulkarni
2020-07-01 13:12   ` Christoph Hellwig
2020-07-01 16:44     ` Keith Busch
2020-07-01 18:19     ` Chaitanya Kulkarni
2020-07-01 18:29       ` Keith Busch
2020-07-01 18:40         ` Chaitanya Kulkarni
2020-07-01  2:25 ` [PATCH V2 2/2] nvmet: use xarray for ctrl ns storing Chaitanya Kulkarni
2020-07-01  6:08   ` Christoph Hellwig
2020-07-01 18:36     ` Chaitanya Kulkarni [this message]
2020-07-01  5:36 ` [PATCH V2 0/2] nvme: replace linked list with xarray for ns tracking Christoph Hellwig
2020-07-01 13:21   ` Keith Busch
2020-07-01 16:49     ` Sagi Grimberg
2020-07-01 18:43       ` Chaitanya Kulkarni
2020-07-01 18:41     ` Chaitanya Kulkarni
2020-07-01 18:41   ` 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=BYAPR04MB4965DA55152F9ACE3C3AC93A866C0@BYAPR04MB4965.namprd04.prod.outlook.com \
    --to=chaitanya.kulkarni@wdc.com \
    --cc=hch@lst.de \
    --cc=kbusch@kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=sagi@grimberg.me \
    --cc=willy@infradead.org \
    /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-NVME Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nvme/0 linux-nvme/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-nvme linux-nvme/ https://lore.kernel.org/linux-nvme \
		linux-nvme@lists.infradead.org
	public-inbox-index linux-nvme

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-nvme


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