This is the second group of patches which I'd like some review on as
part of the *add_disk*() error handling conversion. While converting
drivers to add error handling, I noticed some were using the flag
GENHD_FL_UP to see if a block device is ready, or for bringing a
device down, so to call del_gendisk() safely. This first group of
patches just address the few areas where the flag is used directly.
Direct use of the flag GENHD_FL_UP is useful but incorrect as we can
add the flag in a gendisk left half built. Instead, add a flag to
actually represent the desired goal.
The next group will deal with the use of the flag for del_gendisk().
Luis Chamberlain (6):
genhd: update docs for GENHD_FL_UP
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 | 2 +-
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 | 13 +++++++++++--
7 files changed, 27 insertions(+), 9 deletions(-)
--
2.27.0
The GENHD_FL_UP is used internally so we can know when we can add and
remove partitions, so just clarify that. Right now it has this 1980's
description comparing it to network drivers "ifconfig up" idea, and
that can easily lead to invalid uses.
Signed-off-by: Luis Chamberlain <[email protected]>
---
include/linux/genhd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 51f27b9b38b5..dc07a957c9e1 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -61,7 +61,7 @@ struct partition_meta_info {
* Affects responses to the ``CDROM_GET_CAPABILITY`` ioctl.
*
* ``GENHD_FL_UP`` (0x0010): indicates that the block device is "up",
- * with a similar meaning to network interfaces.
+ * and we can add / remove partitions.
*
* ``GENHD_FL_SUPPRESS_PARTITION_INFO`` (0x0020): don't include
* partition information in ``/proc/partitions`` or in the output of
--
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index 832547cf038f..80561bca1f51 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -766,7 +766,7 @@ 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 (!(flags & GENHD_FL_DISK_ADDED)) {
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
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 c6c9c196ff27..72703d243b44 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -630,6 +630,8 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
if (ret)
goto exit_del_events;
+ disk->flags |= GENHD_FL_DISK_ADDED;
+
return 0;
exit_del_events:
disk_del_events(disk);
@@ -677,6 +679,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.
*
@@ -686,6 +691,9 @@ void del_gendisk(struct gendisk *disk)
{
might_sleep();
+ if (!blk_disk_registered(disk))
+ return;
+
if (WARN_ON_ONCE(!disk->queue))
return;
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index dc07a957c9e1..73024416d2d5 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_registered(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/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..3848353fba11 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 !(disk->flags & GENHD_FL_DISK_ADDED);
}
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 (ns->disk->flags & GENHD_FL_DISK_ADDED) {
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..8048678969ba 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 (head->disk->flags & GENHD_FL_DISK_ADDED) {
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]>
---
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..c41d0e550d39 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 (!(disk->flags & GENHD_FL_DISK_ADDED))
goto abort_claiming;
if (bdev_is_partition(bdev))
ret = blkdev_get_part(bdev, mode);
--
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 9890a1532cb0..5bb3e604e618 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2637,7 +2637,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 (md->disk->flags & GENHD_FL_DISK_ADDED) {
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
On 7/15/21 10:23 PM, 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 c6c9c196ff27..72703d243b44 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -630,6 +630,8 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
> if (ret)
> goto exit_del_events;
>
> + disk->flags |= GENHD_FL_DISK_ADDED;
> +
> return 0;
> exit_del_events:
> disk_del_events(disk);
> @@ -677,6 +679,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.
> *
> @@ -686,6 +691,9 @@ void del_gendisk(struct gendisk *disk)
> {
> might_sleep();
>
> + if (!blk_disk_registered(disk))
> + return;
> +
> if (WARN_ON_ONCE(!disk->queue))
> return;
>
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index dc07a957c9e1..73024416d2d5 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_registered(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)
>
Bah. The flag is named 'DISK_ADDED', and the wrapper 'disk_registered'.
Please use the same wording (either 'added' or 'registered') for both to
avoid confusion.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 7/15/21 10:23 PM, Luis Chamberlain wrote:
> The GENHD_FL_UP is used internally so we can know when we can add and
> remove partitions, so just clarify that. Right now it has this 1980's
> description comparing it to network drivers "ifconfig up" idea, and
> that can easily lead to invalid uses.
>
> Signed-off-by: Luis Chamberlain <[email protected]>
> ---
> include/linux/genhd.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 51f27b9b38b5..dc07a957c9e1 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -61,7 +61,7 @@ struct partition_meta_info {
> * Affects responses to the ``CDROM_GET_CAPABILITY`` ioctl.
> *
> * ``GENHD_FL_UP`` (0x0010): indicates that the block device is "up",
> - * with a similar meaning to network interfaces.
> + * and we can add / remove partitions.
> *
> * ``GENHD_FL_SUPPRESS_PARTITION_INFO`` (0x0020): don't include
> * partition information in ``/proc/partitions`` or in the output of
>
Reviewed-by: Hannes Reinecke <[email protected]>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 7/15/21 10:23 PM, 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/md/md.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 832547cf038f..80561bca1f51 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -766,7 +766,7 @@ 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 (!(flags & GENHD_FL_DISK_ADDED)) {
> 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);
>
Why again did you introduce the wrapper?
Shouldn't it be used here?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 7/15/21 10:23 PM, 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 9890a1532cb0..5bb3e604e618 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -2637,7 +2637,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 (md->disk->flags & GENHD_FL_DISK_ADDED) {
> 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)
>
Same here: as you have provided a wrapper please use it.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On 7/15/21 10:23 PM, 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..3848353fba11 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 !(disk->flags & GENHD_FL_DISK_ADDED);
> }
>
> 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 (ns->disk->flags & GENHD_FL_DISK_ADDED) {
> 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..8048678969ba 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 (head->disk->flags & GENHD_FL_DISK_ADDED) {
> nvme_cdev_del(&head->cdev, &head->cdev_device);
> del_gendisk(head->disk);
> }
>
Same here: please use the wrapper.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
[email protected] +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
On Fri, Jul 16, 2021 at 07:49:49AM +0200, Hannes Reinecke wrote:
> On 7/15/21 10:23 PM, 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 c6c9c196ff27..72703d243b44 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -630,6 +630,8 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
> > if (ret)
> > goto exit_del_events;
> > + disk->flags |= GENHD_FL_DISK_ADDED;
> > +
> > return 0;
> > exit_del_events:
> > disk_del_events(disk);
> > @@ -677,6 +679,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.
> > *
> > @@ -686,6 +691,9 @@ void del_gendisk(struct gendisk *disk)
> > {
> > might_sleep();
> > + if (!blk_disk_registered(disk))
> > + return;
> > +
> > if (WARN_ON_ONCE(!disk->queue))
> > return;
> > diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> > index dc07a957c9e1..73024416d2d5 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_registered(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)
> >
> Bah. The flag is named 'DISK_ADDED', and the wrapper 'disk_registered'.
> Please use the same wording (either 'added' or 'registered') for both to
> avoid confusion.
Indeed, will stick with blk_disk_added() then.
Luis
On Fri, Jul 16, 2021 at 07:51:00AM +0200, Hannes Reinecke wrote:
> On 7/15/21 10:23 PM, 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/md/md.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/md.h b/drivers/md/md.h
> > index 832547cf038f..80561bca1f51 100644
> > --- a/drivers/md/md.h
> > +++ b/drivers/md/md.h
> > @@ -766,7 +766,7 @@ 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 (!(flags & GENHD_FL_DISK_ADDED)) {
> > 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);
> >
> Why again did you introduce the wrapper?
> Shouldn't it be used here?
Indeed, and that lets us remove the flag copy. Will fix. Thanks
for the review.
Luis
On Thu, Jul 15, 2021 at 01:23:36PM -0700, Luis Chamberlain wrote:
> The GENHD_FL_UP is used internally so we can know when we can add and
> remove partitions, so just clarify that. Right now it has this 1980's
> description comparing it to network drivers "ifconfig up" idea, and
> that can easily lead to invalid uses.
GENHD_FL_UP actually is pretty horrible - it uses a value used for
something that should be mostly read only at device runtime for
state an does so badly. I actually have a patch to kill it, as we can
just check if the inode for the whole device blockdev is hashed instead.
So while patch 1 seems a little pointless given where I'd like to go,
the rest of the series goes exactly where I'd like to move things.
Can you respin this one quickly and send it out to Jens without the RFC?
> {
> might_sleep();
>
> + if (!blk_disk_registered(disk))
> + return;
> +
Can't say I like this all that much. Drivers should keep some
basic sanity for their unregister path, and while blk_disk_registered
can be useful, it's uses should be kept at a minimum.
On Mon, Jul 19, 2021 at 11:00:36AM +0100, Christoph Hellwig wrote:
> > {
> > might_sleep();
> >
> > + if (!blk_disk_registered(disk))
> > + return;
> > +
>
> Can't say I like this all that much. Drivers should keep some
> basic sanity for their unregister path, and while blk_disk_registered
> can be useful, it's uses should be kept at a minimum.
This just means quite a bit of drivers have to invent some scheme to
keep tabs on if registration was completed or not on their own... I
can't see too much downfall for us to embrace this. Anyway I'll keep
it in the mix and respin it as proper patch as it seems you suggested
a respin of the series.
Luis
On Thu, Jul 15, 2021 at 5:24 PM Luis Chamberlain <[email protected]> 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/md/md.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index 832547cf038f..80561bca1f51 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -766,7 +766,7 @@ 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 (!(flags & GENHD_FL_DISK_ADDED)) {
Thanks for the patch Luis! And thanks Christoph for looping me in on
the last iteration.
I think specifically for md, both flags are interchangeable - if
add_disk() is not completed, I'm pretty sure we cannot have the array
properly working hence we shouldn't ever reach this check for such
device. Nevertheless, technically speaking Christoph seems correct and
we are checking here in fact if the disk was del_gendisk'ed().
My opinion is that we don't need to change this usage, if possible,
but I'm not strongly against the change if you feel it fits better.
Just double-checking - after del_gendisk(), this flag is removed anyways right?
Oh, and if possible, loop me in CC for next revisions, using this email.
Cheers,
Guilherme