Re: [PATCH 8/9] nvme: track shared namespaces
On Thu, Sep 21, 2017 at 11:16:31AM -0400, Keith Busch wrote: > If there weren't resistence to renaming structs, it would be more > aligned to how the specification calls these if we rename nvme_ns to > nvme_ns_path, and what you're calling nvme_ns_head is should just be > the nvme_ns. Then we'd still need a good name for what's now nvme_ns :) But seriously speaking - I'm a little worried because Linus is really pissed at me for doing major refactoring in block land (by proxy of Jens who gets all the heat). I just don't feel comfortable with a rename at the moment, even if it's the right thing.
Re: [PATCH 8/9] nvme: track shared namespaces
On Thu, Sep 21, 2017 at 04:37:48PM +0200, Christoph Hellwig wrote: > On Thu, Sep 21, 2017 at 07:22:17AM +0200, Johannes Thumshirn wrote: > > > But head also has connotations in the SAN world. Maybe nvme_ns_chain? > > > > I know that's why I didn't really like it all too much in the first place as > > well. For nvme_ns_chain, it's not a chain really (the list itself is a > > chain, > > the structure really is the list head...), but I suck at naming things so. > > Well, it _is_ the structure for the namespace, and that's the fundamental > problem here given that we use that name for something else at the > moment. > > We could hav nvme_namespace and nvme_ns, but I'm not sure that this > helps clarity.. If there weren't resistence to renaming structs, it would be more aligned to how the specification calls these if we rename nvme_ns to nvme_ns_path, and what you're calling nvme_ns_head is should just be the nvme_ns.
Re: [PATCH 8/9] nvme: track shared namespaces
On Thu, Sep 21, 2017 at 07:22:17AM +0200, Johannes Thumshirn wrote: > > But head also has connotations in the SAN world. Maybe nvme_ns_chain? > > I know that's why I didn't really like it all too much in the first place as > well. For nvme_ns_chain, it's not a chain really (the list itself is a chain, > the structure really is the list head...), but I suck at naming things so. Well, it _is_ the structure for the namespace, and that's the fundamental problem here given that we use that name for something else at the moment. We could hav nvme_namespace and nvme_ns, but I'm not sure that this helps clarity..
Re: [PATCH 8/9] nvme: track shared namespaces
On Wed, Sep 20, 2017 at 04:54:36PM +0200, Christoph Hellwig wrote: > On Wed, Sep 20, 2017 at 10:36:43AM +0200, Johannes Thumshirn wrote: > > Being one of the persons who has to backport a lot of NVMe code to older > > kernels I'm not a huge fan of renaming nmve_ns. > > The churn is main main worry. Well and that I don't have a reall good > name for what currently is nvme_ns either :) > > > That said, I don't have a better name for nvme_ns_head (yet) but I'll try to > > think of one as well. OTOH looking at nvme_ns_head it actaully is the list > > head of the nvme_ns list. > > But head also has connotations in the SAN world. Maybe nvme_ns_chain? I know that's why I didn't really like it all too much in the first place as well. For nvme_ns_chain, it's not a chain really (the list itself is a chain, the structure really is the list head...), but I suck at naming things so. -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH 8/9] nvme: track shared namespaces
On Wed, Sep 20, 2017 at 10:36:43AM +0200, Johannes Thumshirn wrote: > Being one of the persons who has to backport a lot of NVMe code to older > kernels I'm not a huge fan of renaming nmve_ns. The churn is main main worry. Well and that I don't have a reall good name for what currently is nvme_ns either :) > That said, I don't have a better name for nvme_ns_head (yet) but I'll try to > think of one as well. OTOH looking at nvme_ns_head it actaully is the list > head of the nvme_ns list. But head also has connotations in the SAN world. Maybe nvme_ns_chain?
Re: [PATCH 8/9] nvme: track shared namespaces
On Mon, Sep 18, 2017 at 04:14:52PM -0700, 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. > > [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. Being one of the persons who has to backport a lot of NVMe code to older kernels I'm not a huge fan of renaming nmve_ns. That said, I don't have a better name for nvme_ns_head (yet) but I'll try to think of one as well. OTOH looking at nvme_ns_head it actaully is the list head of the nvme_ns list. Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
[PATCH 8/9] nvme: track shared namespaces
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.
[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.
Signed-off-by: Christoph Hellwig
---
drivers/nvme/host/core.c | 193 +--
drivers/nvme/host/lightnvm.c | 14 ++--
drivers/nvme/host/nvme.h | 21 -
3 files changed, 193 insertions(+), 35 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9da538d7ca87..3e8405fd57a9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -247,10 +247,28 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
}
EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
+static void nvme_destroy_ns_head(struct kref *ref)
+{
+ struct nvme_ns_head *head =
+ container_of(ref, struct nvme_ns_head, ref);
+
+ list_del_init(&head->entry);
+ cleanup_srcu_struct(&head->srcu);
+ kfree(head);
+}
+
+static void nvme_put_ns_head(struct nvme_ns_head *head)
+{
+ kref_put(&head->ref, nvme_destroy_ns_head);
+}
+
static void nvme_free_ns(struct kref *kref)
{
struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
+ if (ns->head)
+ nvme_put_ns_head(ns->head);
+
if (ns->ndev)
nvme_nvm_unregister(ns);
@@ -420,7 +438,7 @@ static inline void nvme_setup_flush(struct nvme_ns *ns,
{
memset(cmnd, 0, sizeof(*cmnd));
cmnd->common.opcode = nvme_cmd_flush;
- cmnd->common.nsid = cpu_to_le32(ns->ns_id);
+ cmnd->common.nsid = cpu_to_le32(ns->head->ns_id);
}
static blk_status_t nvme_setup_discard(struct nvme_ns *ns, struct request *req,
@@ -451,7 +469,7 @@ static blk_status_t nvme_setup_discard(struct nvme_ns *ns,
struct request *req,
memset(cmnd, 0, sizeof(*cmnd));
cmnd->dsm.opcode = nvme_cmd_dsm;
- cmnd->dsm.nsid = cpu_to_le32(ns->ns_id);
+ cmnd->dsm.nsid = cpu_to_le32(ns->head->ns_id);
cmnd->dsm.nr = cpu_to_le32(segments - 1);
cmnd->dsm.attributes = cpu_to_le32(NVME_DSMGMT_AD);
@@ -490,7 +508,7 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
memset(cmnd, 0, sizeof(*cmnd));
cmnd->rw.opcode = (rq_data_dir(req) ? nvme_cmd_write : nvme_cmd_read);
- cmnd->rw.nsid = cpu_to_le32(ns->ns_id);
+ cmnd->rw.nsid = cpu_to_le32(ns->head->ns_id);
cmnd->rw.slba = cpu_to_le64(nvme_block_nr(ns, blk_rq_pos(req)));
cmnd->rw.length = cpu_to_le16((blk_rq_bytes(req) >> ns->lba_shift) - 1);
@@ -971,7 +989,7 @@ static int nvme_submit_io(struct nvme_ns *ns, struct
nvme_user_io __user *uio)
memset(&c, 0, sizeof(c));
c.rw.opcode = io.opcode;
c.rw.flags = io.flags;
- c.rw.nsid = cpu_to_le32(ns->ns_id);
+ c.rw.nsid = cpu_to_le32(ns->head->ns_id);
c.rw.slba = cpu_to_le64(io.slba);
c.rw.length = cpu_to_le16(io.nblocks);
c.rw.control = cpu_to_le16(io.control);
@@ -1036,7 +1054,7 @@ static int nvme_ioctl(struct block_device *bdev, fmode_t
mode,
switch (cmd) {
case NVME_IOCTL_ID:
force_successful_syscall_return();
- return ns->ns_id;
+ return ns->head->ns_id;
case NVME_IOCTL_ADMIN_CMD:
return nvme_user_cmd(ns->ctrl, NULL, (void __user *)arg);
case NVME_IOCTL_IO_CMD:
@@ -1196,6 +1214,13 @@ static void nvme_report_ns_ids(struct nvme_ctrl *ctrl,
unsigned int nsid,
}
}
+static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
+{
+ return !uuid_is_null(&ids->uuid) ||
+ memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) ||
+ memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
+}
+
static bool nvme_ns_ids_equal(struct nvme_ns_ids *a, struct nvme_ns_ids *b)
{
return uuid_equal(&a->uuid, &b->uuid) &&
@@ -1251,7 +1276,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)
return -ENODEV;
}
- id = nvme_identify_ns(ctrl, ns->ns_id);
+ id = nvme_identify_ns(ctrl, ns->head->ns_id);
if (!id)
return -ENODEV;
@@ -1260,10 +1285,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)
goto out;
}
- nvme_report_ns_id
