[PATCH 0/3] block: blk_interposer - Block Layer Interposer

[PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Sergei Shtepa]

Hi all.

I try to suggest the Block Layer Interposer (blk_interposer) again.
It`s allows to intercept bio requests, remap bio to another devices
or add new bios.

Initially, blk_interposer was designed to be compatible with
device mapper. Our (my and Hannes) previous attempt to offer
blk_interposer integrated with device mapper did not receive
any comments from the dm-devel team, and without their help
I will not be able to make a full implementation. I hope later
they will have time to support blk_interposer in device mapper.

And my blk-snap module requires an architecture change to
support blk_interposer.

This time I offer it along with a sample.
Despite the fact that blk_interposer is quite simple,
there are a few non-obvious points that I would like to clarify.

However, I suggest the blk_interposer now so that people
could discuss it and use it in their projects as soon as possible.

Sergei Shtepa (3):
  block: blk_interposer - Block Layer Interposer
  block: blk_interposer - sample
  block: blk_interposer - sample config

 block/blk-core.c                        |  32 +++
 include/linux/blk_types.h               |   6 +-
 include/linux/genhd.h                   |  12 +-
 samples/Kconfig                         |   7 +
 samples/Makefile                        |   1 +
 samples/blk_interposer/Makefile         |   2 +
 samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
 7 files changed, 333 insertions(+), 3 deletions(-)
 create mode 100644 samples/blk_interposer/Makefile
 create mode 100644 samples/blk_interposer/blk-interposer.c

--
2.20.1

[PATCH 1/3] block: blk_interposer - Block Layer Interposer

[Sergei Shtepa]

The Block Layer Interposer (blk_interposer) allows to intercept bio.
This allows to connect device mapper and other kernel modules to
the block device stack on the fly.

This simple hook allows to work modules that previously relied on the
interception of the make_request_fn() function from the
request_queue structure. This interception capability was lost in
the v5.8.

Changes:
  * A new __submit_bio_interposed() function call is added
    to the submit_bio_noacct(). The call is made only if the
    interposer was connected.
  * A new bio flag BIO_INTERPOSED can indicate that bio has
    already been interposed or cannot be interposed.
  * The bi_flags member of the BIO structure is expanded
    to a 32-bit variable to accommodate the new flag.
  * New structure blk_interposer contains the interposer
    callback function.
Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
---
 block/blk-core.c          | 32 ++++++++++++++++++++++++++++++++
 include/linux/blk_types.h |  6 ++++--
 include/linux/genhd.h     | 12 +++++++++++-
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2db8bda43b6e..682b9b5a93ff 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1030,6 +1030,35 @@ static blk_qc_t __submit_bio_noacct_mq(struct bio *bio)
 	return ret;
 }
 
+static blk_qc_t __submit_bio_interposed(struct bio *bio)
+{
+	struct bio_list bio_list[2] = { };
+	blk_qc_t ret = BLK_QC_T_NONE;
+
+	current->bio_list = bio_list;
+	if (likely(bio_queue_enter(bio) == 0)) {
+		struct gendisk *disk = bio->bi_disk;
+
+		bio_set_flag(bio, BIO_INTERPOSED);
+		if (likely(blk_has_interposer(disk))) {
+			struct blk_interposer *ip = disk->interposer;
+
+			ip->ip_submit_bio(ip, bio);
+		} else
+			/* interposer was removed */
+			bio_list_add(&current->bio_list[0], bio);
+
+		blk_queue_exit(disk->queue);
+	}
+	current->bio_list = NULL;
+
+	/* Resubmit the remaining bios */
+	while ((bio = bio_list_pop(&bio_list[0])))
+		ret = submit_bio_noacct(bio);
+
+	return ret;
+}
+
 /**
  * submit_bio_noacct - re-submit a bio to the block device layer for I/O
  * @bio:  The bio describing the location in memory and on the device.
@@ -1055,6 +1084,9 @@ blk_qc_t submit_bio_noacct(struct bio *bio)
 		return BLK_QC_T_NONE;
 	}
 
+	if (blk_has_interposer(bio->bi_disk) &&
+	    !bio_flagged(bio, BIO_INTERPOSED))
+		return __submit_bio_interposed(bio);
 	if (!bio->bi_disk->fops->submit_bio)
 		return __submit_bio_noacct_mq(bio);
 	return __submit_bio_noacct(bio);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d9b69bbde5cc..996b803e5aa1 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -207,7 +207,7 @@ struct bio {
 						 * top bits REQ_OP. Use
 						 * accessors.
 						 */
-	unsigned short		bi_flags;	/* status, etc and bvec pool number */
+	unsigned int		bi_flags;	/* status, etc and bvec pool number */
 	unsigned short		bi_ioprio;
 	unsigned short		bi_write_hint;
 	blk_status_t		bi_status;
@@ -284,6 +284,8 @@ enum {
 				 * of this bio. */
 	BIO_CGROUP_ACCT,	/* has been accounted to a cgroup */
 	BIO_TRACKED,		/* set if bio goes through the rq_qos path */
+	BIO_INTERPOSED,		/* bio has been interposed and can be moved to
+				 * a different disk */
 	BIO_FLAG_LAST
 };
 
@@ -302,7 +304,7 @@ enum {
  * freed.
  */
 #define BVEC_POOL_BITS		(3)
-#define BVEC_POOL_OFFSET	(16 - BVEC_POOL_BITS)
+#define BVEC_POOL_OFFSET	(32 - BVEC_POOL_BITS)
 #define BVEC_POOL_IDX(bio)	((bio)->bi_flags >> BVEC_POOL_OFFSET)
 #if (1<< BVEC_POOL_BITS) < (BVEC_POOL_NR+1)
 # error "BVEC_POOL_BITS is too small"
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 03da3f603d30..ea5fc57c3451 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -4,7 +4,7 @@
 
 /*
  * 	genhd.h Copyright (C) 1992 Drew Eckhardt
- *	Generic hard disk header file by  
+ *	Generic hard disk header file by
  * 		Drew Eckhardt
  *
  *		<drew@colorado.edu>
@@ -164,6 +164,13 @@ struct blk_integrity {
 	unsigned char				tag_size;
 };
 
+struct blk_interposer;
+typedef void (*ip_submit_bio_t) (struct blk_interposer *ip, struct bio *bio);
+
+struct blk_interposer {
+	ip_submit_bio_t ip_submit_bio;
+};
+
 struct gendisk {
 	/* major, first_minor and minors are input parameters only,
 	 * don't use directly.  Use disk_devt() and disk_max_parts().
@@ -188,6 +195,7 @@ struct gendisk {
 
 	const struct block_device_operations *fops;
 	struct request_queue *queue;
+	struct blk_interposer *interposer;
 	void *private_data;
 
 	int flags;
@@ -409,4 +417,6 @@ static inline dev_t blk_lookup_devt(const char *name, int partno)
 }
 #endif /* CONFIG_BLOCK */
 
+#define blk_has_interposer(d) ((d)->interposer != NULL)
+
 #endif /* _LINUX_GENHD_H */
-- 
2.20.1

[PATCH 2/3] block: blk_interposer - sample

[Sergei Shtepa]

This sample demonstrates how to use blk_interposer.
It show how to properly connect the interposer module to kernel,
and perform the simplest monitoring of the number of bio.

Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
---
 samples/blk_interposer/Makefile         |   2 +
 samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
 2 files changed, 278 insertions(+)
 create mode 100644 samples/blk_interposer/Makefile
 create mode 100644 samples/blk_interposer/blk-interposer.c

diff --git a/samples/blk_interposer/Makefile b/samples/blk_interposer/Makefile
new file mode 100644
index 000000000000..b11aefde2b1c
--- /dev/null
+++ b/samples/blk_interposer/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_SAMPLE_BLK_INTERPOSER) += blk-interposer.o
diff --git a/samples/blk_interposer/blk-interposer.c b/samples/blk_interposer/blk-interposer.c
new file mode 100644
index 000000000000..92b4c1fcf8f7
--- /dev/null
+++ b/samples/blk_interposer/blk-interposer.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Block layer interposer allow to interpose bio requests from kernel module.
+ * This allows you to monitor requests, modify requests, add new request,
+ * or even redirect requests to another devices.
+ *
+ * This sample demonstrates how to use blk_interposer.
+ * It show how to properly connect the interposer module to kernel,
+ * and perform the simplest monitoring of the number of bio.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/blkdev.h>
+#include <linux/genhd.h>
+#include <linux/blk-mq.h>
+
+int device_major = 8;
+int device_minor;
+int fmode = FMODE_READ | FMODE_WRITE;
+
+/*
+ * Each interposer must have a common part in the form of the blk_interposer structure,
+ * as well as its own unique data.
+ */
+struct my_interposer {
+	/*
+	 * Common part of block device interposer.
+	 */
+	struct blk_interposer interposer;
+	/*
+	 * Specific part for our interposer data.
+	 */
+	atomic_t counter;
+};
+
+struct my_interposer my_ip;
+
+/**
+ * blk_interposer_attach - Attach interposer to disk
+ * @disk: target disk
+ * @interposer: block device interposer
+ */
+static int blk_interposer_attach(struct gendisk *disk, struct blk_interposer *interposer)
+{
+	int ret = 0;
+
+	/*
+	 * Stop disks queue processing.
+	 */
+	blk_mq_freeze_queue(disk->queue);
+	blk_mq_quiesce_queue(disk->queue);
+
+	/*
+	 * Check if the interposer is already busy.
+	 * The interposer will only connect if it is not busy.
+	 */
+	if (blk_has_interposer(disk)) {
+		pr_info("The interposer is already busy.\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/*
+	 * Attach the interposer.
+	 */
+	disk->interposer = interposer;
+	/*
+	 * And while the queue is stopped, we can do something specific for our module.
+	 */
+	pr_info("Block device interposer attached successfully.\n");
+
+out:
+	/*
+	 * Resume disks queue processing
+	 */
+	blk_mq_unquiesce_queue(disk->queue);
+	blk_mq_unfreeze_queue(disk->queue);
+
+	return ret;
+}
+
+/**
+ * blk_interposer_detach - Detach interposer from disk
+ * @disk: target disk
+ * @interposer: block device interposer
+ */
+static int blk_interposer_detach(struct gendisk *disk, struct blk_interposer *interposer)
+{
+	int ret = 0;
+
+	if (WARN_ON(!disk))
+		return -EINVAL;
+
+	/*
+	 * Stop disks queue processing.
+	 */
+	blk_mq_freeze_queue(disk->queue);
+	blk_mq_quiesce_queue(disk->queue);
+
+	/*
+	 * Check if the interposer is still available.
+	 */
+	if (!disk->interposer) {
+		pr_info("The interposer is not available.\n");
+		return -ENOENT;
+		goto out;
+	}
+	/*
+	 * Check if it is really our interposer.
+	 */
+	if (disk->interposer->ip_submit_bio != interposer->ip_submit_bio) {
+		pr_info("The interposer found is not ours.\n");
+		return -EPERM;
+		goto out;
+	}
+
+	/*
+	 * Detach interposer.
+	 */
+	disk->interposer = NULL;
+	/*
+	 * And while the queue is stopped, we can do something specific for our module.
+	 */
+	pr_info("Block device interposer detached successfully.\n");
+
+out:
+	/*
+	 * Resume disks queue processing.
+	 */
+	blk_mq_unquiesce_queue(disk->queue);
+	blk_mq_unfreeze_queue(disk->queue);
+
+	return ret;
+}
+
+/**
+ * blk_interposer_submit_bio - Interpose bio
+ * @interposer: block device interposer
+ * @bio: interposed bio
+ */
+static void blk_interposer_submit_bio(struct blk_interposer *interposer, struct bio *bio)
+{
+	struct my_interposer *ip = container_of(interposer, struct my_interposer, interposer);
+
+	atomic_inc(&ip->counter);
+	if (atomic_read(&ip->counter) < 10)
+		pr_info("%s - %d", __func__, atomic_read(&ip->counter));
+	else if (atomic_read(&ip->counter) == 10)
+		pr_info("%s the flooding of log has stopped", __func__);
+
+	/*
+	 * Here we can submit a new child bio requests before the original.
+	 * If the new bio should not be interposed, set BIO_INTERPOSED flag.
+	 */
+
+	/*
+	 * We can resubmit the original bio or just add it to the current->bio_list.
+	 * In the second case, we do not call submit_bio_checks() twice.
+	 */
+	bio_list_add(&current->bio_list[0], bio);
+
+	/*
+	 * Here we can submit a new bio requests after the original,
+	 * although I don't know how it might be useful.
+	 */
+
+	/*
+	 * All submitted bio will be just placed in the current->bio_list.
+	 * We can't wait for the bio request to complete.
+	 */
+
+	/*
+	 * The function doesn't return anything, because it can only return BLK_QC_T_NONE.
+	 */
+}
+
+static int __init blk_interposer_init(void)
+{
+	int ret = 0;
+	dev_t dev_id;
+	struct block_device *bdev;
+
+	pr_info("Loading module\n");
+
+	pr_info("Tracking device: %d:%d\n", device_major, device_minor);
+	dev_id = MKDEV(device_major, device_minor);
+
+	/*
+	 * Open block device
+	 */
+	bdev = blkdev_get_by_dev(dev_id, fmode, &my_ip);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		pr_err("Unable to open device [%d:%d]: blkdev_get_by_dev return %d\n",
+		       MAJOR(dev_id), MINOR(dev_id), 0-ret);
+		return ret;
+	}
+
+	/*
+	 * Initialize block device interposer fields.
+	 */
+	my_ip.interposer.ip_submit_bio = blk_interposer_submit_bio;
+
+	/*
+	 * Initialize our interposer specific fields
+	 */
+	atomic_set(&my_ip.counter, 0);
+
+	/*
+	 * Attach interposer to disk
+	 */
+	ret = blk_interposer_attach(bdev->bd_disk, &my_ip.interposer);
+	if (ret)
+		pr_info("Failed to attach the block device interposer.\n");
+	/*
+	 * Close device
+	 */
+	blkdev_put(bdev, fmode);
+
+	return ret;
+}
+
+static void __exit blk_interposer_exit(void)
+{
+	int ret;
+	dev_t dev_id;
+	struct block_device *bdev;
+
+	pr_info("Unloading module\n");
+
+	dev_id = MKDEV(device_major, device_minor);
+
+	/*
+	 * Open block device
+	 */
+	bdev = blkdev_get_by_dev(dev_id, fmode, &my_ip);
+	if (IS_ERR(bdev)) {
+		ret = PTR_ERR(bdev);
+		pr_err("Unable to open device [%d:%d]: blkdev_get_by_dev return %d\n",
+		       MAJOR(dev_id), MINOR(dev_id), 0-ret);
+		return;
+	}
+
+	/*
+	 * Detach interposer from disk
+	 */
+	ret = blk_interposer_detach(bdev->bd_disk, &my_ip.interposer);
+	if (ret)
+		pr_info("Failed to detach the block device interposer.\n");
+	/*
+	 * Close device
+	 */
+	blkdev_put(bdev, fmode);
+
+	/*
+	 * The counter shows how much bio was interposed.
+	 */
+	pr_info("%d bio was interposed.\n", atomic_read(&my_ip.counter));
+}
+
+module_init(blk_interposer_init);
+module_exit(blk_interposer_exit);
+
+MODULE_DESCRIPTION("Block Layer Interposer Sample kernel module");
+MODULE_AUTHOR("Veeam Software Group GmbH");
+MODULE_LICENSE("GPL");
+
+module_param_named(device_major, device_major, int, 0644);
+MODULE_PARM_DESC(device_major, "Tracking device major number");
+module_param_named(device_minor, device_minor, int, 0644);
+MODULE_PARM_DESC(device_minor, "Tracking device minor number");
-- 
2.20.1

Re: [PATCH 2/3] block: blk_interposer - sample

[Mike Snitzer]

On Wed, Dec 09 2020 at  8:01am -0500,
Sergei Shtepa <sergei.shtepa@veeam.com> wrote:

> This sample demonstrates how to use blk_interposer.
> It show how to properly connect the interposer module to kernel,
> and perform the simplest monitoring of the number of bio.
> 
> Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
> ---
>  samples/blk_interposer/Makefile         |   2 +
>  samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
>  2 files changed, 278 insertions(+)
>  create mode 100644 samples/blk_interposer/Makefile
>  create mode 100644 samples/blk_interposer/blk-interposer.c
> 
> diff --git a/samples/blk_interposer/Makefile b/samples/blk_interposer/Makefile
> new file mode 100644
> index 000000000000..b11aefde2b1c
> --- /dev/null
> +++ b/samples/blk_interposer/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_SAMPLE_BLK_INTERPOSER) += blk-interposer.o
> diff --git a/samples/blk_interposer/blk-interposer.c b/samples/blk_interposer/blk-interposer.c
> new file mode 100644
> index 000000000000..92b4c1fcf8f7
> --- /dev/null
> +++ b/samples/blk_interposer/blk-interposer.c
> @@ -0,0 +1,276 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Block layer interposer allow to interpose bio requests from kernel module.
> + * This allows you to monitor requests, modify requests, add new request,
> + * or even redirect requests to another devices.
> + *
> + * This sample demonstrates how to use blk_interposer.
> + * It show how to properly connect the interposer module to kernel,
> + * and perform the simplest monitoring of the number of bio.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/blkdev.h>
> +#include <linux/genhd.h>
> +#include <linux/blk-mq.h>
> +
> +int device_major = 8;
> +int device_minor;
> +int fmode = FMODE_READ | FMODE_WRITE;
> +
> +/*
> + * Each interposer must have a common part in the form of the blk_interposer structure,
> + * as well as its own unique data.
> + */
> +struct my_interposer {
> +	/*
> +	 * Common part of block device interposer.
> +	 */
> +	struct blk_interposer interposer;
> +	/*
> +	 * Specific part for our interposer data.
> +	 */
> +	atomic_t counter;
> +};
> +
> +struct my_interposer my_ip;
> +
> +/**
> + * blk_interposer_attach - Attach interposer to disk
> + * @disk: target disk
> + * @interposer: block device interposer
> + */
> +static int blk_interposer_attach(struct gendisk *disk, struct blk_interposer *interposer)
> +{
> +	int ret = 0;
> +
> +	/*
> +	 * Stop disks queue processing.
> +	 */
> +	blk_mq_freeze_queue(disk->queue);
> +	blk_mq_quiesce_queue(disk->queue);
> +
> +	/*
> +	 * Check if the interposer is already busy.
> +	 * The interposer will only connect if it is not busy.
> +	 */
> +	if (blk_has_interposer(disk)) {
> +		pr_info("The interposer is already busy.\n");
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Attach the interposer.
> +	 */
> +	disk->interposer = interposer;
> +	/*
> +	 * And while the queue is stopped, we can do something specific for our module.
> +	 */
> +	pr_info("Block device interposer attached successfully.\n");
> +
> +out:
> +	/*
> +	 * Resume disks queue processing
> +	 */
> +	blk_mq_unquiesce_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue);
> +
> +	return ret;
> +}
> +
> +/**
> + * blk_interposer_detach - Detach interposer from disk
> + * @disk: target disk
> + * @interposer: block device interposer
> + */
> +static int blk_interposer_detach(struct gendisk *disk, struct blk_interposer *interposer)
> +{
> +	int ret = 0;
> +
> +	if (WARN_ON(!disk))
> +		return -EINVAL;
> +
> +	/*
> +	 * Stop disks queue processing.
> +	 */
> +	blk_mq_freeze_queue(disk->queue);
> +	blk_mq_quiesce_queue(disk->queue);
> +
> +	/*
> +	 * Check if the interposer is still available.
> +	 */
> +	if (!disk->interposer) {
> +		pr_info("The interposer is not available.\n");
> +		return -ENOENT;
> +		goto out;
> +	}
> +	/*
> +	 * Check if it is really our interposer.
> +	 */
> +	if (disk->interposer->ip_submit_bio != interposer->ip_submit_bio) {
> +		pr_info("The interposer found is not ours.\n");
> +		return -EPERM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Detach interposer.
> +	 */
> +	disk->interposer = NULL;
> +	/*
> +	 * And while the queue is stopped, we can do something specific for our module.
> +	 */
> +	pr_info("Block device interposer detached successfully.\n");
> +
> +out:
> +	/*
> +	 * Resume disks queue processing.
> +	 */
> +	blk_mq_unquiesce_queue(disk->queue);
> +	blk_mq_unfreeze_queue(disk->queue);
> +
> +	return ret;
> +}

This attach and detach code needs to be elevated out of samples so that
any future consumer of blk_interposer doesn't reinvent it.  It is far
too fundamental.

The way you've proposed this be merged is very much unacceptable.

Nacked-by: Mike Snitzer <snitzer@redhat.com>

Re: [PATCH 2/3] block: blk_interposer - sample

[Sergei Shtepa]

The 12/09/2020 17:36, Mike Snitzer wrote:
> On Wed, Dec 09 2020 at  8:01am -0500,
> Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> 
> > This sample demonstrates how to use blk_interposer.
> > It show how to properly connect the interposer module to kernel,
> > and perform the simplest monitoring of the number of bio.
> > 
> > Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
> > ---
> >  samples/blk_interposer/Makefile         |   2 +
> >  samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
> >  2 files changed, 278 insertions(+)
> >  create mode 100644 samples/blk_interposer/Makefile
> >  create mode 100644 samples/blk_interposer/blk-interposer.c
> > 
> > diff --git a/samples/blk_interposer/Makefile b/samples/blk_interposer/Makefile
> > new file mode 100644
> > index 000000000000..b11aefde2b1c
> > --- /dev/null
> > +++ b/samples/blk_interposer/Makefile
> > @@ -0,0 +1,2 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +obj-$(CONFIG_SAMPLE_BLK_INTERPOSER) += blk-interposer.o
> > diff --git a/samples/blk_interposer/blk-interposer.c b/samples/blk_interposer/blk-interposer.c
> > new file mode 100644
> > index 000000000000..92b4c1fcf8f7
> > --- /dev/null
> > +++ b/samples/blk_interposer/blk-interposer.c
> > @@ -0,0 +1,276 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Block layer interposer allow to interpose bio requests from kernel module.
> > + * This allows you to monitor requests, modify requests, add new request,
> > + * or even redirect requests to another devices.
> > + *
> > + * This sample demonstrates how to use blk_interposer.
> > + * It show how to properly connect the interposer module to kernel,
> > + * and perform the simplest monitoring of the number of bio.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/errno.h>
> > +#include <linux/blkdev.h>
> > +#include <linux/genhd.h>
> > +#include <linux/blk-mq.h>
> > +
> > +int device_major = 8;
> > +int device_minor;
> > +int fmode = FMODE_READ | FMODE_WRITE;
> > +
> > +/*
> > + * Each interposer must have a common part in the form of the blk_interposer structure,
> > + * as well as its own unique data.
> > + */
> > +struct my_interposer {
> > +	/*
> > +	 * Common part of block device interposer.
> > +	 */
> > +	struct blk_interposer interposer;
> > +	/*
> > +	 * Specific part for our interposer data.
> > +	 */
> > +	atomic_t counter;
> > +};
> > +
> > +struct my_interposer my_ip;
> > +
> > +/**
> > + * blk_interposer_attach - Attach interposer to disk
> > + * @disk: target disk
> > + * @interposer: block device interposer
> > + */
> > +static int blk_interposer_attach(struct gendisk *disk, struct blk_interposer *interposer)
> > +{
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * Stop disks queue processing.
> > +	 */
> > +	blk_mq_freeze_queue(disk->queue);
> > +	blk_mq_quiesce_queue(disk->queue);
> > +
> > +	/*
> > +	 * Check if the interposer is already busy.
> > +	 * The interposer will only connect if it is not busy.
> > +	 */
> > +	if (blk_has_interposer(disk)) {
> > +		pr_info("The interposer is already busy.\n");
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * Attach the interposer.
> > +	 */
> > +	disk->interposer = interposer;
> > +	/*
> > +	 * And while the queue is stopped, we can do something specific for our module.
> > +	 */
> > +	pr_info("Block device interposer attached successfully.\n");
> > +
> > +out:
> > +	/*
> > +	 * Resume disks queue processing
> > +	 */
> > +	blk_mq_unquiesce_queue(disk->queue);
> > +	blk_mq_unfreeze_queue(disk->queue);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * blk_interposer_detach - Detach interposer from disk
> > + * @disk: target disk
> > + * @interposer: block device interposer
> > + */
> > +static int blk_interposer_detach(struct gendisk *disk, struct blk_interposer *interposer)
> > +{
> > +	int ret = 0;
> > +
> > +	if (WARN_ON(!disk))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Stop disks queue processing.
> > +	 */
> > +	blk_mq_freeze_queue(disk->queue);
> > +	blk_mq_quiesce_queue(disk->queue);
> > +
> > +	/*
> > +	 * Check if the interposer is still available.
> > +	 */
> > +	if (!disk->interposer) {
> > +		pr_info("The interposer is not available.\n");
> > +		return -ENOENT;
> > +		goto out;
> > +	}
> > +	/*
> > +	 * Check if it is really our interposer.
> > +	 */
> > +	if (disk->interposer->ip_submit_bio != interposer->ip_submit_bio) {
> > +		pr_info("The interposer found is not ours.\n");
> > +		return -EPERM;
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * Detach interposer.
> > +	 */
> > +	disk->interposer = NULL;
> > +	/*
> > +	 * And while the queue is stopped, we can do something specific for our module.
> > +	 */
> > +	pr_info("Block device interposer detached successfully.\n");
> > +
> > +out:
> > +	/*
> > +	 * Resume disks queue processing.
> > +	 */
> > +	blk_mq_unquiesce_queue(disk->queue);
> > +	blk_mq_unfreeze_queue(disk->queue);
> > +
> > +	return ret;
> > +}
> 
> This attach and detach code needs to be elevated out of samples so that
> any future consumer of blk_interposer doesn't reinvent it.  It is far
> too fundamental.
> 
> The way you've proposed this be merged is very much unacceptable.
> 
> Nacked-by: Mike Snitzer <snitzer@redhat.com>
> 
Yes, but on the other hand, while the queue is suspended, the module can perform
some other actions specific to it.

And since the functions blk_mq_freeze_queue(), blk_mq_quiesce_queue(),
blk_mq_unquiesce_queue() and blk_mq_unfreeze_queue() are public,
the module creator can implement its module connection functionality regardless of
whether we make the functions blk_interposer_attach() and blk_interposer_detach()
in the kernel or not.

I'll think about it and try to come up with a better solution.
-- 
Sergei Shtepa
Veeam Software developer.

Re: [PATCH 2/3] block: blk_interposer - sample

[Mike Snitzer]

On Thu, Dec 10 2020 at 10:54am -0500,
Sergei Shtepa <sergei.shtepa@veeam.com> wrote:

> The 12/09/2020 17:36, Mike Snitzer wrote:
> > On Wed, Dec 09 2020 at  8:01am -0500,
> > Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> > 
> > > This sample demonstrates how to use blk_interposer.
> > > It show how to properly connect the interposer module to kernel,
> > > and perform the simplest monitoring of the number of bio.
> > > 
> > > Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
> > > ---
> > >  samples/blk_interposer/Makefile         |   2 +
> > >  samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
> > >  2 files changed, 278 insertions(+)
> > >  create mode 100644 samples/blk_interposer/Makefile
> > >  create mode 100644 samples/blk_interposer/blk-interposer.c
> > > 
> > > diff --git a/samples/blk_interposer/Makefile b/samples/blk_interposer/Makefile
> > > new file mode 100644
> > > index 000000000000..b11aefde2b1c
> > > --- /dev/null
> > > +++ b/samples/blk_interposer/Makefile
> > > @@ -0,0 +1,2 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +obj-$(CONFIG_SAMPLE_BLK_INTERPOSER) += blk-interposer.o
> > > diff --git a/samples/blk_interposer/blk-interposer.c b/samples/blk_interposer/blk-interposer.c
> > > new file mode 100644
> > > index 000000000000..92b4c1fcf8f7
> > > --- /dev/null
> > > +++ b/samples/blk_interposer/blk-interposer.c
> > > @@ -0,0 +1,276 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +/*
> > > + * Block layer interposer allow to interpose bio requests from kernel module.
> > > + * This allows you to monitor requests, modify requests, add new request,
> > > + * or even redirect requests to another devices.
> > > + *
> > > + * This sample demonstrates how to use blk_interposer.
> > > + * It show how to properly connect the interposer module to kernel,
> > > + * and perform the simplest monitoring of the number of bio.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/types.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/blkdev.h>
> > > +#include <linux/genhd.h>
> > > +#include <linux/blk-mq.h>
> > > +
> > > +int device_major = 8;
> > > +int device_minor;
> > > +int fmode = FMODE_READ | FMODE_WRITE;
> > > +
> > > +/*
> > > + * Each interposer must have a common part in the form of the blk_interposer structure,
> > > + * as well as its own unique data.
> > > + */
> > > +struct my_interposer {
> > > +	/*
> > > +	 * Common part of block device interposer.
> > > +	 */
> > > +	struct blk_interposer interposer;
> > > +	/*
> > > +	 * Specific part for our interposer data.
> > > +	 */
> > > +	atomic_t counter;
> > > +};
> > > +
> > > +struct my_interposer my_ip;
> > > +
> > > +/**
> > > + * blk_interposer_attach - Attach interposer to disk
> > > + * @disk: target disk
> > > + * @interposer: block device interposer
> > > + */
> > > +static int blk_interposer_attach(struct gendisk *disk, struct blk_interposer *interposer)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	/*
> > > +	 * Stop disks queue processing.
> > > +	 */
> > > +	blk_mq_freeze_queue(disk->queue);
> > > +	blk_mq_quiesce_queue(disk->queue);
> > > +
> > > +	/*
> > > +	 * Check if the interposer is already busy.
> > > +	 * The interposer will only connect if it is not busy.
> > > +	 */
> > > +	if (blk_has_interposer(disk)) {
> > > +		pr_info("The interposer is already busy.\n");
> > > +		ret = -EBUSY;
> > > +		goto out;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Attach the interposer.
> > > +	 */
> > > +	disk->interposer = interposer;
> > > +	/*
> > > +	 * And while the queue is stopped, we can do something specific for our module.
> > > +	 */
> > > +	pr_info("Block device interposer attached successfully.\n");
> > > +
> > > +out:
> > > +	/*
> > > +	 * Resume disks queue processing
> > > +	 */
> > > +	blk_mq_unquiesce_queue(disk->queue);
> > > +	blk_mq_unfreeze_queue(disk->queue);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * blk_interposer_detach - Detach interposer from disk
> > > + * @disk: target disk
> > > + * @interposer: block device interposer
> > > + */
> > > +static int blk_interposer_detach(struct gendisk *disk, struct blk_interposer *interposer)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (WARN_ON(!disk))
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Stop disks queue processing.
> > > +	 */
> > > +	blk_mq_freeze_queue(disk->queue);
> > > +	blk_mq_quiesce_queue(disk->queue);
> > > +
> > > +	/*
> > > +	 * Check if the interposer is still available.
> > > +	 */
> > > +	if (!disk->interposer) {
> > > +		pr_info("The interposer is not available.\n");
> > > +		return -ENOENT;
> > > +		goto out;
> > > +	}
> > > +	/*
> > > +	 * Check if it is really our interposer.
> > > +	 */
> > > +	if (disk->interposer->ip_submit_bio != interposer->ip_submit_bio) {
> > > +		pr_info("The interposer found is not ours.\n");
> > > +		return -EPERM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Detach interposer.
> > > +	 */
> > > +	disk->interposer = NULL;
> > > +	/*
> > > +	 * And while the queue is stopped, we can do something specific for our module.
> > > +	 */
> > > +	pr_info("Block device interposer detached successfully.\n");
> > > +
> > > +out:
> > > +	/*
> > > +	 * Resume disks queue processing.
> > > +	 */
> > > +	blk_mq_unquiesce_queue(disk->queue);
> > > +	blk_mq_unfreeze_queue(disk->queue);
> > > +
> > > +	return ret;
> > > +}
> > 
> > This attach and detach code needs to be elevated out of samples so that
> > any future consumer of blk_interposer doesn't reinvent it.  It is far
> > too fundamental.
> > 
> > The way you've proposed this be merged is very much unacceptable.
> > 
> > Nacked-by: Mike Snitzer <snitzer@redhat.com>
> > 
> Yes, but on the other hand, while the queue is suspended, the module can perform
> some other actions specific to it.
> 
> And since the functions blk_mq_freeze_queue(), blk_mq_quiesce_queue(),
> blk_mq_unquiesce_queue() and blk_mq_unfreeze_queue() are public,
> the module creator can implement its module connection functionality regardless of
> whether we make the functions blk_interposer_attach() and blk_interposer_detach()
> in the kernel or not.
> 
> I'll think about it and try to come up with a better solution.

This practice of sprinkling code in samples/ subdir and getting a
minimalist hook into the kernel (with no actual in-kernel consumer) is
_not_ how we do things.

I'm not well-versed on the samples/ subdir nor when it came into
existence; but it should die in a fire!

Mike

[PATCH 3/3] block: blk_interposer - sample config

[Sergei Shtepa]

Attaches the blk_interposer sample to the kernel assembly.

Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>
---
 samples/Kconfig  | 7 +++++++
 samples/Makefile | 1 +
 2 files changed, 8 insertions(+)

diff --git a/samples/Kconfig b/samples/Kconfig
index 0ed6e4d71d87..72e2a9399e10 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -216,4 +216,11 @@ config SAMPLE_WATCH_QUEUE
 	  Build example userspace program to use the new mount_notify(),
 	  sb_notify() syscalls and the KEYCTL_WATCH_KEY keyctl() function.
 
+config SAMPLE_BLK_INTERPOSER
+	tristate "Builds the sample block layer interposer -- loadable module only"
+	depends on m
+	help
+	  Builds the sample block layer interposer kernel module to illustrate
+	  the use of blk_interposer feature.
+
 endif # SAMPLES
diff --git a/samples/Makefile b/samples/Makefile
index c3392a595e4b..953082c17249 100644
--- a/samples/Makefile
+++ b/samples/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_SAMPLE_INTEL_MEI)		+= mei/
 subdir-$(CONFIG_SAMPLE_WATCHDOG)	+= watchdog
 subdir-$(CONFIG_SAMPLE_WATCH_QUEUE)	+= watch_queue
 obj-$(CONFIG_DEBUG_KMEMLEAK_TEST)	+= kmemleak/
+obj-$(CONFIG_SAMPLE_BLK_INTERPOSER)	+= blk_interposer/
-- 
2.20.1

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Mike Snitzer]

On Wed, Dec 09 2020 at  8:01am -0500,
Sergei Shtepa <sergei.shtepa@veeam.com> wrote:

> Hi all.
> 
> I try to suggest the Block Layer Interposer (blk_interposer) again.
> It`s allows to intercept bio requests, remap bio to another devices
> or add new bios.
> 
> Initially, blk_interposer was designed to be compatible with
> device mapper. Our (my and Hannes) previous attempt to offer
> blk_interposer integrated with device mapper did not receive
> any comments from the dm-devel team, and without their help
> I will not be able to make a full implementation. I hope later
> they will have time to support blk_interposer in device mapper.

