[PATCH 0/5] genhd: implement device_add_disk_with_groups()

[PATCH 0/5] genhd: implement device_add_disk_with_groups()

[Hannes Reinecke]

When creating a block device some drivers need to create additional sysfs
groups to store driver-specific informations.
With the current workflow of adding these groups with a separate call to
sysfs after the device has been created we are introducing a race with udev,
as the uevent is generated before the sysfs attributes are created, and udev
fails to read the required information.

This patchset adds a new function 'device_add_disk_with_groups()' and converts
the obvious candidates to use this new function.

As usual, comments and reviews are welcome.

Hannes Reinecke (5):
  genhd: drop 'bool' argument from __device_add_disk()
  block: genhd: add device_add_disk_with_groups
  nvme: register ns_id attributes as default sysfs groups
  aoe: use device_add_disk_with_groups()
  zram: use device_add_disk_with_groups()

 block/genhd.c                 | 38 ++++++++++++++++++++++++++++++--------
 drivers/block/aoe/aoe.h       |  1 -
 drivers/block/aoe/aoeblk.c    | 21 +++++++--------------
 drivers/block/aoe/aoedev.c    |  1 -
 drivers/block/zram/zram_drv.c | 28 +++++++---------------------
 drivers/nvme/host/core.c      | 14 +++++++-------
 drivers/nvme/host/multipath.c | 12 +++---------
 drivers/nvme/host/nvme.h      |  2 +-
 include/linux/genhd.h         |  3 +++
 9 files changed, 58 insertions(+), 62 deletions(-)

-- 
2.12.3

[PATCH 1/5] genhd: drop 'bool' argument from __device_add_disk()

[Hannes Reinecke]

Split off the last part of __device_add_disk() into __device_get_disk().
With that we can drop the 'bool' argument and streamline the function.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 block/genhd.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index f1543a45e73b..cfa7f4f78435 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -647,15 +647,13 @@ static void register_disk(struct device *parent, struct gendisk *disk)
  * __device_add_disk - add disk information to kernel list
  * @parent: parent device for the disk
  * @disk: per-device partitioning information
- * @register_queue: register the queue if set to true
  *
  * This function registers the partitioning information in @disk
  * with the kernel.
  *
  * FIXME: error handling
  */
-static void __device_add_disk(struct device *parent, struct gendisk *disk,
-			      bool register_queue)
+static void __device_add_disk(struct device *parent, struct gendisk *disk)
 {
 	dev_t devt;
 	int retval;
@@ -699,9 +697,10 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 				    exact_match, exact_lock, disk);
 	}
 	register_disk(parent, disk);
-	if (register_queue)
-		blk_register_queue(disk);
+}
 
+void __device_get_disk(struct gendisk *disk)
+{
 	/*
 	 * Take an extra ref on queue which will be put on disk_release()
 	 * so that it sticks around as long as @disk is there.
@@ -714,13 +713,18 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 void device_add_disk(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk, true);
+	__device_add_disk(parent, disk);
+
+	blk_register_queue(disk);
+
+	__device_get_disk(disk);
 }
 EXPORT_SYMBOL(device_add_disk);
 
 void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk, false);
+	__device_add_disk(parent, disk);
+	__device_get_disk(disk);
 }
 EXPORT_SYMBOL(device_add_disk_no_queue_reg);
 
-- 
2.12.3

Re: [PATCH 1/5] genhd: drop 'bool' argument from __device_add_disk()

[Christoph Hellwig]

> +void __device_get_disk(struct gendisk *disk)

This function can be static. Also it is a bit misnamed.  I'm not
really sure this split is worth it, but if we really want to kill
off the argument it might be worth to just open code the new
__device_get_disk in both callers.

[PATCH 2/5] block: genhd: add device_add_disk_with_groups

[Hannes Reinecke]

Update __device_add_disk() to take an 'groups' argument so that
individual drivers can register a device with additional sysfs
attributes.
This avoids race condition the driver would otherwise have if these
groups need to be created with sysfs_add_groups().

Signed-off-by: Martin Wilck <martin.wilck@suse.com>
Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 block/genhd.c         | 28 +++++++++++++++++++++++-----
 include/linux/genhd.h |  3 +++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index cfa7f4f78435..fbe27cb2c9d7 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -567,7 +567,8 @@ static int exact_lock(dev_t devt, void *data)
 	return 0;
 }
 
-static void register_disk(struct device *parent, struct gendisk *disk)
+static void register_disk(struct device *parent, struct gendisk *disk,
+			  const struct attribute_group **groups)
 {
 	struct device *ddev = disk_to_dev(disk);
 	struct block_device *bdev;
@@ -582,6 +583,10 @@ static void register_disk(struct device *parent, struct gendisk *disk)
 	/* delay uevents, until we scanned partition table */
 	dev_set_uevent_suppress(ddev, 1);
 
+	if (groups) {
+		WARN_ON(ddev->groups);
+		ddev->groups = groups;
+	}
 	if (device_add(ddev))
 		return;
 	if (!sysfs_deprecated) {
@@ -647,13 +652,15 @@ static void register_disk(struct device *parent, struct gendisk *disk)
  * __device_add_disk - add disk information to kernel list
  * @parent: parent device for the disk
  * @disk: per-device partitioning information
+ * @groups: Additional per-device sysfs groups
  *
  * This function registers the partitioning information in @disk
  * with the kernel.
  *
  * FIXME: error handling
  */
-static void __device_add_disk(struct device *parent, struct gendisk *disk)
+static void __device_add_disk(struct device *parent, struct gendisk *disk,
+			      const struct attribute_group **groups)
 {
 	dev_t devt;
 	int retval;
@@ -696,7 +703,7 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk)
 		blk_register_region(disk_devt(disk), disk->minors, NULL,
 				    exact_match, exact_lock, disk);
 	}
-	register_disk(parent, disk);
+	register_disk(parent, disk, groups);
 }
 
 void __device_get_disk(struct gendisk *disk)
