two fixup for the block_device / hd_struct merge

two fixup for the block_device / hd_struct merge

[Christoph Hellwig]

Hi Jens,

find two fixes for the block_device / hd_struct merge series attached.

[PATCH 1/2] block: grab a device refcount in disk_uevent

[Christoph Hellwig]

Sending uevents requires the struct device to be alive.  To
ensure that grab the device refcount instead of just an inode
reference.

Fixes: bc359d03c7ec ("block: add a disk_uevent helper")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 79aa40b4c39c..af4d2ab4a633 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -365,12 +365,12 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action)
 	xa_for_each(&disk->part_tbl, idx, part) {
 		if (bdev_is_partition(part) && !bdev_nr_sectors(part))
 			continue;
-		if (!bdgrab(part))
+		if (!kobject_get_unless_zero(&part->bd_device.kobj))
 			continue;
 
 		rcu_read_unlock();
 		kobject_uevent(bdev_kobj(part), action);
-		bdput(part);
+		put_device(&part->bd_device);
 		rcu_read_lock();
 	}
 	rcu_read_unlock();
-- 
2.30.2

Re: [PATCH 1/2] block: grab a device refcount in disk_uevent

[Ming Lei]

On Thu, Jul 01, 2021 at 10:16:37AM +0200, Christoph Hellwig wrote:
> Sending uevents requires the struct device to be alive.  To
> ensure that grab the device refcount instead of just an inode
> reference.
> 
> Fixes: bc359d03c7ec ("block: add a disk_uevent helper")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/genhd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 79aa40b4c39c..af4d2ab4a633 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -365,12 +365,12 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action)
>  	xa_for_each(&disk->part_tbl, idx, part) {
>  		if (bdev_is_partition(part) && !bdev_nr_sectors(part))
>  			continue;
> -		if (!bdgrab(part))
> +		if (!kobject_get_unless_zero(&part->bd_device.kobj))
>  			continue;

->bd_device is embedded in the block device, and it has same lifetime
with the block device, even part_release() calls bdput() to release this
device, so why doesn't work by holding a inode reference?


Thanks,
Ming

Re: [PATCH 1/2] block: grab a device refcount in disk_uevent

[Christoph Hellwig]

On Thu, Jul 01, 2021 at 04:54:05PM +0800, Ming Lei wrote:
> On Thu, Jul 01, 2021 at 10:16:37AM +0200, Christoph Hellwig wrote:
> > Sending uevents requires the struct device to be alive.  To
> > ensure that grab the device refcount instead of just an inode
> > reference.
> > 
> > Fixes: bc359d03c7ec ("block: add a disk_uevent helper")
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  block/genhd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index 79aa40b4c39c..af4d2ab4a633 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -365,12 +365,12 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action)
> >  	xa_for_each(&disk->part_tbl, idx, part) {
> >  		if (bdev_is_partition(part) && !bdev_nr_sectors(part))
> >  			continue;
> > -		if (!bdgrab(part))
> > +		if (!kobject_get_unless_zero(&part->bd_device.kobj))
> >  			continue;
> 
> ->bd_device is embedded in the block device, and it has same lifetime
> with the block device, even part_release() calls bdput() to release this
> device, so why doesn't work by holding a inode reference?

Because sending a uevent on a device that has device_del called on it
is going to blow up.

Re: [PATCH 1/2] block: grab a device refcount in disk_uevent

[Ming Lei]

On Thu, Jul 01, 2021 at 11:02:32AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 01, 2021 at 04:54:05PM +0800, Ming Lei wrote:
> > On Thu, Jul 01, 2021 at 10:16:37AM +0200, Christoph Hellwig wrote:
> > > Sending uevents requires the struct device to be alive.  To
> > > ensure that grab the device refcount instead of just an inode
> > > reference.
> > > 
> > > Fixes: bc359d03c7ec ("block: add a disk_uevent helper")
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  block/genhd.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index 79aa40b4c39c..af4d2ab4a633 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -365,12 +365,12 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action)
> > >  	xa_for_each(&disk->part_tbl, idx, part) {
> > >  		if (bdev_is_partition(part) && !bdev_nr_sectors(part))
> > >  			continue;
> > > -		if (!bdgrab(part))
> > > +		if (!kobject_get_unless_zero(&part->bd_device.kobj))
> > >  			continue;
> > 
> > ->bd_device is embedded in the block device, and it has same lifetime
> > with the block device, even part_release() calls bdput() to release this
> > device, so why doesn't work by holding a inode reference?
> 
> Because sending a uevent on a device that has device_del called on it
> is going to blow up.

But grabbing one reference can't prevent device_del() from being called.

IMO, if driver core doesn't allow to sending uevent on one deleted device,
it should return a failure instead of kernel panic.

Thanks,
Ming

Re: [PATCH 1/2] block: grab a device refcount in disk_uevent

[Ming Lei]

On Thu, Jul 01, 2021 at 05:17:22PM +0800, Ming Lei wrote:
> On Thu, Jul 01, 2021 at 11:02:32AM +0200, Christoph Hellwig wrote:
> > On Thu, Jul 01, 2021 at 04:54:05PM +0800, Ming Lei wrote:
> > > On Thu, Jul 01, 2021 at 10:16:37AM +0200, Christoph Hellwig wrote:
> > > > Sending uevents requires the struct device to be alive.  To
> > > > ensure that grab the device refcount instead of just an inode
> > > > reference.
> > > > 
> > > > Fixes: bc359d03c7ec ("block: add a disk_uevent helper")
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > >  block/genhd.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/block/genhd.c b/block/genhd.c
> > > > index 79aa40b4c39c..af4d2ab4a633 100644
> > > > --- a/block/genhd.c
> > > > +++ b/block/genhd.c
> > > > @@ -365,12 +365,12 @@ void disk_uevent(struct gendisk *disk, enum kobject_action action)
> > > >  	xa_for_each(&disk->part_tbl, idx, part) {
> > > >  		if (bdev_is_partition(part) && !bdev_nr_sectors(part))
> > > >  			continue;
> > > > -		if (!bdgrab(part))
> > > > +		if (!kobject_get_unless_zero(&part->bd_device.kobj))
> > > >  			continue;
> > > 
> > > ->bd_device is embedded in the block device, and it has same lifetime
> > > with the block device, even part_release() calls bdput() to release this
> > > device, so why doesn't work by holding a inode reference?
> > 
> > Because sending a uevent on a device that has device_del called on it
> > is going to blow up.
> 
> But grabbing one reference can't prevent device_del() from being called.
> 
> IMO, if driver core doesn't allow to sending uevent on one deleted device,
> it should return a failure instead of kernel panic.

Maybe the reason is that sending uevent needs the device/kobject not
'released' in viewpoint of driver core world, other device resource(such
as device name) may be referred but have been freed.

If that is the reason, this patch looks fine, and
kobject_get_unless_zero() may be replaced with get_device() since the
referred memory isn't freed yet.

Thanks,
Ming

Re: [PATCH 1/2] block: grab a device refcount in disk_uevent

[Christoph Hellwig]

On Thu, Jul 01, 2021 at 05:27:37PM +0800, Ming Lei wrote:
> 
> Maybe the reason is that sending uevent needs the device/kobject not
> 'released' in viewpoint of driver core world, other device resource(such
> as device name) may be referred but have been freed.
> 
> If that is the reason, this patch looks fine, and
> kobject_get_unless_zero() may be replaced with get_device() since the
> referred memory isn't freed yet.

Yes, sorry.  Braino while looking up the cause.  But there isn't much
of a point in sending a uevent to a partition already deleted, so
I think the unless_zero here still makes sense.

[PATCH 2/2] block: remove the bdgrab in blk_drop_partitions

[Christoph Hellwig]

There is no need to hold a bdev reference when removing the partition.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/partitions/core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 347c56a51d87..3907cebd2be9 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -537,12 +537,8 @@ void blk_drop_partitions(struct gendisk *disk)
 
 	lockdep_assert_held(&disk->open_mutex);
 
-	xa_for_each_start(&disk->part_tbl, idx, part, 1) {
-		if (!bdgrab(part))
-			continue;
+	xa_for_each_start(&disk->part_tbl, idx, part, 1)
 		delete_partition(part);
-		bdput(part);
-	}
 }
 
 static bool blk_add_partition(struct gendisk *disk,
-- 
2.30.2

Re: two fixup for the block_device / hd_struct merge

[Jens Axboe]

On 7/1/21 2:16 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> find two fixes for the block_device / hd_struct merge series attached.

Applied, thanks.

-- 
Jens Axboe