[PATCH v2 1/2] block: Introduce alloc_disk_node_attr()

[PATCH v2 1/2] block: Introduce alloc_disk_node_attr()

[Bart Van Assche]

This patch does not change the behavior of any existing code but
introduces a function that will be used by the next patch.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Matias Bjorling <mb@lightnvm.io>
Cc: <stable@vger.kernel.org>
---
 block/genhd.c         | 26 ++++++++++++++++++++++++--
 include/linux/genhd.h | 13 +++++++++++--
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 8cc719a37b32..3b643152907d 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1413,10 +1413,20 @@ dev_t blk_lookup_devt(const char *name, int partno)
 }
 EXPORT_SYMBOL(blk_lookup_devt);
 
-struct gendisk *__alloc_disk_node(int minors, int node_id)
+/**
+ * __alloc_disk_node - allocate a gendisk structure and initialize it
+ * @minors: number of partitions to allocate memory for
+ * @node_id: NUMA node to allocate memory from
+ * @ag: NULL or pointer to a NULL-terminated array with attribute groups that
+ *	must be attached to the disk_to_dev() of the allocated disk structure
+ */
+struct gendisk *__alloc_disk_node(int minors, int node_id,
+				  const struct attribute_group **ag)
 {
 	struct gendisk *disk;
 	struct disk_part_tbl *ptbl;
+	const struct attribute_group **agp;
+	int num_groups = 0;
 
 	if (minors > DISK_MAX_PARTS) {
 		printk(KERN_ERR
@@ -1425,7 +1435,15 @@ struct gendisk *__alloc_disk_node(int minors, int node_id)
 		minors = DISK_MAX_PARTS;
 	}
 
-	disk = kzalloc_node(sizeof(struct gendisk), GFP_KERNEL, node_id);
+	if (ag) {
+		for (agp = ag; *agp; agp++)
+			num_groups++;
+		/* Only count the terminating NULL if the array is not empty. */
+		if (num_groups)
+			num_groups++;
+	}
+	disk = kzalloc_node(struct_size(disk, ag, num_groups), GFP_KERNEL,
+			    node_id);
 	if (disk) {
 		if (!init_part_stats(&disk->part0)) {
 			kfree(disk);
@@ -1461,6 +1479,10 @@ struct gendisk *__alloc_disk_node(int minors, int node_id)
 		rand_initialize_disk(disk);
 		disk_to_dev(disk)->class = &block_class;
 		disk_to_dev(disk)->type = &disk_type;
+		if (num_groups) {
+			memcpy(disk->ag, ag, num_groups * sizeof(ag));
+			disk_to_dev(disk)->groups = disk->ag;
+		}
 		device_initialize(disk_to_dev(disk));
 	}
 	return disk;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 57864422a2c8..50ab6cf77fdf 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -211,6 +211,11 @@ struct gendisk {
 	int node_id;
 	struct badblocks *bb;
 	struct lockdep_map lockdep_map;
+	/*
+	 * Storage space for NULL-terminated attribute group array for
+	 * disk_to_dev() of this gendisk structure.
+	 */
+	const struct attribute_group *ag[0];
 };
 
 static inline struct gendisk *part_to_disk(struct hd_struct *part)
@@ -608,7 +613,8 @@ extern void __delete_partition(struct percpu_ref *);
 extern void delete_partition(struct gendisk *, int);
 extern void printk_all_partitions(void);
 
-extern struct gendisk *__alloc_disk_node(int minors, int node_id);
+extern struct gendisk *__alloc_disk_node(int minors, int node_id,
+					 const struct attribute_group **ag);
 extern struct kobject *get_disk_and_module(struct gendisk *disk);
 extern void put_disk(struct gendisk *disk);
 extern void put_disk_and_module(struct gendisk *disk);
@@ -634,6 +640,9 @@ extern ssize_t part_fail_store(struct device *dev,
 #endif /* CONFIG_FAIL_MAKE_REQUEST */
 
 #define alloc_disk_node(minors, node_id)				\
+	alloc_disk_node_attr(minors, node_id, NULL)
+
+#define alloc_disk_node_attr(minors, node_id, ag)			\
 ({									\
 	static struct lock_class_key __key;				\
 	const char *__name;						\
@@ -641,7 +650,7 @@ extern ssize_t part_fail_store(struct device *dev,
 									\
 	__name = "(gendisk_completion)"#minors"("#node_id")";		\
 									\
-	__disk = __alloc_disk_node(minors, node_id);			\
+	__disk = __alloc_disk_node(minors, node_id, ag);		\
 									\
 	if (__disk)							\
 		lockdep_init_map(&__disk->lockdep_map, __name, &__key, 0); \
-- 
2.18.0

[PATCH v2 2/2] nvme: Fix race conditions related to creation of /dev/nvme0n<x>

[Bart Van Assche]

For the udev rules that create symbolic links under /dev it is
essential that all sysfs attributes are registered before an object
becomes visible. This patch avoids that udevd fails to create the
/dev/disk/by-id/nvme-uuid.* symbolic link.

Fixes: 2b9b6e86bca7 ("NVMe: Export namespace attributes to sysfs")
Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Matias Bjorling <mb@lightnvm.io>
Cc: <stable@vger.kernel.org>
---
 drivers/nvme/host/core.c     | 17 +++++------------
 drivers/nvme/host/lightnvm.c | 34 +++++++---------------------------
 drivers/nvme/host/nvme.h     |  9 ++++-----
 3 files changed, 16 insertions(+), 44 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd8ec1dd9219..0121db66d2c6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3043,6 +3043,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	struct nvme_id_ns *id;
 	char disk_name[DISK_NAME_LEN];
 	int node = dev_to_node(ctrl->dev), flags = GENHD_FL_EXT_DEVT;
+	const struct attribute_group *nvme_attr_groups[3];
 
 	ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
 	if (!ns)
@@ -3080,7 +3081,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 		}
 	}
 
-	disk = alloc_disk_node(0, node);
+	nvme_attr_groups[0] = &nvme_ns_id_attr_group;
+	nvme_attr_groups[1] = nvme_nvm_attr_group(ns);
+	nvme_attr_groups[2] = NULL;
+	disk = alloc_disk_node_attr(0, node, nvme_attr_groups);
 	if (!disk)
 		goto out_unlink_ns;
 
@@ -3100,13 +3104,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	nvme_get_ctrl(ctrl);
 
 	device_add_disk(ctrl->device, ns->disk);
-	if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvme_ns_id_attr_group))
-		pr_warn("%s: failed to create sysfs group for identification\n",
-			ns->disk->disk_name);
-	if (ns->ndev && nvme_nvm_register_sysfs(ns))
-		pr_warn("%s: failed to register lightnvm sysfs group for identification\n",
-			ns->disk->disk_name);
 
 	nvme_mpath_add_disk(ns, id);
 	nvme_fault_inject_init(ns);
@@ -3132,10 +3129,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
 	nvme_fault_inject_fini(ns);
 	if (ns->disk && ns->disk->flags & GENHD_FL_UP) {
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvme_ns_id_attr_group);
-		if (ns->ndev)
-			nvme_nvm_unregister_sysfs(ns);
 		del_gendisk(ns->disk);
 		blk_cleanup_queue(ns->queue);
 		if (blk_get_integrity(ns->disk))
diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
index 6fe5923c95d4..11e687f3c36f 100644
--- a/drivers/nvme/host/lightnvm.c
+++ b/drivers/nvme/host/lightnvm.c
@@ -1270,39 +1270,19 @@ static const struct attribute_group nvm_dev_attr_group_20 = {
 	.attrs		= nvm_dev_attrs_20,
 };
 
-int nvme_nvm_register_sysfs(struct nvme_ns *ns)
+const struct attribute_group *nvme_nvm_attr_group(const struct nvme_ns *ns)
 {
-	struct nvm_dev *ndev = ns->ndev;
-	struct nvm_geo *geo = &ndev->geo;
+	const struct nvm_dev *ndev = ns->ndev;
 
 	if (!ndev)
-		return -EINVAL;
+		return NULL;
 
-	switch (geo->major_ver_id) {
+	switch (ndev->geo->major_ver_id) {
 	case 1:
-		return sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_12);
+		return &nvm_dev_attr_group_12;
 	case 2:
-		return sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_20);
+		return &nvm_dev_attr_group_20;
 	}
 
-	return -EINVAL;
-}
-
-void nvme_nvm_unregister_sysfs(struct nvme_ns *ns)
-{
-	struct nvm_dev *ndev = ns->ndev;
-	struct nvm_geo *geo = &ndev->geo;
-
-	switch (geo->major_ver_id) {
-	case 1:
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_12);
-		break;
-	case 2:
-		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
-					&nvm_dev_attr_group_20);
-		break;
-	}
+	return NULL;
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bb4a2003c097..112596ad1ed3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -551,8 +551,7 @@ static inline void nvme_mpath_stop(struct nvme_ctrl *ctrl)
 void nvme_nvm_update_nvm_info(struct nvme_ns *ns);
 int nvme_nvm_register(struct nvme_ns *ns, char *disk_name, int node);
 void nvme_nvm_unregister(struct nvme_ns *ns);
-int nvme_nvm_register_sysfs(struct nvme_ns *ns);
-void nvme_nvm_unregister_sysfs(struct nvme_ns *ns);
+const struct attribute_group *nvme_nvm_attr_group(const struct nvme_ns *ns);
 int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd, unsigned long arg);
 #else
 static inline void nvme_nvm_update_nvm_info(struct nvme_ns *ns) {};
@@ -563,11 +562,11 @@ static inline int nvme_nvm_register(struct nvme_ns *ns, char *disk_name,
 }
 
 static inline void nvme_nvm_unregister(struct nvme_ns *ns) {};
-static inline int nvme_nvm_register_sysfs(struct nvme_ns *ns)
+static inline const struct attribute_group *
+nvme_nvm_attr_group(const struct nvme_ns *ns)
 {
-	return 0;
+	return NULL;
 }
-static inline void nvme_nvm_unregister_sysfs(struct nvme_ns *ns) {};
 static inline int nvme_nvm_ioctl(struct nvme_ns *ns, unsigned int cmd,
 							unsigned long arg)
 {
-- 
2.18.0

Re: [PATCH v2 1/2] block: Introduce alloc_disk_node_attr()

[Greg Kroah-Hartman]

On Mon, Aug 13, 2018 at 10:25:30AM -0700, Bart Van Assche wrote:
> This patch does not change the behavior of any existing code but
> introduces a function that will be used by the next patch.

"next" is hard to determine in a git log :)

Try being a bit more specific as to why you are doing this.

thanks,

greg k-h

Re: [PATCH v2 2/2] nvme: Fix race conditions related to creation of /dev/nvme0n<x>

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>