@@ -711,9 +718,20 @@ void __device_get_disk(struct gendisk *disk)
 	blk_integrity_add(disk);
 }
 
+void device_add_disk_with_groups(struct device *parent, struct gendisk *disk,
+				 const struct attribute_group **groups)
+{
+	__device_add_disk(parent, disk, groups);
+
+	blk_register_queue(disk);
+
+	__device_get_disk(disk);
+}
+EXPORT_SYMBOL(device_add_disk_with_groups);
+
 void device_add_disk(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk);
+	__device_add_disk(parent, disk, NULL);
 
 	blk_register_queue(disk);
 
@@ -723,7 +741,7 @@ EXPORT_SYMBOL(device_add_disk);
 
 void device_add_disk_no_queue_reg(struct device *parent, struct gendisk *disk)
 {
-	__device_add_disk(parent, disk);
+	__device_add_disk(parent, disk, NULL);
 	__device_get_disk(disk);
 }
 EXPORT_SYMBOL(device_add_disk_no_queue_reg);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 6cb8a5789668..f41152979296 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -394,6 +394,9 @@ extern void part_round_stats(struct request_queue *q, int cpu, struct hd_struct
 
 /* block/genhd.c */
 extern void device_add_disk(struct device *parent, struct gendisk *disk);
+extern void device_add_disk_with_groups(struct device *parent,
+					struct gendisk *disk,
+					const struct attribute_group **groups);
 static inline void add_disk(struct gendisk *disk)
 {
 	device_add_disk(NULL, disk);
-- 
2.12.3

Re: [PATCH 2/5] block: genhd: add device_add_disk_with_groups

[Christoph Hellwig]

On Wed, Jul 25, 2018 at 08:28:37AM +0200, Hannes Reinecke wrote:
> Update __device_add_disk() to take an 'groups' argument so that
> individual drivers can register a device with additional sysfs
> attributes.
> This avoids race condition the driver would otherwise have if these
> groups need to be created with sysfs_add_groups().
> 
> Signed-off-by: Martin Wilck <martin.wilck@suse.com>
> Signed-off-by: Hannes Reinecke <hare@suse.com>

This should be From: Martin, shouldn't it?

My feedback last time was that I'd rather see the new groups argument
to device_add_disk instead of introducing yet another add_disk variant.

I still think that would be the better way to go.

Re: [PATCH 2/5] block: genhd: add device_add_disk_with_groups

[Martin Wilck]

On Wed, 2018-07-25 at 09:46 +0200, Christoph Hellwig wrote:
> On Wed, Jul 25, 2018 at 08:28:37AM +0200, Hannes Reinecke wrote:
> > Update __device_add_disk() to take an 'groups' argument so that
> > individual drivers can register a device with additional sysfs
> > attributes.
> > This avoids race condition the driver would otherwise have if these
> > groups need to be created with sysfs_add_groups().
> > 
> > Signed-off-by: Martin Wilck <martin.wilck@suse.com>
> > Signed-off-by: Hannes Reinecke <hare@suse.com>
> 
> This should be From: Martin, shouldn't it?

I'm fine with From: Hannes here. I'll be mostly offline in the next few
weeks.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[PATCH 3/5] nvme: register ns_id attributes as default sysfs groups

[Hannes Reinecke]

We should be registering the ns_id attribute as default sysfs
attribute groups, otherwise we have a race condition between
the uevent and the attributes appearing in sysfs.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/nvme/host/core.c      | 14 +++++++-------
 drivers/nvme/host/multipath.c | 12 +++---------
 drivers/nvme/host/nvme.h      |  2 +-
 3 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e77e6418a21c..45dab8d4aff2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2689,6 +2689,11 @@ const struct attribute_group nvme_ns_id_attr_group = {
 	.is_visible	= nvme_ns_id_attrs_are_visible,
 };
 
+const struct attribute_group *nvme_ns_id_attr_groups[] = {
+	&nvme_ns_id_attr_group,
+	NULL,
+};
+
 #define nvme_show_str_function(field)						\
 static ssize_t  field##_show(struct device *dev,				\
 			    struct device_attribute *attr, char *buf)		\
@@ -3056,11 +3061,8 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 	kfree(id);
 
-	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);
+	device_add_disk_with_groups(ctrl->device, ns->disk,
+				    nvme_ns_id_attr_groups);
 	if (ns->ndev && nvme_nvm_register_sysfs(ns))
 		pr_warn("%s: failed to register lightnvm sysfs group for identification\n",
 			ns->disk->disk_name);
