fixes and cleanups for block_device refcounting

fixes and cleanups for block_device refcounting

[Christoph Hellwig]

Hi Jens,

this series fixes up a possible race with the block_device lookup
changes, and the finishes off the conversion to stop using the inode
refcount for block devices.

Diffstat:
 block/genhd.c           |    5 +--
 block/partitions/core.c |   14 ++++-----
 drivers/block/loop.c    |    5 ---
 fs/block_dev.c          |   71 +++++++++++++++---------------------------------
 fs/btrfs/zoned.c        |   11 ++-----
 include/linux/blkdev.h  |    2 -
 6 files changed, 34 insertions(+), 74 deletions(-)

[PATCH 1/8] block: delay freeing the gendisk

[Christoph Hellwig]

blkdev_get_no_open acquires a reference to the block_device through
the block device inode and then tries to acquire a device model
reference to the gendisk.  But at this point the disk migh already
be freed (although the race is free).  Fix this by only freeing the
gendisk from the whole device bdevs ->free_inode callback as well.

Fixes: 22ae8ce8b892 ("block: simplify bdev/disk lookup in blkdev_get")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c  | 3 +--
 fs/block_dev.c | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index af4d2ab4a633..e6d782714ad3 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1079,10 +1079,9 @@ static void disk_release(struct device *dev)
 	disk_release_events(disk);
 	kfree(disk->random);
 	xa_destroy(&disk->part_tbl);
-	bdput(disk->part0);
 	if (test_bit(GD_QUEUE_REF, &disk->state) && disk->queue)
 		blk_put_queue(disk->queue);
-	kfree(disk);
+	bdput(disk->part0);
 }
 struct class block_class = {
 	.name		= "block",
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 0c424a0cadaa..9ef4f1fc2cb0 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -812,6 +812,8 @@ static void bdev_free_inode(struct inode *inode)
 	free_percpu(bdev->bd_stats);
 	kfree(bdev->bd_meta_info);
 
+	if (!bdev_is_partition(bdev))
+		kfree(bdev->bd_disk);
 	kmem_cache_free(bdev_cachep, BDEV_I(inode));
 }
 
-- 
2.30.2

Re: [PATCH 1/8] block: delay freeing the gendisk

[Ming Lei]

On Wed, Jul 21, 2021 at 05:35:16PM +0200, Christoph Hellwig wrote:
> blkdev_get_no_open acquires a reference to the block_device through
> the block device inode and then tries to acquire a device model
> reference to the gendisk.  But at this point the disk migh already
> be freed (although the race is free).  Fix this by only freeing the
> gendisk from the whole device bdevs ->free_inode callback as well.
> 
> Fixes: 22ae8ce8b892 ("block: simplify bdev/disk lookup in blkdev_get")
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

[PATCH 2/8] block: assert the locking state in delete_partition

[Christoph Hellwig]

Add a lockdep assert instead of the outdated locking comment.

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

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 4230d4f71879..9902b1635b7d 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -281,12 +281,10 @@ struct device_type part_type = {
 	.uevent		= part_uevent,
 };
 
