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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
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---
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 <[email protected]>
> ---
> 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);
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.
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 <[email protected]>
> ---
> 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.
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(-)
>
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.
?? 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