@@ -3087,8 +3089,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);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 1ffd3e8b13a1..2f57ff69e26d 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -226,13 +226,9 @@ void nvme_mpath_add_disk(struct nvme_ns_head *head)
 		return;
 
 	mutex_lock(&head->subsys->lock);
-	if (!(head->disk->flags & GENHD_FL_UP)) {
-		device_add_disk(&head->subsys->dev, head->disk);
-		if (sysfs_create_group(&disk_to_dev(head->disk)->kobj,
-				&nvme_ns_id_attr_group))
-			pr_warn("%s: failed to create sysfs group for identification\n",
-				head->disk->disk_name);
-	}
+	if (!(head->disk->flags & GENHD_FL_UP))
+		device_add_disk_with_groups(&head->subsys->dev, head->disk,
+					    nvme_ns_id_attr_groups);
 	mutex_unlock(&head->subsys->lock);
 }
 
@@ -240,8 +236,6 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 	if (!head->disk)
 		return;
-	sysfs_remove_group(&disk_to_dev(head->disk)->kobj,
-			   &nvme_ns_id_attr_group);
 	del_gendisk(head->disk);
 	blk_set_queue_dying(head->disk->queue);
 	/* make sure all pending bios are cleaned up */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4ad0c8ad2a27..4b911274150d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -446,7 +446,7 @@ int nvme_delete_ctrl_sync(struct nvme_ctrl *ctrl);
 int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 		u8 log_page, void *log, size_t size, u64 offset);
 
-extern const struct attribute_group nvme_ns_id_attr_group;
+extern const struct attribute_group *nvme_ns_id_attr_groups[];
 extern const struct block_device_operations nvme_ns_head_ops;
 
 #ifdef CONFIG_NVME_MULTIPATH
-- 
2.12.3

[PATCH 4/5] aoe: use device_add_disk_with_groups()

[Hannes Reinecke]

Use device_add_disk_with_groups() to avoid a race condition with
udev during startup.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/block/aoe/aoe.h    |  1 -
 drivers/block/aoe/aoeblk.c | 21 +++++++--------------
 drivers/block/aoe/aoedev.c |  1 -
 3 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index c0ebda1283cc..015c68017a1c 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -201,7 +201,6 @@ int aoeblk_init(void);
 void aoeblk_exit(void);
 void aoeblk_gdalloc(void *);
 void aoedisk_rm_debugfs(struct aoedev *d);
