diff mbox series

[RFC,2/6] block: add flag for add_disk() completion notation

Message ID 20210715202341.2016612-3-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series block: enhance use of GENHD_FL_UP | expand

Commit Message

Luis Chamberlain July 15, 2021, 8:23 p.m. UTC
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(-)

Comments

Hannes Reinecke July 16, 2021, 5:49 a.m. UTC | #1
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 <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 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
Luis Chamberlain July 16, 2021, 8 p.m. UTC | #2
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 <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 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
Christoph Hellwig July 19, 2021, 10 a.m. UTC | #3
>  {
>  	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.
Luis Chamberlain July 19, 2021, 10:38 p.m. UTC | #4
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
diff mbox series

Patch

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)