Re: [PATCH v6 4/4] dm: add DM_INTERPOSED_FLAG

[Christoph Hellwig]

On Wed, Mar 03, 2021 at 03:30:18PM +0300, Sergei Shtepa wrote:
> DM_INTERPOSED_FLAG allow to create dm targets on "the fly".
> Underlying block device opens without a flag FMODE_EXCL.
> Dm target receives bio from the original device via
> blk_interposer.
> 
> Signed-off-by: Sergei Shtepa <sergei.shtepa@xxxxxxxxx>
> ---
>  drivers/md/dm-core.h          |   6 ++
>  drivers/md/dm-ioctl.c         |   9 +++
>  drivers/md/dm-table.c         | 115 +++++++++++++++++++++++++++++++---
>  drivers/md/dm.c               |  38 +++++++----
>  include/linux/device-mapper.h |   1 +
>  include/uapi/linux/dm-ioctl.h |   6 ++
>  6 files changed, 154 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/md/dm-core.h b/drivers/md/dm-core.h
> index 5953ff2bd260..e5c845f9b1df 100644
> --- a/drivers/md/dm-core.h
> +++ b/drivers/md/dm-core.h
> @@ -21,6 +21,8 @@
>  
>  #define DM_RESERVED_MAX_IOS		1024
>  
> +struct dm_interposed_dev;
> +
>  struct dm_kobject_holder {
>  	struct kobject kobj;
>  	struct completion completion;
> @@ -114,6 +116,10 @@ struct mapped_device {
>  	bool init_tio_pdu:1;
>  
>  	struct srcu_struct io_barrier;
> +
> +	/* for interposers logic */
> +	bool is_interposed;
> +	struct dm_interposed_dev *ip_dev;
>  };
>  
>  void disable_discard(struct mapped_device *md);
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index 5e306bba4375..2bcb316144a1 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1267,6 +1267,11 @@ static inline fmode_t get_mode(struct dm_ioctl *param)
>  	return mode;
>  }
>  
> +static inline bool get_interposer_flag(struct dm_ioctl *param)
> +{
> +	return (param->flags & DM_INTERPOSED_FLAG);
> +}
> +
>  static int next_target(struct dm_target_spec *last, uint32_t next, void *end,
>  		       struct dm_target_spec **spec, char **target_params)
>  {
> @@ -1338,6 +1343,8 @@ static int table_load(struct file *filp, struct dm_ioctl *param, size_t param_si
>  	if (!md)
>  		return -ENXIO;
>  
> +	md->is_interposed = get_interposer_flag(param);
> +
>  	r = dm_table_create(&t, get_mode(param), param->target_count, md);
>  	if (r)
>  		goto err;
> @@ -2098,6 +2105,8 @@ int __init dm_early_create(struct dm_ioctl *dmi,
>  	if (r)
>  		goto err_hash_remove;
>  
> +	md->is_interposed = get_interposer_flag(dmi);
> +
>  	/* add targets */
>  	for (i = 0; i < dmi->target_count; i++) {
>  		r = dm_table_add_target(t, spec_array[i]->target_type,
> diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> index 95391f78b8d5..0b2f9b66ade5 100644
> --- a/drivers/md/dm-table.c
> +++ b/drivers/md/dm-table.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "dm-core.h"
> +#include "dm-interposer.h"
>  
>  #include <linux/module.h>
>  #include <linux/vmalloc.h>
> @@ -225,12 +226,13 @@ void dm_table_destroy(struct dm_table *t)
>  /*
>   * See if we've already got a device in the list.
>   */
> -static struct dm_dev_internal *find_device(struct list_head *l, dev_t dev)
> +static struct dm_dev_internal *find_device(struct list_head *l, dev_t dev, bool is_interposed)
>  {
>  	struct dm_dev_internal *dd;
>  
>  	list_for_each_entry (dd, l, list)
> -		if (dd->dm_dev->bdev->bd_dev == dev)
> +		if ((dd->dm_dev->bdev->bd_dev == dev) &&
> +		    (dd->dm_dev->is_interposed == is_interposed))
>  			return dd;
>  
>  	return NULL;
> @@ -358,6 +360,90 @@ dev_t dm_get_dev_t(const char *path)
>  }
>  EXPORT_SYMBOL_GPL(dm_get_dev_t);
>  
> +/*
> + * Redirect bio from interposed device to dm device
> + */
> +static void dm_interpose_fn(struct dm_interposed_dev *ip_dev, struct bio *bio)
> +{
> +	struct mapped_device *md = ip_dev->private;
> +
> +	if (bio_flagged(bio, BIO_REMAPPED)) {
> +		/*
> +		 * Since bio has already been remapped, we need to subtract
> +		 * the block device offset from the beginning of the disk.
> +		 */
> +		bio->bi_iter.bi_sector -= get_start_sect(bio->bi_bdev);
> +
> +		bio_clear_flag(bio, BIO_REMAPPED);
> +	}

So instead of doing this shoudn't the imposer just always submit to the
whole device?  But if we keep it, the logic in this funtion should go
into a block layer helper, passing a block device instead of the
dm_interposed_dev.  This avoids having such fragile logic in drivers.

> +	if ((ofs + len) > bdev_nr_sectors(bdev)) {
> +		DMERR("The specified range of sectors exceeds of the size of the block device.");
> +		return -ERANGE;
> +	}
> +
> +	md->ip_dev = kzalloc(sizeof(struct dm_interposed_dev), GFP_KERNEL);
> +	if (!md->ip_dev)
> +		return -ENOMEM;
> +
> +	if ((ofs == 0) && (len == 0))

Lots of superflous inner braces.