Re: [PATCH V2 2/2] nvmet: use xarray for ctrl ns storing

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

(+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