-/*
- * Must be called either with open_mutex held, before a disk can be opened or
- * after all disk users are gone.
- */
 static void delete_partition(struct block_device *part)
 {
+	lockdep_assert_held(&part->bd_disk->open_mutex);
+
 	fsync_bdev(part);
 	__invalidate_device(part, true);
 
-- 
2.30.2

Re: [PATCH 2/8] block: assert the locking state in delete_partition

[Ming Lei]

On Wed, Jul 21, 2021 at 05:35:17PM +0200, Christoph Hellwig wrote:
> Add a lockdep assert instead of the outdated locking comment.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ming Lei <ming.lei@redhat.com>

-- 
Ming

[PATCH 3/8] block: grab a device model reference in blkdev_get_no_open

[Christoph Hellwig]

Opening a block device needs to ensure it is fully present instead of
just the allocated memory.  Switch from an inode reference as obtained
by bdget to a full device model reference.

In fact we should not use inode references for anything in the block
layer.  There are three users left, two can be trivially removed
and the third (xen-blkfront) is a complete mess that needs more
attention.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9ef4f1fc2cb0..24a6970f3623 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1342,9 +1342,16 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 		goto bdput;
 	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
 		goto put_disk;
-	if (!try_module_get(bdev->bd_disk->fops->owner))
+	if (!try_module_get(disk->fops->owner))
 		goto put_disk;
+
+	/* switch to a device model reference instead of the inode one: */
+	if (!kobject_get_unless_zero(&bdev->bd_device.kobj))
+		goto put_module;
+	bdput(bdev);
 	return bdev;
+put_module:
+	module_put(disk->fops->owner);
 put_disk:
 	put_disk(disk);
 bdput:
@@ -1356,7 +1363,7 @@ void blkdev_put_no_open(struct block_device *bdev)
 {
 	module_put(bdev->bd_disk->fops->owner);
 	put_disk(bdev->bd_disk);
-	bdput(bdev);
+	put_device(&bdev->bd_device);
 }
 
 /**
-- 
2.30.2

Re: [PATCH 3/8] block: grab a device model reference in blkdev_get_no_open

[Ming Lei]

On Wed, Jul 21, 2021 at 05:35:18PM +0200, Christoph Hellwig wrote:
> Opening a block device needs to ensure it is fully present instead of
> just the allocated memory.  Switch from an inode reference as obtained
> by bdget to a full device model reference.
> 
> In fact we should not use inode references for anything in the block
> layer.  There are three users left, two can be trivially removed
> and the third (xen-blkfront) is a complete mess that needs more
> attention.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/block_dev.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9ef4f1fc2cb0..24a6970f3623 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1342,9 +1342,16 @@ struct block_device *blkdev_get_no_open(dev_t dev)
>  		goto bdput;
>  	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
>  		goto put_disk;
> -	if (!try_module_get(bdev->bd_disk->fops->owner))
> +	if (!try_module_get(disk->fops->owner))
>  		goto put_disk;
> +
> +	/* switch to a device model reference instead of the inode one: */
> +	if (!kobject_get_unless_zero(&bdev->bd_device.kobj))

If bdev->bd_device.kobj is grabbed in every open, getting disk reference might
be moved into add_partition, and drop it in part_release(). Then we can avoid
extra disk reference in open/close().

Not mention two disk references are actually grabbed in case of opening disk.

Thanks,
Ming

[PATCH 4/8] block: don't grab a reference to the whole device in blkdev_get_part

[Christoph Hellwig]

blkdev_get_no_open already acquires a reference to the disk, which has
the same effect as the disk keeps a reference on the whole device bdev.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 24a6970f3623..7de519dcfdff 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1282,16 +1282,14 @@ static void blkdev_put_whole(struct block_device *bdev, fmode_t mode)
 static int blkdev_get_part(struct block_device *part, fmode_t mode)
 {
 	struct gendisk *disk = part->bd_disk;
-	struct block_device *whole;
 	int ret;
 
 	if (part->bd_openers)
 		goto done;
 
-	whole = bdgrab(disk->part0);
-	ret = blkdev_get_whole(whole, mode);
+	ret = blkdev_get_whole(bdev_whole(part), mode);
 	if (ret)
-		goto out_put_whole;
+		return ret;
 
 	ret = -ENXIO;
 	if (!bdev_nr_sectors(part))
@@ -1306,9 +1304,7 @@ static int blkdev_get_part(struct block_device *part, fmode_t mode)
 	return 0;
 
 out_blkdev_put:
-	blkdev_put_whole(whole, mode);
-out_put_whole:
-	bdput(whole);
+	blkdev_put_whole(bdev_whole(part), mode);
 	return ret;
 }
 
@@ -1321,7 +1317,6 @@ static void blkdev_put_part(struct block_device *part, fmode_t mode)
 	blkdev_flush_mapping(part);
 	whole->bd_disk->open_partitions--;
 	blkdev_put_whole(whole, mode);
-	bdput(whole);
 }
 
 struct block_device *blkdev_get_no_open(dev_t dev)
-- 
2.30.2

[PATCH 5/8] btrfs: no need to grab a reference to disk->part0

[Christoph Hellwig]

The struct block_device for the whole disk will not be freed while
the disk in use, so don't bother to grab a reference to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/btrfs/zoned.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 297c0b1c0634..21c5654967b0 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1362,20 +1362,16 @@ void btrfs_rewrite_logical_zoned(struct btrfs_ordered_extent *ordered)
 	struct extent_map_tree *em_tree;
 	struct extent_map *em;
 	struct btrfs_ordered_sum *sum;
-	struct block_device *bdev;
 	u64 orig_logical = ordered->disk_bytenr;
 	u64 *logical = NULL;
 	int nr, stripe_len;
 
 	/* Zoned devices should not have partitions. So, we can assume it is 0 */
 	ASSERT(ordered->partno == 0);
-	bdev = bdgrab(ordered->disk->part0);
-	if (WARN_ON(!bdev))
-		return;
 
-	if (WARN_ON(btrfs_rmap_block(fs_info, orig_logical, bdev,
-				     ordered->physical, &logical, &nr,
-				     &stripe_len)))
+	if (WARN_ON(btrfs_rmap_block(fs_info, orig_logical,
+			ordered->disk->part0, ordered->physical, &logical,
+			&nr, &stripe_len)))
 		goto out;
 
 	WARN_ON(nr != 1);
