Re: [PATCH 2/5] block: WARN_ON_ONCE() when we remove active partitions

[Jan Kara <jack@suse.cz>]

On Tue 17-10-23 20:48:20, Christoph Hellwig wrote:
> From: Christian Brauner <brauner@kernel.org>
> 
> The logic for disk->open_partitions is:
> 
> blkdev_get_by_*()
> -> bdev_is_partition()
>    -> blkdev_get_part()
>       -> blkdev_get_whole() // bdev_whole->bd_openers++
>       -> if (part->bd_openers == 0)
>                  disk->open_partitions++
>          part->bd_openers
> 
> In other words, when we first claim/open a partition we increment
> disk->open_partitions and only when all part->bd_openers are closed will
> disk->open_partitions be zero. That should mean that
> disk->open_partitions is always > 0 as long as there's anyone that
> has an open partition.
> 
> So the check for disk->open_partitions should meand that we can never
> remove an active partition that has a holder and holder ops set. Assert
> that in the code. The main disk isn't removed so that check doesn't work
> for disk->part0 which is what we want. After all we only care about
> partition not about the main disk.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/partitions/core.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/block/partitions/core.c b/block/partitions/core.c
> index b0585536b407a5..f47ffcfdfcec22 100644
> --- a/block/partitions/core.c
> +++ b/block/partitions/core.c
> @@ -274,17 +274,6 @@ void drop_partition(struct block_device *part)
>  	put_device(&part->bd_device);
>  }
>  
> -static void delete_partition(struct block_device *part)
> -{
> -	/*
> -	 * Remove the block device from the inode hash, so that it cannot be
> -	 * looked up any more even when openers still hold references.
> -	 */
> -	remove_inode_hash(part->bd_inode);
> -	bdev_mark_dead(part, false);
> -	drop_partition(part);
> -}
> -
>  static ssize_t whole_disk_show(struct device *dev,
>  			       struct device_attribute *attr, char *buf)
>  {
> @@ -674,8 +663,23 @@ int bdev_disk_changed(struct gendisk *disk, bool invalidate)
>  	sync_blockdev(disk->part0);
>  	invalidate_bdev(disk->part0);
>  
> -	xa_for_each_start(&disk->part_tbl, idx, part, 1)
> -		delete_partition(part);
> +	xa_for_each_start(&disk->part_tbl, idx, part, 1) {
> +		/*
> +		 * Remove the block device from the inode hash, so that
> +		 * it cannot be looked up any more even when openers
> +		 * still hold references.
> +		 */
> +		remove_inode_hash(part->bd_inode);
> +
> +		/*
> +		 * If @disk->open_partitions isn't elevated but there's
> +		 * still an active holder of that block device things
> +		 * are broken.
> +		 */
> +		WARN_ON_ONCE(atomic_read(&part->bd_openers));
> +		invalidate_bdev(part);
> +		drop_partition(part);
> +	}
>  	clear_bit(GD_NEED_PART_SCAN, &disk->state);
>  
>  	/*
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR