Re: [PATCH 07/10] nvme: track shared namespaces

[J Freyensee <james_p_freyensee@linux.intel.com>]

On Mon, 2017-08-28 at 09:51 +0300, Sagi Grimberg wrote:
> 
> On 23/08/17 20:58, Christoph Hellwig wrote:
> > Introduce a new struct nvme_ns_head [1] that holds information about
> > an actual namespace, unlike struct nvme_ns, which only holds the
> > per-controller namespace information.  For private namespaces there
> > is a 1:1 relation of the two, but for shared namespaces this lets us
> > discover all the paths to it.  For now only the identifiers are moved
> > to the new structure, but most of the information in struct nvme_ns
> > should eventually move over.
> > 
> > To allow lockless path lookup the list of nvme_ns structures per
> > nvme_ns_head is protected by SRCU, which requires freeing the nvme_ns
> > structure through call_srcu.
> 
> I haven't read the later patches yet, but what requires sleep in the
> path selection?
> 
> > 
> > [1] comments welcome if you have a better name for it, the current one
> > is
> >      horrible.  One idea would be to rename the current struct nvme_ns
> >      to struct nvme_ns_link or similar and use the nvme_ns name for the
> >      new structure.  But that would involve a lot of churn.
> 
> maybe nvme_ns_primary?

Since it looks like it holds all unique identifier values and should hold
other namespace characteristics later, maybe:

nvme_ns_item?
Or nvme_ns_entry?
Or nvme_ns_element?
Or nvme_ns_unit?
Or nvme_ns_entity?
Or nvme_ns_container?

> > +/*
> > + * Anchor structure for namespaces.  There is one for each namespace
> > in a
> > + * NVMe subsystem that any of our controllers can see, and the
> > namespace
> > + * structure for each controller is chained of it.  For private
> > namespaces
> > + * there is a 1:1 relation to our namespace structures, that is ->list
> > + * only ever has a single entry for private namespaces.
> > + */
> > +struct nvme_ns_head {
> > +	struct list_head	list;
> 
> Maybe siblings is a better name than list,
> and the nvme_ns list_head should be called
> sibling_entry (or just sibling)?


I think that sounds good too.


> 
> > +	struct srcu_struct      srcu;
> > +	unsigned		ns_id;
> > +	u8			eui64[8];
> > +	u8			nguid[16];
> > +	uuid_t			uuid;
> > +	struct list_head	entry;
> > +	struct kref		ref;
> > +};
> > +
> >   struct nvme_ns {
> >   	struct list_head list;
> >   
> >   	struct nvme_ctrl *ctrl;
> >   	struct request_queue *queue;
> >   	struct gendisk *disk;
> > +	struct list_head siblings;
> >   	struct nvm_dev *ndev;
> >   	struct kref kref;
> > +	struct nvme_ns_head *head;
> >   	int instance;
> >   
> > -	u8 eui[8];
> > -	u8 nguid[16];
> > -	uuid_t uuid;
> > -
> > -	unsigned ns_id;
> >   	int lba_shift;
> >   	u16 ms;
> >   	u16 sgs;
> > 
> 
>