@@ -1402,7 +1398,6 @@ void btrfs_rewrite_logical_zoned(struct btrfs_ordered_extent *ordered)
 
 out:
 	kfree(logical);
-	bdput(bdev);
 }
 
 bool btrfs_check_meta_write_pointer(struct btrfs_fs_info *fs_info,
-- 
2.30.2

Re: [PATCH 5/8] btrfs: no need to grab a reference to disk->part0

[David Sterba]

On Wed, Jul 21, 2021 at 05:35:20PM +0200, Christoph Hellwig wrote:
> The struct block_device for the whole disk will not be freed while
> the disk in use, so don't bother to grab a reference to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Acked-by: David Sterba <dsterba@suse.com>

[PATCH 6/8] loop: don't grab a reference to the block device

[Christoph Hellwig]

The whole device block device won't be removed while the disk is still
alive, so don't bother to grab a reference to it.
---
 drivers/block/loop.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f37b9e3d833c..62c5120cf744 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1249,10 +1249,6 @@ static int loop_configure(struct loop_device *lo, fmode_t mode,
 	if (partscan)
 		lo->lo_disk->flags &= ~GENHD_FL_NO_PART_SCAN;
 
-	/* Grab the block_device to prevent its destruction after we
-	 * put /dev/loopXX inode. Later in __loop_clr_fd() we bdput(bdev).
-	 */
-	bdgrab(bdev);
 	mutex_unlock(&lo->lo_mutex);
 	if (partscan)
 		loop_reread_partitions(lo);
@@ -1332,7 +1328,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 	blk_queue_physical_block_size(lo->lo_queue, 512);
 	blk_queue_io_min(lo->lo_queue, 512);
 	if (bdev) {
-		bdput(bdev);
 		invalidate_bdev(bdev);
 		bdev->bd_inode->i_mapping->wb_err = 0;
 	}
-- 
2.30.2

[PATCH 7/8] block: remove bdgrab

[Christoph Hellwig]

All callers are gone, and no one should grab a pure inode reference to
a block device anymore.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/block_dev.c         | 15 ---------------
 include/linux/blkdev.h |  1 -
 2 files changed, 16 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 7de519dcfdff..5c56d88fd838 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -931,21 +931,6 @@ static struct block_device *bdget(dev_t dev)
 	return &BDEV_I(inode)->bdev;
 }
 
-/**
- * bdgrab -- Grab a reference to an already referenced block device
- * @bdev:	Block device to grab a reference to.
- *
- * Returns the block_device with an additional reference when successful,
- * or NULL if the inode is already beeing freed.
- */
-struct block_device *bdgrab(struct block_device *bdev)
-{
-	if (!igrab(bdev->bd_inode))
-		return NULL;
-	return bdev;
-}
-EXPORT_SYMBOL(bdgrab);
-
 long nr_blockdev_pages(void)
 {
 	struct inode *inode;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3177181c4326..98772da38bb1 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1984,7 +1984,6 @@ void blkdev_put_no_open(struct block_device *bdev);
 struct block_device *bdev_alloc(struct gendisk *disk, u8 partno);
 void bdev_add(struct block_device *bdev, dev_t dev);
 struct block_device *I_BDEV(struct inode *inode);
-struct block_device *bdgrab(struct block_device *bdev);
 void bdput(struct block_device *);
 int truncate_bdev_range(struct block_device *bdev, fmode_t mode, loff_t lstart,
 		loff_t lend);
-- 
2.30.2

[PATCH 8/8] block: remove bdget/bdput

[Christoph Hellwig]

Now that we've stopped using inode references for anything meaninful
in the block layer get rid of the helpers for it and just open code
the places that look up a block device by dev_t and drop the main
inode reference held by the device model.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/genhd.c           |  4 ++--
 block/partitions/core.c |  8 ++++----
 fs/block_dev.c          | 34 ++++++++++------------------------
 include/linux/blkdev.h  |  1 -
 4 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index e6d782714ad3..3ea27bee0615 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1081,7 +1081,7 @@ static void disk_release(struct device *dev)
 	xa_destroy(&disk->part_tbl);
 	if (test_bit(GD_QUEUE_REF, &disk->state) && disk->queue)
 		blk_put_queue(disk->queue);
-	bdput(disk->part0);
+	iput(disk->part0->bd_inode); /* actually frees the disk */
 }
 struct class block_class = {
 	.name		= "block",
@@ -1266,7 +1266,7 @@ struct gendisk *__alloc_disk_node(int minors, int node_id)
 
 out_destroy_part_tbl:
 	xa_destroy(&disk->part_tbl);
-	bdput(disk->part0);
+	iput(disk->part0->bd_inode);
 out_free_disk:
 	kfree(disk);
 	return NULL;
diff --git a/block/partitions/core.c b/block/partitions/core.c
index 9902b1635b7d..4397ac9f905e 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c
@@ -261,7 +261,7 @@ static void part_release(struct device *dev)
 {
 	if (MAJOR(dev->devt) == BLOCK_EXT_MAJOR)
 		blk_free_ext_minor(MINOR(dev->devt));
-	bdput(dev_to_bdev(dev));
+	iput(dev_to_bdev(dev)->bd_inode);
 }
 
 static int part_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -360,7 +360,7 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 		err = -ENOMEM;
 		bdev->bd_meta_info = kmemdup(info, sizeof(*info), GFP_KERNEL);
 		if (!bdev->bd_meta_info)
-			goto out_bdput;
+			goto out_iput;
 	}
 
 	pdev = &bdev->bd_device;
@@ -415,8 +415,8 @@ static struct block_device *add_partition(struct gendisk *disk, int partno,
 		kobject_uevent(&pdev->kobj, KOBJ_ADD);
 	return bdev;
 
-out_bdput:
-	bdput(bdev);
+out_iput:
+	iput(bdev->bd_inode);
 	return ERR_PTR(err);
 out_del:
 	kobject_put(bdev->bd_holder_dir);
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 5c56d88fd838..8747c264e64c 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -921,16 +921,6 @@ void bdev_add(struct block_device *bdev, dev_t dev)
 	insert_inode_hash(bdev->bd_inode);
 }
 
-static struct block_device *bdget(dev_t dev)
-{
-	struct inode *inode;
-
-	inode = ilookup(blockdev_superblock, dev);
-	if (!inode)
-		return NULL;
-	return &BDEV_I(inode)->bdev;
-}
-
 long nr_blockdev_pages(void)
 {
 	struct inode *inode;
@@ -944,12 +934,6 @@ long nr_blockdev_pages(void)
 	return ret;
 }
 
-void bdput(struct block_device *bdev)
-{
-	iput(bdev->bd_inode);
-}
-EXPORT_SYMBOL(bdput);
- 
 /**
  * bd_may_claim - test whether a block device can be claimed
  * @bdev: block device of interest
@@ -1308,18 +1292,20 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 {
 	struct block_device *bdev;
 	struct gendisk *disk;
+	struct inode *inode;
 
-	bdev = bdget(dev);
-	if (!bdev) {
+	inode = ilookup(blockdev_superblock, dev);
+	if (!inode) {
 		blk_request_module(dev);
-		bdev = bdget(dev);
-		if (!bdev)
+		inode = ilookup(blockdev_superblock, dev);
+		if (!inode)
 			return NULL;
 	}
 
+	bdev = &BDEV_I(inode)->bdev;
 	disk = bdev->bd_disk;
 	if (!kobject_get_unless_zero(&disk_to_dev(disk)->kobj))
-		goto bdput;
+		goto iput;
 	if ((disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
 		goto put_disk;
 	if (!try_module_get(disk->fops->owner))
@@ -1328,14 +1314,14 @@ struct block_device *blkdev_get_no_open(dev_t dev)
 	/* switch to a device model reference instead of the inode one: */
 	if (!kobject_get_unless_zero(&bdev->bd_device.kobj))
 		goto put_module;
-	bdput(bdev);
+	iput(bdev->bd_inode);
 	return bdev;
 put_module:
 	module_put(disk->fops->owner);
 put_disk:
 	put_disk(disk);
-bdput:
-	bdput(bdev);
+iput:
+	iput(bdev->bd_inode);
 	return NULL;
 }
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 98772da38bb1..b94de1d194b8 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1984,7 +1984,6 @@ void blkdev_put_no_open(struct block_device *bdev);
 struct block_device *bdev_alloc(struct gendisk *disk, u8 partno);
 void bdev_add(struct block_device *bdev, dev_t dev);
 struct block_device *I_BDEV(struct inode *inode);
-void bdput(struct block_device *);
 int truncate_bdev_range(struct block_device *bdev, fmode_t mode, loff_t lstart,
 		loff_t lend);
 
-- 
2.30.2

Re: fixes and cleanups for block_device refcounting

[Josef Bacik]

On 7/21/21 11:35 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series fixes up a possible race with the block_device lookup
> changes, and the finishes off the conversion to stop using the inode
> refcount for block devices.
> 

You can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

to the series, thanks,

Josef