Excuse me?  I gave you quite a bit of early feedback!  I then went on
PTO for ~10 days, when I returned last week I had to deal with fixing
some 5.10 dm/block bio splitting regressions that only got resolved this
past Saturday in time for 5.10-rc7.

blk_interposer was/is on my short list to review closer (Hannes' version
that refined yours a bit more).. primarily to see if I could avoid the
extra clone and endio hooking.

The development window for 5.11 is past, so you pushing this without
using the approach discussed (in terms of DM) doesn't help your cause.

> And my blk-snap module requires an architecture change to
> support blk_interposer.
> 
> This time I offer it along with a sample.
> Despite the fact that blk_interposer is quite simple,
> there are a few non-obvious points that I would like to clarify.
> 
> However, I suggest the blk_interposer now so that people
> could discuss it and use it in their projects as soon as possible.

So you weren't willing to put the work in on something DM oriented
because you expected me to do the work for you?  And now you're looking
to side-step the consensus that was built because I didn't contribute
quick enough?  That's pretty messed up.

In the time-scale that is Linux kernel development.. you've really
jumped the gun and undercut my enthusiasm to work through the details.
I'd rather not read into your actions more than I already have here, but
I'm not liking what you've done here.  Kind of left in dismay with how
you decided to go down this path without a followup note to me or others
(that I'm aware of).

But I'm still willing to make blk_interposer work as we all discussed
(in terms of DM).

Mike

> Sergei Shtepa (3):
>   block: blk_interposer - Block Layer Interposer
>   block: blk_interposer - sample
>   block: blk_interposer - sample config
> 
>  block/blk-core.c                        |  32 +++
>  include/linux/blk_types.h               |   6 +-
>  include/linux/genhd.h                   |  12 +-
>  samples/Kconfig                         |   7 +
>  samples/Makefile                        |   1 +
>  samples/blk_interposer/Makefile         |   2 +
>  samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
>  7 files changed, 333 insertions(+), 3 deletions(-)
>  create mode 100644 samples/blk_interposer/Makefile
>  create mode 100644 samples/blk_interposer/blk-interposer.c
> 
> --
> 2.20.1
> 

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Sergei Shtepa]

