[PATCH 0/5] block: replace incorrect uses of GENHD_FL_UP

[PATCH 0/5] block: replace incorrect uses of GENHD_FL_UP

[Luis Chamberlain]

I've bumped this from RFC to PATCH form as request by Christoph,
as it seems to line up with what he wants to do. As per Hannes
I also stuck to one form of naming, so went with blk_disk_added()
instead of blk_disk_registered() and used that instead of open
coding the flag check.

This is rebased onto next-20210720 and I've made the patch series
independent of my *add_disk*() error handling series. This goes
compile and boot tested.

Luis Chamberlain (5):
  block: add flag for add_disk() completion notation
  md: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED on is_mddev_broken()
  mmc/core/block: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED
  nvme: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED
  fs/block_dev: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

 block/genhd.c                 |  8 ++++++++
 drivers/md/md.h               |  4 +---
 drivers/mmc/core/block.c      |  2 +-
 drivers/nvme/host/core.c      |  4 ++--
 drivers/nvme/host/multipath.c |  2 +-
 fs/block_dev.c                |  5 +++--
 include/linux/genhd.h         | 11 ++++++++++-
 7 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.27.0

[PATCH 1/5] block: add flag for add_disk() completion notation

[Luis Chamberlain]

Often drivers may have complex setups where it is not
clear if their disk completed their respective *add_disk*()
call. They either have to invent a setting or, they
incorrectly use GENHD_FL_UP. Using GENHD_FL_UP however is
used internally so we know when we can add / remove
partitions safely. We can easily fail along the way
prior to add_disk() completing and still have
GENHD_FL_UP set, so it would not be correct in that case
to call del_gendisk() on the disk.

Provide a new flag then which allows us to check if
*add_disk*() completed, and conversely just make
del_gendisk() check for this for drivers so that
they can safely call del_gendisk() and we'll figure
it out if it is safe for you to call this.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c         |  8 ++++++++
 include/linux/genhd.h | 11 ++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/genhd.c b/block/genhd.c
index af4d2ab4a633..a858eed05e55 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -539,6 +539,8 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
 
 	disk_add_events(disk);
 	blk_integrity_add(disk);