-void aoedisk_rm_sysfs(struct aoedev *d);
 
 int aoechr_init(void);
 void aoechr_exit(void);
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 429ebb84b592..b9d30150ce20 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -177,10 +177,15 @@ static struct attribute *aoe_attrs[] = {
 	NULL,
 };
 
-static const struct attribute_group attr_group = {
+static const struct attribute_group aoe_attr_group = {
 	.attrs = aoe_attrs,
 };
 
+static const struct attribute_group *aoe_attr_groups[] = {
+	&aoe_attr_group,
+	NULL,
+};
+
 static const struct file_operations aoe_debugfs_fops = {
 	.open = aoe_debugfs_open,
 	.read = seq_read,
@@ -220,17 +225,6 @@ aoedisk_rm_debugfs(struct aoedev *d)
 }
 
 static int
-aoedisk_add_sysfs(struct aoedev *d)
-{
-	return sysfs_create_group(&disk_to_dev(d->gd)->kobj, &attr_group);
-}
-void
-aoedisk_rm_sysfs(struct aoedev *d)
-{
-	sysfs_remove_group(&disk_to_dev(d->gd)->kobj, &attr_group);
-}
-
-static int
 aoeblk_open(struct block_device *bdev, fmode_t mode)
 {
 	struct aoedev *d = bdev->bd_disk->private_data;
@@ -417,8 +411,7 @@ aoeblk_gdalloc(void *vp)
 
 	spin_unlock_irqrestore(&d->lock, flags);
 
-	add_disk(gd);
-	aoedisk_add_sysfs(d);
+	device_add_disk_with_groups(NULL, gd, aoe_attr_groups);
 	aoedisk_add_debugfs(d);
 
 	spin_lock_irqsave(&d->lock, flags);
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index 697f735b07a4..d92fa1fe3580 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -275,7 +275,6 @@ freedev(struct aoedev *d)
 	del_timer_sync(&d->timer);
 	if (d->gd) {
 		aoedisk_rm_debugfs(d);
-		aoedisk_rm_sysfs(d);
 		del_gendisk(d->gd);
 		put_disk(d->gd);
 		blk_cleanup_queue(d->blkq);
-- 
2.12.3

[PATCH 5/5] zram: use device_add_disk_with_groups()

[Hannes Reinecke]

Use device_add_disk_with_groups() to avoid a race condition with
udev during startup.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/block/zram/zram_drv.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index da51293e7c03..6f55676e2164 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1619,6 +1619,11 @@ static const struct attribute_group zram_disk_attr_group = {
 	.attrs = zram_disk_attrs,
 };
 
+static const struct attribute_group *zram_disk_attr_groups[] = {
+	&zram_disk_attr_group,
+	NULL,
+};
+
 /*
  * Allocate and initialize new zram device. the function returns
  * '>= 0' device_id upon success, and negative value otherwise.
@@ -1699,24 +1704,14 @@ static int zram_add(void)
 
 	zram->disk->queue->backing_dev_info->capabilities |=
 			(BDI_CAP_STABLE_WRITES | BDI_CAP_SYNCHRONOUS_IO);
-	add_disk(zram->disk);
-
-	ret = sysfs_create_group(&disk_to_dev(zram->disk)->kobj,
-				&zram_disk_attr_group);
-	if (ret < 0) {
-		pr_err("Error creating sysfs group for device %d\n",
-				device_id);
-		goto out_free_disk;
-	}
+	device_add_disk_with_groups(NULL, zram->disk, zram_disk_attr_groups);
+
 	strlcpy(zram->compressor, default_compressor, sizeof(zram->compressor));
 
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;
 
-out_free_disk:
-	del_gendisk(zram->disk);
-	put_disk(zram->disk);
 out_free_queue:
 	blk_cleanup_queue(queue);
 out_free_idr:
@@ -1745,15 +1740,6 @@ static int zram_remove(struct zram *zram)
 	mutex_unlock(&bdev->bd_mutex);
 
 	zram_debugfs_unregister(zram);
-	/*
-	 * Remove sysfs first, so no one will perform a disksize
-	 * store while we destroy the devices. This also helps during
-	 * hot_remove -- zram_reset_device() is the last holder of
-	 * ->init_lock, no later/concurrent disksize_store() or any
-	 * other sysfs handlers are possible.
-	 */
-	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
-			&zram_disk_attr_group);
 
 	/* Make sure all the pending I/O are finished */
 	fsync_bdev(bdev);
-- 
2.12.3