The 12/09/2020 16:51, Mike Snitzer wrote:
> On Wed, Dec 09 2020 at  8:01am -0500,
> Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> 
> > Hi all.
> > 
> > I try to suggest the Block Layer Interposer (blk_interposer) again.
> > It`s allows to intercept bio requests, remap bio to another devices
> > or add new bios.
> > 
> > Initially, blk_interposer was designed to be compatible with
> > device mapper. Our (my and Hannes) previous attempt to offer
> > blk_interposer integrated with device mapper did not receive
> > any comments from the dm-devel team, and without their help
> > I will not be able to make a full implementation. I hope later
> > they will have time to support blk_interposer in device mapper.
> 
> Excuse me?  I gave you quite a bit of early feedback!  I then went on
> PTO for ~10 days, when I returned last week I had to deal with fixing
> some 5.10 dm/block bio splitting regressions that only got resolved this
> past Saturday in time for 5.10-rc7.

Mike,

I would like to clarify some points that I've made, and also try 
to repair the damage from the misunderstandings that I think have occured.

First of all, I actually meant the feedback on Hannes's patch which was
sent on the 19th of November:
https://lore.kernel.org/linux-block/20201119164924.74401-1-hare@suse.de/

Your feedback from the 18th of November ("[PATCH 4/4] dm_interposer - 
Try to use blk_interpose in dm") is very valuable, but I am not sure that
I am currently capable of implementing the proposed DM changes.
The overall architecture of DM is still not clear to me, and I am studying
it right now.

This new patch (the one that Hannes sent on the 19th of November) is also
compatibile with DM and should not pose any problems - the architecture is
the same. There are some changes that make blk_interposer simpler and better,
plus the sample is added.

> 
> blk_interposer was/is on my short list to review closer (Hannes' version
> that refined yours a bit more).. primarily to see if I could avoid the
> extra clone and endio hooking.

Glad to hear that! In order to avoid the additional copying one can only
change an intercepted bio, which might be dangerous.

> 
> The development window for 5.11 is past, so you pushing this without
> using the approach discussed (in terms of DM) doesn't help your cause.
> 
> > And my blk-snap module requires an architecture change to
> > support blk_interposer.
> > 
> > This time I offer it along with a sample.
> > Despite the fact that blk_interposer is quite simple,
> > there are a few non-obvious points that I would like to clarify.
> > 
> > However, I suggest the blk_interposer now so that people
> > could discuss it and use it in their projects as soon as possible.
> 
> So you weren't willing to put the work in on something DM oriented
> because you expected me to do the work for you?  And now you're looking
> to side-step the consensus that was built because I didn't contribute
> quick enough?  That's pretty messed up.

I just think that no one can implement integration of DM with
blk_interposer better than dm-devel team. I will certainly try my best,
but I am afraid that such efforts will only produce incongruous
DM patches that will be a waste of time (yours and mine).

> 
> In the time-scale that is Linux kernel development.. you've really
> jumped the gun and undercut my enthusiasm to work through the details.
> I'd rather not read into your actions more than I already have here, but
> I'm not liking what you've done here.  Kind of left in dismay with how
> you decided to go down this path without a followup note to me or others
> (that I'm aware of).

I am very sorry that I undercut your enthusiasm, but, as you rightly
pointed out, another development windows is closing, and my product
is still not able to work on newer Linux versions starting from 5.8.
That fact makes me particularly sad and forces me to search for different
means to draw some attention to blk_interposer. I've seen an RHEL 8.4
alpha recently, all looks very cool there but made me even more sad ...

Also I certainly remember a separate email that was sent to you on the
7th of December. Maybe it should have been written in the already
existing thread instead.

> 
> But I'm still willing to make blk_interposer work as we all discussed
> (in terms of DM).

I want it too. However, there is a certain difficulty with usage of DM
for backup copying. For instance, there is no changed block tracking (CBT)
and right now I don't have any clue how it could be implemented
considering the existing architecture. I still hope that sometime
in future I could be offer my blk-snap module which was specifically
created for backup copying purposes.

I apologize for causing all that confusion and mess and making you upset.
I hope that all of the above makes sense to you and you will not think
that I was slacking and tried to offload all the work to your team.

> 
> Mike
> 
> > Sergei Shtepa (3):
> >   block: blk_interposer - Block Layer Interposer
> >   block: blk_interposer - sample
> >   block: blk_interposer - sample config
> > 
> >  block/blk-core.c                        |  32 +++
> >  include/linux/blk_types.h               |   6 +-
> >  include/linux/genhd.h                   |  12 +-
> >  samples/Kconfig                         |   7 +
> >  samples/Makefile                        |   1 +
> >  samples/blk_interposer/Makefile         |   2 +
> >  samples/blk_interposer/blk-interposer.c | 276 ++++++++++++++++++++++++
> >  7 files changed, 333 insertions(+), 3 deletions(-)
> >  create mode 100644 samples/blk_interposer/Makefile
> >  create mode 100644 samples/blk_interposer/blk-interposer.c
> > 
> > --
> > 2.20.1
> > 
> 

-- 
Sergei Shtepa
Veeam Software developer.

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Mike Snitzer]

On Thu, Dec 10 2020 at  9:58am -0500,
Sergei Shtepa <sergei.shtepa@veeam.com> wrote:

> The 12/09/2020 16:51, Mike Snitzer wrote:
> > On Wed, Dec 09 2020 at  8:01am -0500,
> > Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> > 
> > > Hi all.
> > > 
> > > I try to suggest the Block Layer Interposer (blk_interposer) again.
> > > It`s allows to intercept bio requests, remap bio to another devices
> > > or add new bios.
> > > 
> > > Initially, blk_interposer was designed to be compatible with
> > > device mapper. Our (my and Hannes) previous attempt to offer
> > > blk_interposer integrated with device mapper did not receive
> > > any comments from the dm-devel team, and without their help
> > > I will not be able to make a full implementation. I hope later
> > > they will have time to support blk_interposer in device mapper.
> > 
> > Excuse me?  I gave you quite a bit of early feedback!  I then went on
> > PTO for ~10 days, when I returned last week I had to deal with fixing
> > some 5.10 dm/block bio splitting regressions that only got resolved this
> > past Saturday in time for 5.10-rc7.
> 
> Mike,
> 
> I would like to clarify some points that I've made, and also try 
> to repair the damage from the misunderstandings that I think have occured.
> 
> First of all, I actually meant the feedback on Hannes's patch which was
> sent on the 19th of November:
> https://lore.kernel.org/linux-block/20201119164924.74401-1-hare@suse.de/
> 
> Your feedback from the 18th of November ("[PATCH 4/4] dm_interposer - 
> Try to use blk_interpose in dm") is very valuable, but I am not sure that
> I am currently capable of implementing the proposed DM changes.
> The overall architecture of DM is still not clear to me, and I am studying
> it right now.
> 
> This new patch (the one that Hannes sent on the 19th of November) is also
> compatibile with DM and should not pose any problems - the architecture is
> the same. There are some changes that make blk_interposer simpler and better,
> plus the sample is added.
> 
> > 
> > blk_interposer was/is on my short list to review closer (Hannes' version
> > that refined yours a bit more).. primarily to see if I could avoid the
> > extra clone and endio hooking.
> 
> Glad to hear that! In order to avoid the additional copying one can only
> change an intercepted bio, which might be dangerous.
> 
> > 
> > The development window for 5.11 is past, so you pushing this without
> > using the approach discussed (in terms of DM) doesn't help your cause.
> > 
> > > And my blk-snap module requires an architecture change to
> > > support blk_interposer.
> > > 
> > > This time I offer it along with a sample.
> > > Despite the fact that blk_interposer is quite simple,
> > > there are a few non-obvious points that I would like to clarify.
> > > 
> > > However, I suggest the blk_interposer now so that people
> > > could discuss it and use it in their projects as soon as possible.
> > 
> > So you weren't willing to put the work in on something DM oriented
> > because you expected me to do the work for you?  And now you're looking
> > to side-step the consensus that was built because I didn't contribute
> > quick enough?  That's pretty messed up.
> 
> I just think that no one can implement integration of DM with
> blk_interposer better than dm-devel team. I will certainly try my best,
> but I am afraid that such efforts will only produce incongruous
> DM patches that will be a waste of time (yours and mine).
> 
> > 
> > In the time-scale that is Linux kernel development.. you've really
> > jumped the gun and undercut my enthusiasm to work through the details.
> > I'd rather not read into your actions more than I already have here, but
> > I'm not liking what you've done here.  Kind of left in dismay with how
> > you decided to go down this path without a followup note to me or others
> > (that I'm aware of).
> 
> I am very sorry that I undercut your enthusiasm, but, as you rightly
> pointed out, another development windows is closing, and my product
> is still not able to work on newer Linux versions starting from 5.8.
> That fact makes me particularly sad and forces me to search for different
> means to draw some attention to blk_interposer. I've seen an RHEL 8.4
> alpha recently, all looks very cool there but made me even more sad ...

Made you more sad because you don't have a working solution for upstream
or RHEL 8.4?

I may have missed it in your past emails but how were you able to
provide blk-snap support for kernels prior to 5.8?

> Also I certainly remember a separate email that was sent to you on the
> 7th of December. Maybe it should have been written in the already
> existing thread instead.

I don't see any mail from you on Dec 7... please bounce it to me if it
was in private, or please point to the relevant list archive.

> > 
> > But I'm still willing to make blk_interposer work as we all discussed
> > (in terms of DM).
> 
> I want it too. However, there is a certain difficulty with usage of DM
> for backup copying. For instance, there is no changed block tracking (CBT)
> and right now I don't have any clue how it could be implemented
> considering the existing architecture. I still hope that sometime
> in future I could be offer my blk-snap module which was specifically
> created for backup copying purposes.
> 
> I apologize for causing all that confusion and mess and making you upset.
> I hope that all of the above makes sense to you and you will not think
> that I was slacking and tried to offload all the work to your team.

My primary concern is that blk_interposer be correct from the start.  To
validate its correctness it needs to be fully implemented and vetted in
terms on upstream Linux kernel code.  DM can easily serve as the primary
upstream consumer until if/when your blk-snap module is proposed for
upstream inclusion.

But short of having an actual upstream consumer of blk_interposer (not
just samples/ code) it cannot go upstream.  Otherwise there are too many
risks of misuse and problems in the longrun.  That and it'd be baggage
block core would need to carry for no upstream Linux benefit.

Mike

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Mike Snitzer]

On Thu, Dec 10 2020 at 11:32am -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Dec 10 2020 at  9:58am -0500,
> Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> 
> > The 12/09/2020 16:51, Mike Snitzer wrote:
> > > On Wed, Dec 09 2020 at  8:01am -0500,
> > > Sergei Shtepa <sergei.shtepa@veeam.com> wrote:
> > > 
> > > > Hi all.
> > > > 
> > > > I try to suggest the Block Layer Interposer (blk_interposer) again.
> > > > It`s allows to intercept bio requests, remap bio to another devices
> > > > or add new bios.
> > > > 
> > > > Initially, blk_interposer was designed to be compatible with
> > > > device mapper. Our (my and Hannes) previous attempt to offer
> > > > blk_interposer integrated with device mapper did not receive
> > > > any comments from the dm-devel team, and without their help
> > > > I will not be able to make a full implementation. I hope later
> > > > they will have time to support blk_interposer in device mapper.
> > > 
> > > Excuse me?  I gave you quite a bit of early feedback!  I then went on
> > > PTO for ~10 days, when I returned last week I had to deal with fixing
> > > some 5.10 dm/block bio splitting regressions that only got resolved this
> > > past Saturday in time for 5.10-rc7.
> > 
> > Mike,
> > 
> > I would like to clarify some points that I've made, and also try 
> > to repair the damage from the misunderstandings that I think have occured.
> > 
> > First of all, I actually meant the feedback on Hannes's patch which was
> > sent on the 19th of November:
> > https://lore.kernel.org/linux-block/20201119164924.74401-1-hare@suse.de/
> > 
> > Your feedback from the 18th of November ("[PATCH 4/4] dm_interposer - 
> > Try to use blk_interpose in dm") is very valuable, but I am not sure that
> > I am currently capable of implementing the proposed DM changes.
> > The overall architecture of DM is still not clear to me, and I am studying
> > it right now.
> > 
> > This new patch (the one that Hannes sent on the 19th of November) is also
> > compatibile with DM and should not pose any problems - the architecture is
> > the same. There are some changes that make blk_interposer simpler and better,
> > plus the sample is added.
> > 
> > > 
> > > blk_interposer was/is on my short list to review closer (Hannes' version
> > > that refined yours a bit more).. primarily to see if I could avoid the
> > > extra clone and endio hooking.
> > 
> > Glad to hear that! In order to avoid the additional copying one can only
> > change an intercepted bio, which might be dangerous.
> > 
> > > 
> > > The development window for 5.11 is past, so you pushing this without
> > > using the approach discussed (in terms of DM) doesn't help your cause.
> > > 
> > > > And my blk-snap module requires an architecture change to
> > > > support blk_interposer.
> > > > 
> > > > This time I offer it along with a sample.
> > > > Despite the fact that blk_interposer is quite simple,
> > > > there are a few non-obvious points that I would like to clarify.
> > > > 
> > > > However, I suggest the blk_interposer now so that people
> > > > could discuss it and use it in their projects as soon as possible.
> > > 
> > > So you weren't willing to put the work in on something DM oriented
> > > because you expected me to do the work for you?  And now you're looking
> > > to side-step the consensus that was built because I didn't contribute
> > > quick enough?  That's pretty messed up.
> > 
> > I just think that no one can implement integration of DM with
> > blk_interposer better than dm-devel team. I will certainly try my best,
> > but I am afraid that such efforts will only produce incongruous
> > DM patches that will be a waste of time (yours and mine).
> > 
> > > 
> > > In the time-scale that is Linux kernel development.. you've really
> > > jumped the gun and undercut my enthusiasm to work through the details.
> > > I'd rather not read into your actions more than I already have here, but
> > > I'm not liking what you've done here.  Kind of left in dismay with how
> > > you decided to go down this path without a followup note to me or others
> > > (that I'm aware of).
> > 
> > I am very sorry that I undercut your enthusiasm, but, as you rightly
> > pointed out, another development windows is closing, and my product
> > is still not able to work on newer Linux versions starting from 5.8.
> > That fact makes me particularly sad and forces me to search for different
> > means to draw some attention to blk_interposer. I've seen an RHEL 8.4
> > alpha recently, all looks very cool there but made me even more sad ...
> 
> Made you more sad because you don't have a working solution for upstream
> or RHEL 8.4?
> 
> I may have missed it in your past emails but how were you able to
> provide blk-snap support for kernels prior to 5.8?

I now clearly understand that the 5.8 block changes to do away with
->make_request_fn in favor of a more optimized ->submit_bio (that isn't
applicable for all devices) is why you're pursuing a "fix" so urgently.

> > > But I'm still willing to make blk_interposer work as we all discussed
> > > (in terms of DM).
> > 
> > I want it too. However, there is a certain difficulty with usage of DM
> > for backup copying. For instance, there is no changed block tracking (CBT)
> > and right now I don't have any clue how it could be implemented
> > considering the existing architecture. I still hope that sometime
> > in future I could be offer my blk-snap module which was specifically
> > created for backup copying purposes.
> > 
> > I apologize for causing all that confusion and mess and making you upset.
> > I hope that all of the above makes sense to you and you will not think
> > that I was slacking and tried to offload all the work to your team.
> 
> My primary concern is that blk_interposer be correct from the start.  To
> validate its correctness it needs to be fully implemented and vetted in
> terms on upstream Linux kernel code.  DM can easily serve as the primary
> upstream consumer until if/when your blk-snap module is proposed for
> upstream inclusion.
> 
> But short of having an actual upstream consumer of blk_interposer (not
> just samples/ code) it cannot go upstream.  Otherwise there are too many
> risks of misuse and problems in the longrun.  That and it'd be baggage
> block core would need to carry for no upstream Linux benefit.

As I shared in private: I have some urgent Red Hat business critical
work I need to do and unfortunately cannot put my near-term focus to
implementing a fully baked blk_interposer for DM.  But I can come back
to it (sadly unlikely to do so until the new year though).

While I still think there needs to be a proper _upstream_ consumer of
blk_interposer as a condition of it going in.. I'll let others make the
call.

As such, I'll defer to Jens, Christoph and others on whether your
minimalist blk_interposer hook is acceptable in the near-term.

Mike

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Jens Axboe]

On 12/11/20 9:30 AM, Mike Snitzer wrote:
> While I still think there needs to be a proper _upstream_ consumer of
> blk_interposer as a condition of it going in.. I'll let others make the
> call.

That's an unequivocal rule.

> As such, I'll defer to Jens, Christoph and others on whether your
> minimalist blk_interposer hook is acceptable in the near-term.

I don't think so, we don't do short term bandaids just to plan on
ripping that out when the real functionality is there. IMHO, the dm
approach is the way to go - it provides exactly the functionality that
is needed in an appropriate way, instead of hacking some "interposer"
into the core block layer.

-- 
Jens Axboe

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Hannes Reinecke]

On 12/11/20 5:33 PM, Jens Axboe wrote:
> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>> While I still think there needs to be a proper _upstream_ consumer of
>> blk_interposer as a condition of it going in.. I'll let others make the
>> call.
> 
> That's an unequivocal rule.
> 
>> As such, I'll defer to Jens, Christoph and others on whether your
>> minimalist blk_interposer hook is acceptable in the near-term.
> 
> I don't think so, we don't do short term bandaids just to plan on
> ripping that out when the real functionality is there. IMHO, the dm
> approach is the way to go - it provides exactly the functionality that
> is needed in an appropriate way, instead of hacking some "interposer"
> into the core block layer.
> 
Which is my plan, too.

I'll be working with the Veeam folks to present a joint patchset 
(including the DM bits) for the next round.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Jens Axboe]

On 12/11/20 9:56 AM, Hannes Reinecke wrote:
> On 12/11/20 5:33 PM, Jens Axboe wrote:
>> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>>> While I still think there needs to be a proper _upstream_ consumer of
>>> blk_interposer as a condition of it going in.. I'll let others make the
>>> call.
>>
>> That's an unequivocal rule.
>>
>>> As such, I'll defer to Jens, Christoph and others on whether your
>>> minimalist blk_interposer hook is acceptable in the near-term.
>>
>> I don't think so, we don't do short term bandaids just to plan on
>> ripping that out when the real functionality is there. IMHO, the dm
>> approach is the way to go - it provides exactly the functionality that
>> is needed in an appropriate way, instead of hacking some "interposer"
>> into the core block layer.
>>
> Which is my plan, too.
> 
> I'll be working with the Veeam folks to present a joint patchset 
> (including the DM bits) for the next round.

Just to be clear, core block additions for something that dm will
consume is obviously fine. Adding this as block layer functionality than
then exposes an application API for setting it up, tearing down, etc -
that is definitely NOT

-- 
Jens Axboe

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Hannes Reinecke]

On 12/11/20 6:04 PM, Jens Axboe wrote:
> On 12/11/20 9:56 AM, Hannes Reinecke wrote:
>> On 12/11/20 5:33 PM, Jens Axboe wrote:
>>> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>>>> While I still think there needs to be a proper _upstream_ consumer of
>>>> blk_interposer as a condition of it going in.. I'll let others make the
>>>> call.
>>>
>>> That's an unequivocal rule.
>>>
>>>> As such, I'll defer to Jens, Christoph and others on whether your
>>>> minimalist blk_interposer hook is acceptable in the near-term.
>>>
>>> I don't think so, we don't do short term bandaids just to plan on
>>> ripping that out when the real functionality is there. IMHO, the dm
>>> approach is the way to go - it provides exactly the functionality that
>>> is needed in an appropriate way, instead of hacking some "interposer"
>>> into the core block layer.
>>>
>> Which is my plan, too.
>>
>> I'll be working with the Veeam folks to present a joint patchset
>> (including the DM bits) for the next round.
> 
> Just to be clear, core block additions for something that dm will
> consume is obviously fine. Adding this as block layer functionality than
> then exposes an application API for setting it up, tearing down, etc -
> that is definitely NOT
> 
That was never my intention.
Any consumers of this thing would need to be in-kernel.
If that was your concern.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Jens Axboe]

On 12/11/20 11:03 AM, Hannes Reinecke wrote:
> On 12/11/20 6:04 PM, Jens Axboe wrote:
>> On 12/11/20 9:56 AM, Hannes Reinecke wrote:
>>> On 12/11/20 5:33 PM, Jens Axboe wrote:
>>>> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>>>>> While I still think there needs to be a proper _upstream_ consumer of
>>>>> blk_interposer as a condition of it going in.. I'll let others make the
>>>>> call.
>>>>
>>>> That's an unequivocal rule.
>>>>
>>>>> As such, I'll defer to Jens, Christoph and others on whether your
>>>>> minimalist blk_interposer hook is acceptable in the near-term.
>>>>
>>>> I don't think so, we don't do short term bandaids just to plan on
>>>> ripping that out when the real functionality is there. IMHO, the dm
>>>> approach is the way to go - it provides exactly the functionality that
>>>> is needed in an appropriate way, instead of hacking some "interposer"
>>>> into the core block layer.
>>>>
>>> Which is my plan, too.
>>>
>>> I'll be working with the Veeam folks to present a joint patchset
>>> (including the DM bits) for the next round.
>>
>> Just to be clear, core block additions for something that dm will
>> consume is obviously fine. Adding this as block layer functionality than
>> then exposes an application API for setting it up, tearing down, etc -
>> that is definitely NOT
>>
> That was never my intention.
> Any consumers of this thing would need to be in-kernel.
> If that was your concern.

Yep, that is/was indeed my concern!

-- 
Jens Axboe

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Bob Liu]

Hi Folks,

On 12/12/20 12:56 AM, Hannes Reinecke wrote:
> On 12/11/20 5:33 PM, Jens Axboe wrote:
>> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>>> While I still think there needs to be a proper _upstream_ consumer of
>>> blk_interposer as a condition of it going in.. I'll let others make the
>>> call.
>>
>> That's an unequivocal rule.
>>
>>> As such, I'll defer to Jens, Christoph and others on whether your
>>> minimalist blk_interposer hook is acceptable in the near-term.
>>
>> I don't think so, we don't do short term bandaids just to plan on
>> ripping that out when the real functionality is there. IMHO, the dm
>> approach is the way to go - it provides exactly the functionality that
>> is needed in an appropriate way, instead of hacking some "interposer"
>> into the core block layer.
>>
> Which is my plan, too.
> 
> I'll be working with the Veeam folks to present a joint patchset (including the DM bits) for the next round.
> 

Besides the dm approach, do you think Veeam's original requirement is a good
use case of "block/bpf: add eBPF based block layer IO filtering"?
https://lwn.net/ml/bpf/20200812163305.545447-1-leah.rumancik@gmail.com/

Thanks,
Bob

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Hannes Reinecke]

On 12/15/20 7:51 AM, Bob Liu wrote:
> Hi Folks,
> 
> On 12/12/20 12:56 AM, Hannes Reinecke wrote:
>> On 12/11/20 5:33 PM, Jens Axboe wrote:
>>> On 12/11/20 9:30 AM, Mike Snitzer wrote:
>>>> While I still think there needs to be a proper _upstream_ consumer of
>>>> blk_interposer as a condition of it going in.. I'll let others make the
>>>> call.
>>>
>>> That's an unequivocal rule.
>>>
>>>> As such, I'll defer to Jens, Christoph and others on whether your
>>>> minimalist blk_interposer hook is acceptable in the near-term.
>>>
>>> I don't think so, we don't do short term bandaids just to plan on
>>> ripping that out when the real functionality is there. IMHO, the dm
>>> approach is the way to go - it provides exactly the functionality that
>>> is needed in an appropriate way, instead of hacking some "interposer"
>>> into the core block layer.
>>>
>> Which is my plan, too.
>>
>> I'll be working with the Veeam folks to present a joint patchset (including the DM bits) for the next round.
>>
> 
> Besides the dm approach, do you think Veeam's original requirement is a good
> use case of "block/bpf: add eBPF based block layer IO filtering"?
> https://lwn.net/ml/bpf/20200812163305.545447-1-leah.rumancik@gmail.com/
> 
That would actually a really cool use-case.
You could also consider a XDP-like functionality for eBPF, to move 
individual requests from one queue to the other; DM on steroids :-)

Should I try to include that patchset?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 0/3] block: blk_interposer - Block Layer Interposer

[Sergei Shtepa]

The 12/15/2020 09:51, Bob Liu wrote:
> Hi Folks,
> 
> On 12/12/20 12:56 AM, Hannes Reinecke wrote:
> > On 12/11/20 5:33 PM, Jens Axboe wrote:
> >> On 12/11/20 9:30 AM, Mike Snitzer wrote:
> >>> While I still think there needs to be a proper _upstream_ consumer of
> >>> blk_interposer as a condition of it going in.. I'll let others make the
> >>> call.
> >>
> >> That's an unequivocal rule.
> >>
> >>> As such, I'll defer to Jens, Christoph and others on whether your
> >>> minimalist blk_interposer hook is acceptable in the near-term.
> >>
> >> I don't think so, we don't do short term bandaids just to plan on
> >> ripping that out when the real functionality is there. IMHO, the dm
> >> approach is the way to go - it provides exactly the functionality that
> >> is needed in an appropriate way, instead of hacking some "interposer"
> >> into the core block layer.
> >>
> > Which is my plan, too.
> > 
> > I'll be working with the Veeam folks to present a joint patchset (including the DM bits) for the next round.
> > 
> 
> Besides the dm approach, do you think Veeam's original requirement is a good
> use case of "block/bpf: add eBPF based block layer IO filtering"?
> https://lwn.net/ml/bpf/20200812163305.545447-1-leah.rumancik@gmail.com/
> 
> Thanks,
> Bob

Hi Bob.
Thank you for your message.

I looked at your patch - it's interesting, but I have a few comments.

1.
#ifdef CONFIG_BPF_IO_FILTER
	struct bpf_prog_array __rcu *progs;
	struct mutex io_filter_lock;
#endif
For DM and blk-snap to work, it is necessary that there is always
the possibility of interception.

2.
We could get rid of the io_filter_lock lock if we do attach/detach while
the queue is stopped. And __rcu for *progs, can not use either.

3.
int io_filter_bpf_run(struct bio *bio)
{
	struct bpf_io_request io_req = {
		.sector_start = bio->bi_iter.bi_sector,
		.sector_cnt = bio_sectors(bio),
		.opf = bio->bi_opf,
	};
	return BPF_PROG_RUN_ARRAY_CHECK(bio->bi_disk->progs, &io_req, BPF_PROG_RUN);
}
Allows to get little information. It will not allow to work with the bios`s data.
blk_interposer allows to get full access to bio.
For use in the DM, we must also be able to add new bio's.

Summary:
For device-mapper purposes, bpf_io_filter is not suitable.
bpf_io_filter in this form is probably convenient to use for monitoring and
studying the block layer.
For the security task, I would suggest making a separate module and using
blk_interposer to intercept bio requests. This will give more flexible
functionality and better performance.

--
Sergei Shtepa
Veeam Software developer.