Re: [PATCH 5/7] dm: track per-add_disk holder relations in DM

From: Yu Kuai <yukuai1@huaweicloud.com>
To: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
	Alasdair Kergon <agk@redhat.com>,
	Mike Snitzer <snitzer@kernel.org>
Cc: Yu Kuai <yukuai1@huaweicloud.com>,
	dm-devel@redhat.com, linux-block@vger.kernel.org,
	"yukuai (C)" <yukuai3@huawei.com>
Subject: Re: [PATCH 5/7] dm: track per-add_disk holder relations in DM
Date: Wed, 9 Nov 2022 10:08:14 +0800	[thread overview]
Message-ID: <9b5b4c2a-6566-2fb4-d3ae-4904f0889ea0@huaweicloud.com> (raw)
In-Reply-To: <20221030153120.1045101-6-hch@lst.de>

Hi,

在 2022/10/30 23:31, Christoph Hellwig 写道:
> dm is a bit special in that it opens the underlying devices.  Commit
> 89f871af1b26 ("dm: delay registering the gendisk") tried to accomodate
> that by allowing to add the holder to the list before add_gendisk and
> then just add them to sysfs once add_disk is called.  But that leads to
> really odd lifetime problems and error handling problems as we can't
> know the state of the kobjects and don't unwind properly.  To fix this
> switch to just registering all existing table_devices with the holder
> code right after add_disk, and remove them before calling del_gendisk.
> 
> Fixes: 89f871af1b26 ("dm: delay registering the gendisk")
> Reported-by: Yu Kuai <yukuai1@huaweicloud.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/md/dm.c | 45 +++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 2917700b1e15c..7b0d6dc957549 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -751,9 +751,16 @@ static struct table_device *open_table_device(struct mapped_device *md,
>   		goto out_free_td;
>   	}
>   
> -	r = bd_link_disk_holder(bdev, dm_disk(md));
> -	if (r)
> -		goto out_blkdev_put;
> +	/*
> +	 * We can be called before the dm disk is added.  In that case we can't
> +	 * register the holder relation here.  It will be done once add_disk was
> +	 * called.
> +	 */
> +	if (md->disk->slave_dir) {
If device_add_disk() or del_gendisk() can concurrent with this, It seems
to me that using 'slave_dir' is not safe.

I'm not quite familiar with dm, can we guarantee that they can't
concurrent?

Thanks,
Kuai
> +		r = bd_link_disk_holder(bdev, md->disk);
> +		if (r)
> +			goto out_blkdev_put;
> +	}
>   
>   	td->dm_dev.mode = mode;
>   	td->dm_dev.bdev = bdev;
> @@ -774,7 +781,8 @@ static struct table_device *open_table_device(struct mapped_device *md,
>    */
>   static void close_table_device(struct table_device *td, struct mapped_device *md)
>   {
> -	bd_unlink_disk_holder(td->dm_dev.bdev, dm_disk(md));
> +	if (md->disk->slave_dir)
> +		bd_unlink_disk_holder(td->dm_dev.bdev, md->disk);
>   	blkdev_put(td->dm_dev.bdev, td->dm_dev.mode | FMODE_EXCL);
>   	put_dax(td->dm_dev.dax_dev);
>   	list_del(&td->list);
> @@ -1951,7 +1959,13 @@ static void cleanup_mapped_device(struct mapped_device *md)
>   		md->disk->private_data = NULL;
>   		spin_unlock(&_minor_lock);
>   		if (dm_get_md_type(md) != DM_TYPE_NONE) {
> +			struct table_device *td;
> +
>   			dm_sysfs_exit(md);
> +			list_for_each_entry(td, &md->table_devices, list) {
> +				bd_unlink_disk_holder(td->dm_dev.bdev,
> +						      md->disk);
> +			}
>   			del_gendisk(md->disk);
>   		}
>   		dm_queue_destroy_crypto_profile(md->queue);
> @@ -2284,6 +2298,7 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>   {
>   	enum dm_queue_mode type = dm_table_get_type(t);
>   	struct queue_limits limits;
> +	struct table_device *td;
>   	int r;
>   
>   	switch (type) {
> @@ -2316,13 +2331,27 @@ int dm_setup_md_queue(struct mapped_device *md, struct dm_table *t)
>   	if (r)
>   		return r;
>   
> -	r = dm_sysfs_init(md);
> -	if (r) {
> -		del_gendisk(md->disk);
> -		return r;
> +	/*
> +	 * Register the holder relationship for devices added before the disk
> +	 * was live.
> +	 */
> +	list_for_each_entry(td, &md->table_devices, list) {
> +		r = bd_link_disk_holder(td->dm_dev.bdev, md->disk);
> +		if (r)
> +			goto out_undo_holders;
>   	}
> +
> +	r = dm_sysfs_init(md);
> +	if (r)
> +		goto out_undo_holders;
>   	md->type = type;
>   	return 0;
> +
> +out_undo_holders:
> +	list_for_each_entry_continue_reverse(td, &md->table_devices, list)
> +		bd_unlink_disk_holder(td->dm_dev.bdev, md->disk);
> +	del_gendisk(md->disk);
> +	return r;
>   }
>   
>   struct mapped_device *dm_get_md(dev_t dev)
> 