+
+	disk->flags |= GENHD_FL_DISK_ADDED;
 }
 
 void device_add_disk(struct device *parent, struct gendisk *disk,
@@ -569,6 +571,9 @@ EXPORT_SYMBOL(device_add_disk_no_queue_reg);
  * with put_disk(), which should be called after del_gendisk(), if
  * __device_add_disk() was used.
  *
+ * Drivers can safely call this even if they are not sure if the respective
+ * __device_add_disk() call succeeded.
+ *
  * Drivers exist which depend on the release of the gendisk to be synchronous,
  * it should not be deferred.
  *
@@ -578,6 +583,9 @@ void del_gendisk(struct gendisk *disk)
 {
 	might_sleep();
 
+	if (!blk_disk_added(disk))
+		return;
+
 	if (WARN_ON_ONCE(!disk->queue))
 		return;
 
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 13b34177cc85..2470c8b56599 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -56,6 +56,10 @@ struct partition_meta_info {
  * Must not be set for devices which are removed entirely when the
  * media is removed.
  *
+ * ``GENHD_FL_DISK_ADDED`` (0x0002): used to clarify that the
+ * respective add_disk*() call completed successfully, so that
+ * we know we can safely process del_gendisk() on the disk.
+ *
  * ``GENHD_FL_CD`` (0x0008): the block device is a CD-ROM-style
  * device.
  * Affects responses to the ``CDROM_GET_CAPABILITY`` ioctl.
@@ -94,7 +98,7 @@ struct partition_meta_info {
  * Used for multipath devices.
  */
 #define GENHD_FL_REMOVABLE			0x0001
-/* 2 is unused (used to be GENHD_FL_DRIVERFS) */
+#define GENHD_FL_DISK_ADDED			0x0002
 /* 4 is unused (used to be GENHD_FL_MEDIA_CHANGE_NOTIFY) */
 #define GENHD_FL_CD				0x0008
 #define GENHD_FL_UP				0x0010
@@ -189,6 +193,11 @@ struct gendisk {
 #define disk_to_cdi(disk)	NULL
 #endif
 
+static inline bool blk_disk_added(struct gendisk *disk)
+{
+	return disk && (disk->flags & GENHD_FL_DISK_ADDED);
+}
+
 static inline int disk_max_parts(struct gendisk *disk)
 {
 	if (disk->flags & GENHD_FL_EXT_DEVT)
-- 
2.27.0

Re: [PATCH 1/5] block: add flag for add_disk() completion notation

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 11:20:44AM -0700, Luis Chamberlain wrote:
> Often drivers may have complex setups where it is not
> clear if their disk completed their respective *add_disk*()
> call. They either have to invent a setting or, they
> incorrectly use GENHD_FL_UP. Using GENHD_FL_UP however is
> used internally so we know when we can add / remove
> partitions safely. We can easily fail along the way
> prior to add_disk() completing and still have
> GENHD_FL_UP set, so it would not be correct in that case
> to call del_gendisk() on the disk.
> 
> Provide a new flag then which allows us to check if
> *add_disk*() completed, and conversely just make
> del_gendisk() check for this for drivers so that
> they can safely call del_gendisk() and we'll figure
> it out if it is safe for you to call this.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  block/genhd.c         |  8 ++++++++
>  include/linux/genhd.h | 11 ++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index af4d2ab4a633..a858eed05e55 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -539,6 +539,8 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>  
>  	disk_add_events(disk);
>  	blk_integrity_add(disk);
> +
> +	disk->flags |= GENHD_FL_DISK_ADDED;

I guess I failed to mention it last time - but I think this needs
to go into disk->state as dynamic state.

> + * Drivers can safely call this even if they are not sure if the respective
> + * __device_add_disk() call succeeded.
> + *
>   * Drivers exist which depend on the release of the gendisk to be synchronous,
>   * it should not be deferred.
>   *
> @@ -578,6 +583,9 @@ void del_gendisk(struct gendisk *disk)
>  {
>  	might_sleep();
>  
> +	if (!blk_disk_added(disk))
> +		return;

I still very much disagree with this check.  It just leads to really
bad driver code.  In genral we need to _fix_ the existing abuses of
the UP check in drivers, not spread this kind of sloppyness further.

[PATCH 2/5] md: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED on is_mddev_broken()

[Luis Chamberlain]

The GENHD_FL_DISK_ADDED flag is what we really want, as the
flag GENHD_FL_UP could be set on a semi-initialized device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/md/md.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/md/md.h b/drivers/md/md.h
index 832547cf038f..cf70e0cfa856 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -764,9 +764,7 @@ struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev);
 
 static inline bool is_mddev_broken(struct md_rdev *rdev, const char *md_type)
 {
-	int flags = rdev->bdev->bd_disk->flags;
-
-	if (!(flags & GENHD_FL_UP)) {
+	if (!blk_disk_added(rdev->bdev->bd_disk)) {
 		if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
 			pr_warn("md: %s: %s array has a missing/failed member\n",
 				mdname(rdev->mddev), md_type);
-- 
2.27.0

Re: [PATCH 2/5] md: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED on is_mddev_broken()

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 11:20:45AM -0700, Luis Chamberlain wrote:
> The GENHD_FL_DISK_ADDED flag is what we really want, as the
> flag GENHD_FL_UP could be set on a semi-initialized device.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>

Based on the commit log for the patch adding this check I think this
is wrong  It actually wants to detected underlying devices for which
del_gendisk has been called.

> ---
>  drivers/md/md.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 832547cf038f..cf70e0cfa856 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -764,9 +764,7 @@ struct md_rdev *md_find_rdev_rcu(struct mddev *mddev, dev_t dev);
>  
>  static inline bool is_mddev_broken(struct md_rdev *rdev, const char *md_type)
>  {
> -	int flags = rdev->bdev->bd_disk->flags;
> -
> -	if (!(flags & GENHD_FL_UP)) {
> +	if (!blk_disk_added(rdev->bdev->bd_disk)) {
>  		if (!test_and_set_bit(MD_BROKEN, &rdev->mddev->flags))
>  			pr_warn("md: %s: %s array has a missing/failed member\n",
>  				mdname(rdev->mddev), md_type);
> -- 
> 2.27.0
> 
---end quoted text---

[PATCH 3/5] mmc/core/block: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Luis Chamberlain]

The GENHD_FL_DISK_ADDED flag is what we really want, as the
flag GENHD_FL_UP could be set on a semi-initialized device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/mmc/core/block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index ce8aed562929..e9818c79fa59 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2644,7 +2644,7 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
 		 * from being accepted.
 		 */
 		card = md->queue.card;
-		if (md->disk->flags & GENHD_FL_UP) {
+		if (blk_disk_added(md->disk)) {
 			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
 			if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
 					card->ext_csd.boot_ro_lockable)
-- 
2.27.0

Re: [PATCH 3/5] mmc/core/block: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 11:20:46AM -0700, Luis Chamberlain wrote:
> The GENHD_FL_DISK_ADDED flag is what we really want, as the
> flag GENHD_FL_UP could be set on a semi-initialized device.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  drivers/mmc/core/block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index ce8aed562929..e9818c79fa59 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2644,7 +2644,7 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
>  		 * from being accepted.
>  		 */
>  		card = md->queue.card;
> -		if (md->disk->flags & GENHD_FL_UP) {
> +		if (blk_disk_added(md->disk)) {
>  			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
>  			if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
>  					card->ext_csd.boot_ro_lockable)
> -- 

I think the proper fix here is to just unwind the mmc initialization,
something like this untested patch:


diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9890a1532cb0..982f0198d8ff 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2283,7 +2283,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 					      sector_t size,
 					      bool default_ro,
 					      const char *subname,
-					      int area_type)
+					      int area_type,
+					      unsigned int part_type)
 {
 	struct mmc_blk_data *md;
 	int devidx, ret;
@@ -2329,6 +2330,7 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 	INIT_LIST_HEAD(&md->rpmbs);
 	md->usage = 1;
 	md->queue.blkdata = md;
+	md->part_type = part_type;
 
 	md->disk->major	= MMC_BLOCK_MAJOR;
 	md->disk->minors = perdev_minors;
@@ -2381,8 +2383,43 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
 		md->disk->disk_name, mmc_card_id(card), mmc_card_name(card),
 		cap_str, md->read_only ? "(ro)" : "");
 
+	device_add_disk(md->parent, md->disk, NULL);
+	md->force_ro.show = force_ro_show;
+	md->force_ro.store = force_ro_store;
+	sysfs_attr_init(&md->force_ro.attr);
+	md->force_ro.attr.name = "force_ro";
+	md->force_ro.attr.mode = S_IRUGO | S_IWUSR;
+	ret = device_create_file(disk_to_dev(md->disk), &md->force_ro);
+	if (ret)
+		goto force_ro_fail;
+
+	if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
+	     card->ext_csd.boot_ro_lockable) {
+		umode_t mode;
+
+		if (card->ext_csd.boot_ro_lock & EXT_CSD_BOOT_WP_B_PWR_WP_DIS)
+			mode = S_IRUGO;
+		else
+			mode = S_IRUGO | S_IWUSR;
+
+		md->power_ro_lock.show = power_ro_lock_show;
+		md->power_ro_lock.store = power_ro_lock_store;
+		sysfs_attr_init(&md->power_ro_lock.attr);
+		md->power_ro_lock.attr.mode = mode;
+		md->power_ro_lock.attr.name =
+					"ro_lock_until_next_power_on";
+		ret = device_create_file(disk_to_dev(md->disk),
+				&md->power_ro_lock);
+		if (ret)
+			goto power_ro_lock_fail;
+	}
 	return md;
 
+ power_ro_lock_fail:
+	device_remove_file(disk_to_dev(md->disk), &md->force_ro);
+ force_ro_fail:
+	del_gendisk(md->disk);
+ 	// XXX: undo mmc_init_queue
  err_kfree:
 	kfree(md);
  out:
@@ -2410,7 +2447,7 @@ static struct mmc_blk_data *mmc_blk_alloc(struct mmc_card *card)
 	}
 
 	return mmc_blk_alloc_req(card, &card->dev, size, false, NULL,
-					MMC_BLK_DATA_AREA_MAIN);
+					MMC_BLK_DATA_AREA_MAIN, 0);
 }
 
 static int mmc_blk_alloc_part(struct mmc_card *card,
@@ -2424,10 +2461,9 @@ static int mmc_blk_alloc_part(struct mmc_card *card,
 	struct mmc_blk_data *part_md;
 
 	part_md = mmc_blk_alloc_req(card, disk_to_dev(md->disk), size, default_ro,
-				    subname, area_type);
+				    subname, area_type, part_type);
 	if (IS_ERR(part_md))
 		return PTR_ERR(part_md);
-	part_md->part_type = part_type;
 	list_add(&part_md->part, &md->part);
 
 	return 0;
@@ -2628,27 +2664,23 @@ static int mmc_blk_alloc_parts(struct mmc_card *card, struct mmc_blk_data *md)
 
 static void mmc_blk_remove_req(struct mmc_blk_data *md)
 {
-	struct mmc_card *card;
+	struct mmc_card *card = md->queue.card;
 
-	if (md) {
-		/*
-		 * Flush remaining requests and free queues. It
-		 * is freeing the queue that stops new requests
-		 * from being accepted.
-		 */
-		card = md->queue.card;
-		if (md->disk->flags & GENHD_FL_UP) {
-			device_remove_file(disk_to_dev(md->disk), &md->force_ro);
-			if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
-					card->ext_csd.boot_ro_lockable)
-				device_remove_file(disk_to_dev(md->disk),
-					&md->power_ro_lock);
-
-			del_gendisk(md->disk);
-		}
-		mmc_cleanup_queue(&md->queue);
-		mmc_blk_put(md);
+	/*
+	 * Flush remaining requests and free queues. It is freeing the queue
+	 * that stops new requests from being accepted.
+	 */
+	if (md->disk->flags & GENHD_FL_UP) {
+		device_remove_file(disk_to_dev(md->disk), &md->force_ro);
+		if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
+				card->ext_csd.boot_ro_lockable)
+			device_remove_file(disk_to_dev(md->disk),
+				&md->power_ro_lock);
+
+		del_gendisk(md->disk);
 	}
+	mmc_cleanup_queue(&md->queue);
+	mmc_blk_put(md);
 }
 
 static void mmc_blk_remove_parts(struct mmc_card *card,
@@ -2672,51 +2704,6 @@ static void mmc_blk_remove_parts(struct mmc_card *card,
 	}
 }
 
-static int mmc_add_disk(struct mmc_blk_data *md)
-{
-	int ret;
-	struct mmc_card *card = md->queue.card;
-
-	device_add_disk(md->parent, md->disk, NULL);
-	md->force_ro.show = force_ro_show;
-	md->force_ro.store = force_ro_store;
-	sysfs_attr_init(&md->force_ro.attr);
-	md->force_ro.attr.name = "force_ro";
-	md->force_ro.attr.mode = S_IRUGO | S_IWUSR;
-	ret = device_create_file(disk_to_dev(md->disk), &md->force_ro);
-	if (ret)
-		goto force_ro_fail;
-
-	if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
-	     card->ext_csd.boot_ro_lockable) {
-		umode_t mode;
-
-		if (card->ext_csd.boot_ro_lock & EXT_CSD_BOOT_WP_B_PWR_WP_DIS)
-			mode = S_IRUGO;
-		else
-			mode = S_IRUGO | S_IWUSR;
-
-		md->power_ro_lock.show = power_ro_lock_show;
-		md->power_ro_lock.store = power_ro_lock_store;
-		sysfs_attr_init(&md->power_ro_lock.attr);
-		md->power_ro_lock.attr.mode = mode;
-		md->power_ro_lock.attr.name =
-					"ro_lock_until_next_power_on";
-		ret = device_create_file(disk_to_dev(md->disk),
-				&md->power_ro_lock);
-		if (ret)
-			goto power_ro_lock_fail;
-	}
-	return ret;
-
-power_ro_lock_fail:
-	device_remove_file(disk_to_dev(md->disk), &md->force_ro);
-force_ro_fail:
-	del_gendisk(md->disk);
-
-	return ret;
-}
-
 #ifdef CONFIG_DEBUG_FS
 
 static int mmc_dbg_card_status_get(void *data, u64 *val)
@@ -2882,7 +2869,7 @@ static void mmc_blk_remove_debugfs(struct mmc_card *card,
 
 static int mmc_blk_probe(struct mmc_card *card)
 {
-	struct mmc_blk_data *md, *part_md;
+	struct mmc_blk_data *md;
 	int ret = 0;
 
 	/*
@@ -2900,6 +2887,8 @@ static int mmc_blk_probe(struct mmc_card *card)
 		return -ENOMEM;
 	}
 
+	dev_set_drvdata(&card->dev, md);
+
 	md = mmc_blk_alloc(card);
 	if (IS_ERR(md)) {
 		ret = PTR_ERR(md);
@@ -2910,18 +2899,6 @@ static int mmc_blk_probe(struct mmc_card *card)
 	if (ret)
 		goto out;
 
-	dev_set_drvdata(&card->dev, md);
-
-	ret = mmc_add_disk(md);
-	if (ret)
-		goto out;
-
-	list_for_each_entry(part_md, &md->part, part) {
-		ret = mmc_add_disk(part_md);
-		if (ret)
-			goto out;
-	}
-
 	/* Add two debugfs entries */
 	mmc_blk_add_debugfs(card, md);
 

[PATCH 4/5] nvme: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Luis Chamberlain]

The GENHD_FL_DISK_ADDED flag is what we really want, as the
flag GENHD_FL_UP could be set on a semi-initialized device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/nvme/host/core.c      | 4 ++--
 drivers/nvme/host/multipath.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 11779be42186..7be78491c838 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1819,7 +1819,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
 static inline bool nvme_first_scan(struct gendisk *disk)
 {
 	/* nvme_alloc_ns() scans the disk prior to adding it */
-	return !(disk->flags & GENHD_FL_UP);
+	return !(blk_disk_added(disk));
 }
 
 static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
@@ -3823,7 +3823,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	nvme_mpath_clear_current_path(ns);
 	synchronize_srcu(&ns->head->srcu); /* wait for concurrent submissions */
 
-	if (ns->disk->flags & GENHD_FL_UP) {
+	if (blk_disk_added(ns->disk)) {
 		if (!nvme_ns_head_multipath(ns->head))
 			nvme_cdev_del(&ns->cdev, &ns->cdev_device);
 		del_gendisk(ns->disk);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 0ea5298469c3..f77bd2d5c1a9 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -764,7 +764,7 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
 {
 	if (!head->disk)
 		return;
-	if (head->disk->flags & GENHD_FL_UP) {
+	if (blk_disk_added(head->disk)) {
 		nvme_cdev_del(&head->cdev, &head->cdev_device);
 		del_gendisk(head->disk);
 	}
-- 
2.27.0

Re: [PATCH 4/5] nvme: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 11:20:47AM -0700, Luis Chamberlain wrote:
> The GENHD_FL_DISK_ADDED flag is what we really want, as the
> flag GENHD_FL_UP could be set on a semi-initialized device.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  drivers/nvme/host/core.c      | 4 ++--
>  drivers/nvme/host/multipath.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 11779be42186..7be78491c838 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1819,7 +1819,7 @@ static void nvme_update_disk_info(struct gendisk *disk,
>  static inline bool nvme_first_scan(struct gendisk *disk)
>  {
>  	/* nvme_alloc_ns() scans the disk prior to adding it */
> -	return !(disk->flags & GENHD_FL_UP);
> +	return !(blk_disk_added(disk));
>  }

So this on is obviously right as it wants to check for the probe
time scan.  Although we don't need the braces.

>
>  
>  static void nvme_set_chunk_sectors(struct nvme_ns *ns, struct nvme_id_ns *id)
> @@ -3823,7 +3823,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>  	nvme_mpath_clear_current_path(ns);
>  	synchronize_srcu(&ns->head->srcu); /* wait for concurrent submissions */
>  
> -	if (ns->disk->flags & GENHD_FL_UP) {
> +	if (blk_disk_added(ns->disk)) {

This is something that goes back to before the damn of time, but I do
not think we actually need it.  All the errors paths before alloc_disk
and add_disk just directly free the ns and never end up here.

>  		if (!nvme_ns_head_multipath(ns->head))
>  			nvme_cdev_del(&ns->cdev, &ns->cdev_device);
>  		del_gendisk(ns->disk);
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 0ea5298469c3..f77bd2d5c1a9 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -764,7 +764,7 @@ void nvme_mpath_remove_disk(struct nvme_ns_head *head)
>  {
>  	if (!head->disk)
>  		return;
> -	if (head->disk->flags & GENHD_FL_UP) {
> +	if (blk_disk_added(head->disk)) {
>  		nvme_cdev_del(&head->cdev, &head->cdev_device);
>  		del_gendisk(head->disk);
>  	}

This one is sort of correct due to the lazy disk addition.  But we
could and probably should check for NVME_NSHEAD_DISK_LIVE instead.

[PATCH 5/5] fs/block_dev: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Luis Chamberlain]

The GENHD_FL_DISK_ADDED flag is what we really want, as the
flag GENHD_FL_UP could be set on a semi-initialized device.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 fs/block_dev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 0c424a0cadaa..0b77f9be8e28 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1338,7 +1338,8 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 	disk = bdev->bd_disk;
 	if (!kobject_get_unless_zero(&disk_to_dev(disk)->kobj))
 		goto bdput;
-	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
+	if ((disk->flags &
+	    (GENHD_FL_DISK_ADDED | GENHD_FL_HIDDEN)) != GENHD_FL_DISK_ADDED)
 		goto put_disk;
 	if (!try_module_get(bdev->bd_disk->fops->owner))
 		goto put_disk;
@@ -1407,7 +1408,7 @@ struct block_device *blkdev_get_by_dev(dev_t dev, fmode_t mode, void *holder)
 
 	mutex_lock(&disk->open_mutex);
 	ret = -ENXIO;
-	if (!(disk->flags & GENHD_FL_UP))
+	if (!blk_disk_added(disk))
 		goto abort_claiming;
 	if (bdev_is_partition(bdev))
 		ret = blkdev_get_part(bdev, mode);
-- 
2.27.0

Re: [PATCH 5/5] fs/block_dev: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED

[Christoph Hellwig]

On Tue, Jul 20, 2021 at 11:20:48AM -0700, Luis Chamberlain wrote:
> The GENHD_FL_DISK_ADDED flag is what we really want, as the
> flag GENHD_FL_UP could be set on a semi-initialized device.

No.  Here we must reject opens once del_gendisk has been called.

Re: [PATCH 0/5] block: replace incorrect uses of GENHD_FL_UP

[luomeng]

Hi:
    When the fuzz test injected memory allocation failed, I had this 
BUG_ON: kernel BUG at fs/sysfs/group.c:116.
   The cause of the bug_ON is that the add_disk memory fails to be 
allocated but no error processing is performed.
   I find your patches add error processing. So what is your next step 
with these patches.
Thanks.

Luo Meng

在 2021/7/21 2:20, Luis Chamberlain 写道:
> I've bumped this from RFC to PATCH form as request by Christoph,
> as it seems to line up with what he wants to do. As per Hannes
> I also stuck to one form of naming, so went with blk_disk_added()
> instead of blk_disk_registered() and used that instead of open
> coding the flag check.
> 
> This is rebased onto next-20210720 and I've made the patch series
> independent of my *add_disk*() error handling series. This goes
> compile and boot tested.
> 
> Luis Chamberlain (5):
>    block: add flag for add_disk() completion notation
>    md: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED on is_mddev_broken()
>    mmc/core/block: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED
>    nvme: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED
>    fs/block_dev: replace GENHD_FL_UP with GENHD_FL_DISK_ADDED
> 
>   block/genhd.c                 |  8 ++++++++
>   drivers/md/md.h               |  4 +---
>   drivers/mmc/core/block.c      |  2 +-
>   drivers/nvme/host/core.c      |  4 ++--
>   drivers/nvme/host/multipath.c |  2 +-
>   fs/block_dev.c                |  5 +++--
>   include/linux/genhd.h         | 11 ++++++++++-
>   7 files changed, 26 insertions(+), 10 deletions(-)
> 

Re: [PATCH 0/5] block: replace incorrect uses of GENHD_FL_UP

[Christoph Hellwig]

On Wed, Aug 11, 2021 at 10:42:20AM +0800, luomeng wrote:
> Hi:
>    When the fuzz test injected memory allocation failed, I had this BUG_ON:
> kernel BUG at fs/sysfs/group.c:116.
>   The cause of the bug_ON is that the add_disk memory fails to be allocated
> but no error processing is performed.
>   I find your patches add error processing. So what is your next step with
> these patches.

I have one more pending series on top of this one I need to submit here:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/alloc_disk

to make sure the disk always has a valid queue reference.  After that
Luis work to return an error from add_disk should be much easier bause
we not have defined state.

Re: [PATCH 0/5] block: replace incorrect uses of GENHD_FL_UP

[luomeng]

在 2021/8/11 13:19, Christoph Hellwig 写道:
> On Wed, Aug 11, 2021 at 10:42:20AM +0800, luomeng wrote:
>> Hi:
>>     When the fuzz test injected memory allocation failed, I had this BUG_ON:
>> kernel BUG at fs/sysfs/group.c:116.
>>    The cause of the bug_ON is that the add_disk memory fails to be allocated
>> but no error processing is performed.
>>    I find your patches add error processing. So what is your next step with
>> these patches.
> I have one more pending series on top of this one I need to submit here:
> 
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/alloc_disk
> 
> to make sure the disk always has a valid queue reference.  After that
> Luis work to return an error from add_disk should be much easier bause
> we not have defined state.
> .
Thanks.

So how about this series, when this series will merge into linux master?

And do you submit these patches ( 
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/alloc_disk) 
on liunx?

Luo Meng