| 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 |
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
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
> { > 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
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)
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(-)